Re: [PATCH v7 spice-gtk] Usbredir: enable lz4 compression

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Mon, Jun 13, 2016 at 07:54:34PM +0300, Snir Sheriber wrote:
> Compressed message type is CompressedData which contains compression
> type (1 byte) followed by the uncompressed data size (4 bytes-exists
> only if data was compressed) followed by the compressed data
>
> If SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4 capability is available &&
> data_size > COMPRESS_THRESHOLD && !AF_LOCAL data will be sent
> compressed otherwise data will be sent uncompressed (as well if
> compression has failed)
>
> Update required spice-protocol to 0.12.12

Now that we did the spice-gtk release, updating the spice-common and
bumping the spice-protocol requirement should be okay.

I tested this before but I plan to double check it today, if no
problems and no one complains, I plan to push this next by Monday.

Minor comments below:

> 
> Signed-off-by: Snir Sheriber <ssheribe@xxxxxxxxxx>
> ---
>  configure.ac           |   2 +-
>  src/channel-usbredir.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 124 insertions(+), 7 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 3fe8055..f2acb23 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -69,7 +69,7 @@ AC_CHECK_LIBM
>  AC_SUBST(LIBM)
>  
>  AC_CONFIG_SUBDIRS([spice-common])
> -PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.11])
> +PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.12])
>  
>  COMMON_CFLAGS='-I ${top_srcdir}/spice-common/ ${SPICE_PROTOCOL_CFLAGS}'
>  AC_SUBST(COMMON_CFLAGS)
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 2c5feae..bb69e26 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -24,6 +24,9 @@
>  #ifdef USE_USBREDIR
>  #include <glib/gi18n.h>
>  #include <usbredirhost.h>
> +#ifdef USE_LZ4
> +#include <lz4.h>
> +#endif
>  #ifdef USE_POLKIT
>  #include "usb-acl-helper.h"
>  #endif
> @@ -32,6 +35,7 @@
>  #include "usbutil.h"
>  #endif
>  
> +#include "common/log.h"
>  #include "spice-client.h"
>  #include "spice-common.h"
>  
> @@ -51,6 +55,7 @@
>  
>  #ifdef USE_USBREDIR
>  
> +#define COMPRESS_THRESHOLD 1000
>  #define SPICE_USBREDIR_CHANNEL_GET_PRIVATE(obj)                                  \
>      (G_TYPE_INSTANCE_GET_PRIVATE((obj), SPICE_TYPE_USBREDIR_CHANNEL, SpiceUsbredirChannelPrivate))
>  
> @@ -233,6 +238,7 @@ static void channel_set_handlers(SpiceChannelClass *klass)
>  {
>      static const spice_msg_handler handlers[] = {
>          [ SPICE_MSG_SPICEVMC_DATA ] = usbredir_handle_msg,
> +        [ SPICE_MSG_SPICEVMC_COMPRESSED_DATA ] = usbredir_handle_msg,
>      };
>  
>      spice_channel_set_handlers(klass, handlers, G_N_ELEMENTS(handlers));
> @@ -269,6 +275,9 @@ void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
>  #if USBREDIR_VERSION >= 0x000701
>      usbredirhost_set_buffered_output_size_cb(priv->host, usbredir_buffered_output_size_callback);
>  #endif
> +#ifdef USE_LZ4
> +    spice_channel_set_capability(channel, SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> +#endif
>  }
>  
>  static gboolean spice_usbredir_channel_open_device(
> @@ -632,11 +641,71 @@ static void usbredir_free_write_cb_data(uint8_t *data, void *user_data)
>      usbredirhost_free_write_buffer(priv->host, data);
>  }
>  
> +#ifdef USE_LZ4
> +static int try_write_compress_LZ4(SpiceUsbredirChannel *channel, uint8_t *data, int count) {
> +    SpiceChannelPrivate *c;
> +    SpiceMsgOut *msg_out_compressed;
> +    int bound, compressed_data_count;
> +    uint8_t *compressed_buf;
> +    SpiceMsgCompressedData compressed_data_msg = {
> +            .type = SPICE_DATA_COMPRESSION_TYPE_LZ4,
> +            .uncompressed_size = count
> +        };
> +
> +    c = SPICE_CHANNEL(channel)->priv;
> +    if (g_socket_get_family(c->sock) == G_SOCKET_FAMILY_UNIX) {
> +        /* AF_LOCAL socket - data will not be compressed */
> +        return FALSE;
> +    }
> +    if (count <= COMPRESS_THRESHOLD) {
> +        /* Not enough data to compress */
> +        return FALSE;
> +    }
> +    if (!spice_channel_test_capability(SPICE_CHANNEL(channel),
> +                                       SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4)) {
> +        /* No server compression capability - data will not be compressed */
> +        return FALSE;
> +    }
> +    bound = LZ4_compressBound(count);
> +    if (bound == 0) {
> +        /* Invalid bound - data will not be compressed */
> +        return FALSE;
> +    }

What about devices of isochronous type? Here, the packages for my webcam
were < COMPRESS_THRESHOLD but I guess we could save some time/process in
general by avoiding compressing in case the device is isoc.

> +
> +    compressed_buf = (uint8_t*)spice_malloc(bound);
> +    compressed_data_count = LZ4_compress_default((char*)data,
> +                                                 (char*)compressed_buf,
> +                                                 count,
> +                                                 bound);
> +    if (compressed_data_count > 0 && compressed_data_count < count) {
> +        compressed_data_msg.compressed_data = compressed_buf;
> +        msg_out_compressed = spice_msg_out_new(SPICE_CHANNEL(channel),
> +                                               SPICE_MSGC_SPICEVMC_COMPRESSED_DATA);
> +        msg_out_compressed->marshallers->msg_SpiceMsgCompressedData(msg_out_compressed->marshaller,
> +                                                                    &compressed_data_msg);
> +        spice_marshaller_add_ref_full(msg_out_compressed->marshaller,
> +                                      compressed_data_msg.compressed_data,
> +                                      compressed_data_count,
> +                                      usbredir_free_write_cb_data,
> +                                      channel);
> +        spice_msg_out_send(msg_out_compressed);
> +        return TRUE;
> +    }
> +    /* if not - free & fallback to sending the message uncompressed */
> +    free(compressed_buf);
> +    return FALSE;
> +}
> +#endif
> +
>  static int usbredir_write_callback(void *user_data, uint8_t *data, int count)
>  {
>      SpiceUsbredirChannel *channel = user_data;
>      SpiceMsgOut *msg_out;
>  
> +#ifdef USE_LZ4
> +    if (try_write_compress_LZ4(channel, data, count))
> +        return count;
> +#endif
>      msg_out = spice_msg_out_new(SPICE_CHANNEL(channel),
>                                  SPICE_MSGC_SPICEVMC_DATA);
>      spice_marshaller_add_ref_full(msg_out->marshaller, data, count,
> @@ -724,11 +793,49 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
>      usbredirhost_write_guest_data(priv->host);
>  }
>  
> +static int try_handle_compressed_msg(SpiceMsgCompressedData *compressed_data_msg,
> +                                     uint8_t **buf,
> +                                     int *size) {
> +    int decompressed_size = 0;
> +    char *decompressed = NULL;
> +
> +    if (compressed_data_msg->uncompressed_size == 0) {
> +        spice_warning("Invalid uncompressed_size");
> +        return FALSE;
> +    }
> +
> +    switch (compressed_data_msg->type) {
> +#ifdef USE_LZ4
> +    case SPICE_DATA_COMPRESSION_TYPE_LZ4:
> +        decompressed = g_malloc(compressed_data_msg->uncompressed_size);
> +        decompressed_size = LZ4_decompress_safe ((char*)compressed_data_msg->compressed_data,
> +                                                 decompressed,
> +                                                 compressed_data_msg->compressed_size,
> +                                                 compressed_data_msg->uncompressed_size);
> +        break;
> +#endif
> +    default:
> +        spice_warning("Unknown Compression Type");
> +        return FALSE;
> +    }
> +    if (decompressed_size != compressed_data_msg->uncompressed_size) {
> +        spice_warning("Decompress Error decompressed_size=%d expected=%d",
> +                      decompressed_size, compressed_data_msg->uncompressed_size);

with -Werror-format, build fails here with:
 error: format ‘%d’ expects argument of type ‘int’, but argument 7 has
 type ‘uint32_t {aka unsigned int}’ expected=%d",

Cheers,
  toso

> +        g_free(decompressed);
> +        return FALSE;
> +    }
> +
> +    *size = decompressed_size;
> +    *buf = (uint8_t*)decompressed;
> +    return TRUE;
> +
> +}
> +
>  static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
>  {
>      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
> -    int r, size;
> +    int r = 0, size;
>      uint8_t *buf;
>  
>      g_return_if_fail(priv->host != NULL);
> @@ -736,13 +843,23 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
>      /* No recursion allowed! */
>      g_return_if_fail(priv->read_buf == NULL);
>  
> -    buf = spice_msg_in_raw(in, &size);
> -    priv->read_buf = buf;
> -    priv->read_buf_size = size;
> +    if (spice_msg_in_type(in) == SPICE_MSG_SPICEVMC_COMPRESSED_DATA) {
> +        SpiceMsgCompressedData *compressed_data_msg = spice_msg_in_parsed(in);
> +        if (try_handle_compressed_msg(compressed_data_msg, &buf, &size)) {
> +            priv->read_buf_size = size;
> +            priv->read_buf = buf;
> +        } else {
> +            r = usbredirhost_read_parse_error;
> +        }
> +    } else { /* Regular SPICE_MSG_SPICEVMC_DATA msg */
> +        buf = spice_msg_in_raw(in, &size);
> +        priv->read_buf_size = size;
> +        priv->read_buf = buf;
> +    }
>  
>      spice_usbredir_channel_lock(channel);
> -
> -    r = usbredirhost_read_guest_data(priv->host);
> +    if (r == 0)
> +        r = usbredirhost_read_guest_data(priv->host);
>      if (r != 0) {
>          SpiceUsbDevice *spice_device = priv->spice_device;
>          device_error_data err_data;
> -- 
> 2.5.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]