[Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets
Hans de Goede
hdegoede at redhat.com
Fri Oct 23 06:28:52 PDT 2015
Hi,
On 10/23/2015 09:53 AM, 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 isoc packet.
>
> usbredirhost uses the device information to set two levels of threshold.
> In case application's buffer size is higher then the higher threshold we
> will drop enough isoc packets to be able to provide a complete video
> frame again.
>
> The calculation aims at 10fps as worst case. The higher threshold is set
> to 180ms and the lower threshold to 30ms which would give us at least
> 150ms of continuous data.
>
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
> ---
> usbredirhost/usbredirhost.c | 62 +++++++++++++++++++++++++++++++++++++++++++--
> usbredirhost/usbredirhost.h | 14 ++++++++++
> 2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> index ad30722..adf9c5f 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_buffered_output_size buffered_output_size_func;
> 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,30 @@ static void usbredirhost_send_stream_status(struct usbredirhost *host,
> }
> }
>
> +static int usbredirhost_can_write_iso_package(struct usbredirhost *host)
> +{
> + uint64_t size;
> +
> + if (!host->buffered_output_size_func)
> + return true;
> +
> + size = host->buffered_output_size_func(host->func_priv);
> + 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 +1059,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))
> + usbredirparser_send_iso_packet(host->parser, id, &iso_packet,
> + data, len);
> break;
> }
> case usb_redir_type_bulk: {
> @@ -1120,6 +1153,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;
> + host->iso_threshold.lower = reference / 2;
> + host->iso_threshold.higher = reference * 3;
> + 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 +1221,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 +1405,17 @@ static void usbredirhost_log_data(struct usbredirhost *host, const char *desc,
>
> /**************************************************************************/
>
> +void usbredirhost_set_buffered_output_size_cb(struct usbredirhost *host,
> + usbredirhost_buffered_output_size buffered_output_size_func)
> +{
> + if (!host) {
> + ERROR("invalid usbredirhost");
> + return;
> + }
> +
> + host->buffered_output_size_func = buffered_output_size_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..042e084 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_buffered_output_size)(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,18 @@ 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_buffered_output_size callback should return the
> + application's buffer size (in bytes) that are handling the isochronous data.
applications *pending writes* buffer size (in bytes).
(so add the pending writes, drop the "that are ... isoc data", since the application
is not aware which data is isoc data and which is not.
Otherwise looks good, no need to do a new version, just push it with the above change:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> +
> + usbredirhost will set two levels of threshold based in the information
> + provided by the usb device. In case the application's buffer is increasing
> + too much then usbredirhost uses the threshold limits to drop isochronous
> + packages but still send full frames whenever is possible.
> +*/
> +void usbredirhost_set_buffered_output_size_cb(struct usbredirhost *host,
> + usbredirhost_buffered_output_size buffered_output_size_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.
>
Regards,
Hans
More information about the Spice-devel
mailing list