[Spice-devel] [usbredir PATCH v5 1/2] usbredirhost: new callback for iso streams
Hans de Goede
hdegoede at redhat.com
Thu Oct 22 10:57:53 PDT 2015
Hi,
On 22-10-15 17:41, Victor Toso wrote:
> 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.
Adding the above text to the commit msg is fine, also feel free to edit it
as needed to get a coherent commit msg.
Regards,
Hans
> 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