[Spice-devel] [usbredir PATCH v5 1/2] usbredirhost: new callback for iso streams
Victor Toso
victortoso at redhat.com
Thu Oct 22 08:41:28 PDT 2015
Hi,
On Thu, Oct 22, 2015 at 05:28:44PM +0200, Hans de Goede wrote:
> Hi,
>
> On 22-10-15 16:07, Victor Toso wrote:
> >For streaming devices it might be necessary from application to drop
> >data for different reasons. This patch provides a new callback that it
> >is called before queueing the most recent iso packages.
> >
> >Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
> >---
> > usbredirhost/usbredirhost.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
> > usbredirhost/usbredirhost.h | 12 +++++++++
> > 2 files changed, 73 insertions(+), 2 deletions(-)
> >
> >diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> >index ad30722..4c20bff 100644
> >--- a/usbredirhost/usbredirhost.c
> >+++ b/usbredirhost/usbredirhost.c
> >@@ -23,6 +23,7 @@
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <stdarg.h>
> >+#include <stdbool.h>
> > #include <string.h>
> > #include <errno.h>
> > #include <unistd.h>
> >@@ -109,6 +110,7 @@ struct usbredirhost {
> > usbredirparser_read read_func;
> > usbredirparser_write write_func;
> > usbredirhost_flush_writes flush_writes_func;
> >+ usbredirhost_can_write_iso can_write_iso_func;
>
> The can_write name no longer seems accurate, and from the
> user of libusbredirhost there is nothing iso[c] specific
> about this, so I would rename this to:
>
> usbredirhost_buffered_output_size buffered_output_size_func;
>
Sure!
> > void *func_priv;
> > int verbose;
> > libusb_context *ctx;
> >@@ -130,6 +132,11 @@ struct usbredirhost {
> > struct usbredirtransfer transfers_head;
> > struct usbredirfilter_rule *filter_rules;
> > int filter_rules_count;
> >+ struct {
> >+ uint64_t higher;
> >+ uint64_t lower;
> >+ bool dropping;
> >+ } iso_threshold;
> > };
> >
> > struct usbredirhost_dev_ids {
> >@@ -1003,6 +1010,31 @@ static void usbredirhost_send_stream_status(struct usbredirhost *host,
> > }
> > }
> >
> >+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
> >+ uint8_t ep)
> >+{
> >+ uint64_t size;
> >+
> >+ if (!host->can_write_iso_func)
> >+ return true;
> >+
> >+ size = host->can_write_iso_func(host->func_priv);
>
> Which will change this to:
>
> size = host->buffered_output_size_func(host->func_priv);
>
> Which seems to make more sense to me.
>
Agreed.
> >+ if (size >= host->iso_threshold.higher) {
> >+ if (!host->iso_threshold.dropping)
> >+ DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
> >+ size, host->iso_threshold.higher);
> >+ host->iso_threshold.dropping = true;
> >+ } else if (size < host->iso_threshold.lower) {
> >+ if (host->iso_threshold.dropping)
> >+ DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
> >+ size, host->iso_threshold.lower);
> >+
> >+ host->iso_threshold.dropping = false;
> >+ }
> >+
> >+ return !host->iso_threshold.dropping;
> >+}
> >+
> > static void usbredirhost_send_stream_data(struct usbredirhost *host,
> > uint64_t id, uint8_t ep, uint8_t status, uint8_t *data, int len)
> > {
> >@@ -1028,8 +1060,10 @@ static void usbredirhost_send_stream_data(struct usbredirhost *host,
> > .status = status,
> > .length = len,
> > };
> >- usbredirparser_send_iso_packet(host->parser, id, &iso_packet,
> >- data, len);
> >+
> >+ if (usbredirhost_can_write_iso_package(host, ep))
> >+ usbredirparser_send_iso_packet(host->parser, id, &iso_packet,
> >+ data, len);
> > break;
> > }
> > case usb_redir_type_bulk: {
> >@@ -1120,6 +1154,16 @@ static void usbredirhost_stop_stream(struct usbredirhost *host,
> > FLUSH(host);
> > }
> >
> >+static void usbredirhost_set_iso_threshold(struct usbredirhost *host,
> >+ uint8_t pkts_per_transfer, uint8_t transfer_count, uint16_t max_packetsize)
> >+{
> >+ uint64_t reference = pkts_per_transfer * transfer_count * max_packetsize;
>
> So this is based on the bufsize calculations in qemu, which aim for a buffer
> of circa 60 ms. And you try to keep any buffered writes (which not have yet
> moved to the os networkstack buffers) between 30 ms and 120 ms, this gives
> you a window of aprox 90 ms once you re-start collecting isoc packets, assuming
> the pipe is stalled. Those 90 ms may or may not contain a complete video frame,
> depending on camera framerate which may be as low as 5 fps / aka 200ms per frame.
>
> Now 5 fps is only seen in really low light conditions, so lets aim for 10 fps,
> still we need to tweak these values a bit, maybe we should increase the higher limit
> to reference * 3, giving us aprox 150 ms of continues data collection on a stalled
> pipe, which should give us at least 1 complete video frame in there at 10 fps.
>
Many thanks for explaining, the values make much more sense now. I'll
include this explanation in the commit log if you don't mind. I believe
that it could make easier future fixes and tweaks.
> Otherwise both patches look good, thanks for your work on this.
>
> Regards,
>
> Hans
Thanks for your help, I'll send a v6 with the changes.
Best regards,
Victor Toso
>
>
>
> >+ host->iso_threshold.lower = reference / 2;
> >+ host->iso_threshold.higher = reference * 2;
> >+ DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> >+ host->iso_threshold.higher, host->iso_threshold.lower);
> >+}
> >+
> > /* Called from both parser read and packet complete callbacks */
> > static void usbredirhost_alloc_stream_unlocked(struct usbredirhost *host,
> > uint64_t id, uint8_t ep, uint8_t type, uint8_t pkts_per_transfer,
> >@@ -1178,6 +1222,10 @@ static void usbredirhost_alloc_stream_unlocked(struct usbredirhost *host,
> > host->endpoint[EP2I(ep)].transfer[i], ISO_TIMEOUT);
> > libusb_set_iso_packet_lengths(
> > host->endpoint[EP2I(ep)].transfer[i]->transfer, pkt_size);
> >+
> >+ usbredirhost_set_iso_threshold(
> >+ host, pkts_per_transfer, transfer_count,
> >+ host->endpoint[EP2I(ep)].max_packetsize);
> > break;
> > case usb_redir_type_bulk:
> > libusb_fill_bulk_transfer(
> >@@ -1358,6 +1406,17 @@ static void usbredirhost_log_data(struct usbredirhost *host, const char *desc,
> >
> > /**************************************************************************/
> >
> >+void usbredirhost_set_cb_can_write_iso(struct usbredirhost *host,
> >+ usbredirhost_can_write_iso can_write_iso_func)
> >+{
> >+ if (!host) {
> >+ ERROR("invalid usbredirhost");
> >+ return;
> >+ }
> >+
> >+ host->can_write_iso_func = can_write_iso_func;
> >+}
> >+
> > /* Return value:
> > 0 All ok
> > 1 Packet borked, continue with next packet / urb
> >diff --git a/usbredirhost/usbredirhost.h b/usbredirhost/usbredirhost.h
> >index c0042c9..a03b10b 100644
> >--- a/usbredirhost/usbredirhost.h
> >+++ b/usbredirhost/usbredirhost.h
> >@@ -33,6 +33,8 @@ struct usbredirhost;
> >
> > typedef void (*usbredirhost_flush_writes)(void *priv);
> >
> >+typedef uint64_t (*usbredirhost_can_write_iso)(void *priv);
> >+
> > /* This function creates an usbredirhost instance, including its embedded
> > libusbredirparser instance and sends the initial usb_redir_hello packet to
> > the usb-guest.
> >@@ -114,6 +116,16 @@ void usbredirhost_close(struct usbredirhost *host);
> > int usbredirhost_set_device(struct usbredirhost *host,
> > libusb_device_handle *usb_dev_handle);
> >
> >+/* Call this function to set a callback in usbredirhost.
> >+ The usbredirhost_can_write_iso callback should return the application's
> >+ buffer size (in bytes) that are handling the isochronous data.
> >+ usbredirhost set two levels of threshold based in the information provided
> >+ by the device; if buffer size is higher then the higher threshold, usbredir
> >+ will drop isochronous packages till it reaches lower threshold.
> >+*/
> >+void usbredirhost_set_cb_can_write_iso(struct usbredirhost *host,
> >+ usbredirhost_can_write_iso can_write_iso_func);
> >+
> > /* Call this whenever there is data ready for the usbredirhost to read from
> > the usb-guest
> > returns 0 on success, or an error code from the below enum on error.
> >
More information about the Spice-devel
mailing list