Re: [Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets

2015-10-30 Thread Victor Toso 
Hi,

On Fri, Oct 23, 2015 at 03:28:52PM +0200, Hans de Goede wrote:
> Hi,
> 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 

Thanks this is now pushed as:
a88e197b18785d6de2322b5f26484c4130a6f2b9

with the follow up pre-release bump to 0.7.1 as:
e1a7e3dbbe091bfdc568372ff5ab18ed7eae972e

Many thanks for the help,
  Victor Toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets

2015-10-23 Thread Hans de Goede 

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 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -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, _packet,
-   data, len);
+
+if (usbredirhost_can_write_iso_package(host))
+usbredirparser_send_iso_packet(host->parser, id, _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

[Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets

2015-10-23 Thread Victor Toso 
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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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, _packet,
-   data, len);
+
+if (usbredirhost_can_write_iso_package(host))
+usbredirparser_send_iso_packet(host->parser, id, _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)
+{