[Spice-devel] [PATCH v7 spice-gtk] Usbredir: enable lz4 compression
Snir Sheriber
ssheribe at redhat.com
Mon Jun 27 14:54:37 UTC 2016
Hi,
On 06/27/2016 04:44 PM, Victor Toso wrote:
> Hi,
>
> On Sun, Jun 26, 2016 at 04:44:42PM +0300, Snir Sheriber wrote:
>> Hi,
>>
>> On 06/24/2016 06:57 PM, Victor Toso wrote:
>>> 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 at redhat.com>
>>>> ---
>>>> 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.
>> yes, it should be better to send data of isochronous devices uncompressed
>> (although it is not guaranteed that isoc devices data is already
>> compressed).
>> as far as i looked with Uri there are two ways to do so:
>> 1. by looking for the type in each of the endpoints descriptors via
>> libusb
>> 2. to check if usbredir_buffered_output_size_callback is being called
>> and if it is, it means the device is isoc (kind of hackish)
>> what do you think should be used here?
> (1) should be better indeed. The problem with (2) is that the callback
> might not be called unless there are data to be dropped.
>
> For me, this could (should?) be an additional patch but let me know if
> you want me to have them upstream together otherwise I don't really mind
> to have this as it is (with fixes for build, but I can do that)
>
> Cheers,
> toso
>
Thanks!
Sure, in additional patch it will be better.
(and i can also do this build-fix & rebase if needed)
Snir.
>>>> +
>>>> + 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
>> Thanks! shouldn't be missed :/
>>>> + 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list