Hi,
On 21-10-15 13:54, Victor Toso wrote:
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.
That can only happen if the 2 thresholds are too close to each other
or if your code has a bug.
Lets assume that the isoc data is the only data filling the channel
transmit queue, and that the lower threshold is 1MB and the high one
3MB, then with my above example we will start dropping packets once
the queue reaches 3MB of data, the queue will over time drain
dowbn to 1MB, then (quickly) fill up to 3 MB again. But while filling
up we've queues dup 2 MB of continues (so no dropped packets) of
video isoc packets. This 2 MB should contain at least 1 complete
video frame which the guest os can actually process.
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?
Yes it is wrong, a video frame can easily consists of 100-s of isoc
packets. So if you send through 1 packet on every 20, then the guest
will never get a complete video frame and just drop those packets
to the floor on the guest side since they are not part of a complete
video frame. So you've just wasted precious bandwidth by sending these
packets at all. The best thing we can do when the usb isoc data
rate is more then the network can handle is buffer + send a whole
bunch of packets, then stop for a while, then send a whole bunch again,
so that we hopefully have at least one complete video frame in each
"whole bunch of packets".
IOW lets say the webcam generates 30MB/s worth of data, but we only
have 15MB/s worth of bandwidth. Now lets assume that the cam is running
at 15 fps, so we've 2MB of data per frame. Your code does the equivalent
of send 100k, drop 100k, send 100k, drop 100k, and we never end up
sending a single complete video frame *ever*.
OTOH lets say we set the low threshold at 200ms of data and the high
treshold at 600ms of data (we can calculate this from the isoc transfer
count and pkts_per_transfer) So this means a low threshold of 6 MB and
a high threshold of 18MB, so now we send 12MB, drop 12MB, send 12MB, drop
12MB, and the guest will actually get a number of whole video frames to
display, then miss a few, then get a few more, and we actually have
a working video stream.
Also note that this is all about bandwidth, not latency. The usbredir code
will send isoc data as fast as the pipe can handle it, it does not wait
for any acknowledgements. So unless another layer (e.g. tcp) is
actually waiting for acks (tcp will do this, but in real world scenario
the tcp window should be large enough for this not to matter), then
latency does not matter! You are talking about latency in some of
your commit messages here, but this really is all about bandwidth, if
your traffic shaping used to test this is only introducing latency,
then you are likely still throttling bandwidth by introducing a high
latency without adjusting the tcp-window size to compensate.
Regards,
Hans
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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel