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@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.
> 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
> > > +
> > > + 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel