Hi,
On Mon, Jun 27, 2016 at 05:54:37PM +0300, Snir Sheriber wrote:
> 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@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.
I'm wrong here. The callback should always be called. But I still think
(1) is better as (2) would really be a workaround.
> >
> > 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.
Thanks :)
> > > > > +
> > > > > + 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