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

[Snir Sheriber]

Hi,

On 07/11/2016 07:44 PM, Marc-André Lureau wrote:
> Hi Snir
>
> On Mon, Jul 11, 2016 at 6:33 PM, Snir Sheriber <ssheribe at redhat.com> 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
>>
>> E.g.
>> Camera/mic device will usually be recognised as isochronous
>> while storage devices won't
> But what's the rationale for disabling isochronous devices
> compression? The data is assumed to be compressed already?
Yes, exactly (i should have mentioned it :/), It is assumed that there 
is a strong correlation
between isochronous devices and devices which their data is usually 
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      | 35 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>> index 9c71c07..0accd44 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..9c39cf6 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,30 @@ 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 = FALSE;
>> +    gint i,j,k;
>> +
>> +    g_return_val_if_fail(libdev != NULL, FALSE);
>> +    g_return_val_if_fail(!libusb_get_active_config_descriptor(libdev, &conf_desc), FALSE);
>> +
>> +    for(i = 0; !isoc_found && i < conf_desc->bNumInterfaces; i++) {
>> +        for(j = 0; !isoc_found && j < conf_desc->interface[i].num_altsetting; j++) {
>> +            for(k = 0; !isoc_found && k < conf_desc->interface[i].altsetting[j].bNumEndpoints;k++) {
>> +                gint attributes = conf_desc->interface[i].altsetting[j].endpoint[k].bmAttributes;
>> +                gint type = attributes & LIBUSB_TRANSFER_TYPE_MASK;
>> +                if (type == LIBUSB_TRANSFER_TYPE_ISOCHRONOUS)
>> +                    isoc_found = TRUE;
>> +            }
>> +        }
>> +    }
>> +
>> +    libusb_free_config_descriptor(conf_desc);
>> +    return isoc_found;
>> +}
>> +
>>   /*
>>    * SpiceUsbDeviceInfo
>>    */
>> @@ -2042,6 +2067,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);
>>   #ifndef G_OS_WIN32
>>       info->libdev = libusb_ref_device(libdev);
>>   #endif
>> @@ -2085,6 +2111,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
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>