[Spice-devel] [usbredir PATCH v4 1/2] usbredirhost: new callback for iso streams
Victor Toso
victortoso at redhat.com
Wed Oct 21 04:54:17 PDT 2015
Hi,
On Wed, Oct 21, 2015 at 01:40:01PM +0200, Hans de Goede wrote:
> Hi,
>
> On 21-10-15 12:38, 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 | 70 +++++++++++++++++++++++++++++++++++++++++++--
> > usbredirhost/usbredirhost.h | 13 +++++++++
> > 2 files changed, 81 insertions(+), 2 deletions(-)
> >
> >diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> >index ad30722..1eaa13e 100644
> >--- a/usbredirhost/usbredirhost.c
> >+++ b/usbredirhost/usbredirhost.c
> >@@ -109,6 +109,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;
> > void *func_priv;
> > int verbose;
> > libusb_context *ctx;
> >@@ -130,6 +131,11 @@ struct usbredirhost {
> > struct usbredirtransfer transfers_head;
> > struct usbredirfilter_rule *filter_rules;
> > int filter_rules_count;
> >+ struct {
> >+ uint64_t higher;
> >+ uint64_t lower;
> >+ uint64_t packets;
> >+ } iso_threshold;
> > };
> >
> > struct usbredirhost_dev_ids {
> >@@ -1003,6 +1009,50 @@ static void usbredirhost_send_stream_status(struct usbredirhost *host,
> > }
> > }
> >
> >+/* This function should always be called with LOCK in the host */
> >+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
> >+ uint8_t ep)
> >+{
> >+ uint64_t size;
> >+ int can_write_packet = 1;
> >+
> >+ if (!host->can_write_iso_func)
> >+ return can_write_packet;
> >+
> >+ size = host->can_write_iso_func(host->func_priv);
> >+ if (size >= host->iso_threshold.higher) {
> >+ /* Drop some packages, this one included */
> >+ can_write_packet = 0;
> >+ host->endpoint[EP2I(ep)].drop_packets =
> >+ (host->endpoint[EP2I(ep)].pkts_per_transfer *
> >+ host->endpoint[EP2I(ep)].transfer_count) * 2;
> >+ goto end;
> >+ }
> >+
> >+ if (size >= host->iso_threshold.lower) {
> >+ /* While we are in the lower threshold, we drop a fixed interval
> >+ * of iso packages so client application can keep streaming without
> >+ * buffering too fast */
> >+ if (host->iso_threshold.packets == 0) {
> >+ host->iso_threshold.packets =
> >+ (host->endpoint[EP2I(ep)].pkts_per_transfer *
> >+ host->endpoint[EP2I(ep)].transfer_count);
> >+ host->endpoint[EP2I(ep)].drop_packets = host->iso_threshold.packets;
> >+ can_write_packet = 0;
> >+ } else {
> >+ host->iso_threshold.packets--;
> >+ }
> >+ goto end;
> >+ }
> >+
> >+ /* In case we were above lower threshold, reset package counter */
>
> Comment should say below lower threshold.
>
> >+ if (host->iso_threshold.packets != 0) {
> >+ host->iso_threshold.packets = 0;
> >+ }
> >+end:
> >+ return can_write_packet;
> >+}
> >+
>
> This function seems over complicated, I would expect something like this:
>
> struct {
> uint64_t higher;
> uint64_t lower;
> bool dropping;
> } iso_threshold;
>
> With dropping being initialized to false;
>
> And then:
>
> static bool 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);
> if (size >= host->iso_threshold.higher)
> host->iso_threshold.dropping = true;
> else if (size < host->iso_threshold.lower)
> host->iso_threshold.dropping = false;
>
> return !iso_threshold.dropping;
> }
>
> This way you make the lower threshold a proper hysteresis
> and you stop the code from oscillating around the higher limit
> causing pretty much every video frame to become corrupted.
>
> As an added bonus the code above is much easier to understand
> then your original implementation.
>
Yes, I started with this implementation at first but it seems to have
the same issue that we had in other versions: We endup dropping every
other package instead of dropping a bunch of packages at once.
My idea with this implementation is that we drop frames while we are
above lower threshold but not all of them, so we can still keep some
sort of stream going on when we have high latency. Is it wrong?
>
> > 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 +1078,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: {
> >@@ -1358,6 +1410,20 @@ 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,
> >+ uint64_t threshold_higher, uint64_t threshold_lower)
> >+{
> >+ if (!host) {
> >+ ERROR("invalid usbredirhost");
> >+ return;
> >+ }
> >+
> >+ host->can_write_iso_func = can_write_iso_func;
> >+ host->iso_threshold.higher = threshold_higher;
> >+ host->iso_threshold.lower = threshold_lower;
>
> I would not let the thresholds be set by the caller, but instead
> calculate them when the isoc streaming starts based on the transfer
> count and packets per transfer.
>
I agree. Can you help me a little with some snip as how to calculate,
let's say, the size or amount of packages for 200 ms of
stream?
> Regards,
>
> Hans
>
Thanks,
toso
>
> >+}
> >+
> > /* 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..784fefd 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,17 @@ 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 and two threshold levels in usbredirhost.
> >+ The usbredirhost_can_write_iso callback should return the application's
> >+ buffer size (in bytes) that are handling the isochronous data.
> >+ If the buffer size is between lower and higher threshold, usbredirhost will
> >+ drop frames before delivering to application; If the buffer size is bigger
> >+ then maximum threshold, usbredirhost will drop all the frames.
> >+*/
> >+void usbredirhost_set_cb_can_write_iso(struct usbredirhost *host,
> >+ usbredirhost_can_write_iso can_write_iso_func,
> >+ uint64_t threshold_higher, uint64_t threshold_lower);
> >+
> > /* 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