[Spice-devel] [PATCH 2/2] Usbredir: Avoid compression of isochronous devices

[Victor Toso]

Hi,

On Sun, Jul 10, 2016 at 07:18:23PM +0300, Snir Sheriber wrote:
> Device is considered isochronous if one of its endpoints is
> defined as isochronous transfer, in that case data transfer
> over the usbredir channel will not be compressed
> 
> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> ---
>  src/channel-usbredir.c        |  9 +++++++++
>  src/usb-device-manager-priv.h |  1 +
>  src/usb-device-manager.c      | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index d420a96..ca8ed99 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -544,6 +544,11 @@ void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel *channe
>      g_object_unref(task);
>  }
>  
> +static SpiceUsbDevice *spice_usbredir_channel_get_spice_usb_device(SpiceUsbredirChannel *channel)
> +{
> +    return channel->priv->spice_device;
> +}
> +
>  G_GNUC_INTERNAL
>  libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
>  {
> @@ -672,6 +677,10 @@ static int try_write_compress_LZ4(SpiceUsbredirChannel *channel, uint8_t *data,
>          /* No server compression capability - data will not be compressed */
>          return FALSE;
>      }
> +    if(spice_usb_device_is_isochronous(spice_usbredir_channel_get_spice_usb_device(channel))) {
> +        /* Don't compress - one of the device endpoints is isochronous */
> +        return FALSE;
> +    }
>      bound = LZ4_compressBound(count);
>      if (bound == 0) {
>          /* Invalid bound - data will not be compressed */
> diff --git a/src/usb-device-manager-priv.h b/src/usb-device-manager-priv.h
> index b6fa9c9..83884d7 100644
> --- a/src/usb-device-manager-priv.h
> +++ b/src/usb-device-manager-priv.h
> @@ -40,6 +40,7 @@ guint8 spice_usb_device_get_busnum(const SpiceUsbDevice *device);
>  guint8 spice_usb_device_get_devaddr(const SpiceUsbDevice *device);
>  guint16 spice_usb_device_get_vid(const SpiceUsbDevice *device);
>  guint16 spice_usb_device_get_pid(const SpiceUsbDevice *device);
> +gboolean spice_usb_device_is_isochronous(const SpiceUsbDevice *device);
>  
>  #endif
>  
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 53505fa..2cb265b 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -153,6 +153,7 @@ typedef struct _SpiceUsbDeviceInfo {
>      guint8  devaddr;
>      guint16 vid;
>      guint16 pid;
> +    gboolean isochronous;
>  #ifdef G_OS_WIN32
>      guint8  state;
>  #else
> @@ -2017,6 +2018,32 @@ gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *for
>  
>  
>  #ifdef USE_USBREDIR
> +
> +static gboolean probe_isochronous_endpoint(libusb_device *libdev) {
> +    struct libusb_config_descriptor * conf_desc;
> +    gboolean isoc_found;
> +
> +    if(libdev != NULL) {
Does g_return_vail_if fail fits here? Otherwise, an early return should
do:

if (libdev == NULL)
    return;

> +        if(!libusb_get_active_config_descriptor(libdev, &conf_desc)) {

Ditto.

i, j and k should be declared outside of for

> +            for(int i=0;i<conf_desc->bNumInterfaces;i++) {
> +                for(int j=0;j<conf_desc->interface[i].num_altsetting;j++) {
> +                    for(int k=0;k < conf_desc->interface[i].altsetting[j].bNumEndpoints;k++) {

Maybe a helper here would be nice, like:

gint attributes = conf_desc->interface[i].altsetting[j].endpoint[k].bmAttributes;
gint type = attributes & LIBUSB_TRANSFER_TYPE_MASK;
if (type == LIBUSB_TRANSFER_TYPE_ISOCHRONOUS)

> +                        if ((conf_desc->interface[i].altsetting[j].endpoint[k].bmAttributes &
> +                            LIBUSB_TRANSFER_TYPE_MASK) == LIBUSB_TRANSFER_TYPE_ISOCHRONOUS) {
> +                            isoc_found = TRUE;
> +                            goto done;

If you take the suggestion bellow, a break here will do

> +                        }
> +                    }
> +                }
> +            }
> +        }
> +    }
> +    isoc_found = FALSE;
> +done:

Then you can remove the goto+label if you move isoc_found = FALSE before
the for loop

> +    libusb_free_config_descriptor(conf_desc);
> +    return isoc_found;
> +}
> +
>  /*
>   * SpiceUsbDeviceInfo
>   */
> @@ -2042,6 +2069,7 @@ static SpiceUsbDeviceInfo *spice_usb_device_new(libusb_device *libdev)
>      info->vid = vid;
>      info->pid = pid;
>      info->ref = 1;
> +    info->isochronous = probe_isochronous_endpoint(libdev);

Yes, on device_new should be enough

>  #ifndef G_OS_WIN32
>      info->libdev = libusb_ref_device(libdev);
>  #endif
> @@ -2085,6 +2113,15 @@ guint16 spice_usb_device_get_pid(const SpiceUsbDevice *device)
>      return info->pid;
>  }
>  
> +gboolean spice_usb_device_is_isochronous(const SpiceUsbDevice *device)
> +{
> +    const SpiceUsbDeviceInfo *info = (const SpiceUsbDeviceInfo *)device;
> +
> +    g_return_val_if_fail(info != NULL, 0);
> +
> +    return info->isochronous;
> +}
> +
>  #ifdef G_OS_WIN32
>  void spice_usb_device_set_state(SpiceUsbDevice *device, guint8 state)
>  {
> -- 
> 2.5.5
> 

Which devices have you tested? Might be interesting to include them on
commit-log like "tested with: .."

Many thanks!

Reviewed-by: Victor Toso <victortoso at redhat.com>

> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel