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. > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel