[Spice-devel] [usbredir PATCH v4 1/2] usbredirhost: new callback for iso streams

[Hans de Goede]

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.


>   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.

Regards,

Hans


> +}
> +
>   /* 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.
>