Hi,
On Thu, Apr 07, 2016 at 05:38:41PM +0300, Snir Sheriber wrote:
> On 04/04/2016 11:48 AM, Victor Toso wrote:
> >Hi,
> >
> >On Sun, Apr 03, 2016 at 06:40:09PM +0300, Snir Sheriber wrote:
> >>Compressed message type is CompressedData which contains compression
> >>type (1 byte) followed by the uncompressed data size (4 bytes) followed
> >>by the compressed data size (4 bytes) followed by the compressed data
> >>
> >>If SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4 capability is available &&
> >>data_size > COMPRESS_THRESHOLD data will be sent compressed otherwise
> >>data will be sent uncompressed (as well if compression has failed)
> >>---
> >> src/channel-usbredir.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++----
> >> 1 file changed, 89 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> >>index d95a6c5..cd5cebd 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
> >>@@ -52,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))
> >>@@ -195,6 +199,8 @@ 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));
> >>@@ -231,6 +237,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(
> >>@@ -544,11 +553,52 @@ static void usbredir_free_write_cb_data(uint8_t *data, void *user_data)
> >> usbredirhost_free_write_buffer(priv->host, data);
> >> }
> >>+#ifdef USE_LZ4
> >>+static void usbredir_free_write_cb_compressed_data(uint8_t *data, void *user_data)
> >>+{
> >>+ SpiceUsbredirChannel *channel = user_data;
> >>+ SpiceUsbredirChannelPrivate *priv = channel->priv;
> >>+
> >>+ usbredirhost_free_write_buffer(priv->host, (uint8_t*)SPICE_CONTAINEROF(data, SpiceMsgCompressedData, compressed_data));
> >>+}
> >>+#endif
> >>+
> >> static int usbredir_write_callback(void *user_data, uint8_t *data, int count)
> >> {
> >> SpiceUsbredirChannel *channel = user_data;
> >> SpiceMsgOut *msg_out;
> >>-
> >>+#ifdef USE_LZ4
> >>+ int bound;
> >>+
> >>+ if(count > COMPRESS_THRESHOLD &&
> >>+ spice_channel_test_capability(SPICE_CHANNEL(channel),
> >>+ SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4) &&
> >>+ ((bound = LZ4_compressBound(count)) != 0)) {
> >So, in this case, you are compressing all outgoing data in channel-usbredir.
> >
> >I'm not 100% positive but I don't think we should compress data from isoc
> >devices. They should already be compressed. What do you think?
> >
> >For other types of data in channel-usbredir and channel-webdav this is fine and
> >probably would safe a great deal of bandwidth. Have you done any measurements to
> >check the gains?
> >
> >Cheers,
> > toso
> Yes, you are right, when compressing data which is already compressed the
> compression ratio is not high (for example with the camera device), I'll see
> how can i identify isoc devices and maybe compress only those.
>
> I've made some testing- when compressing usb storage device with ntfs fs
> it's mostly depends on the files which are being transfer, the compression
> ratio can be 9:10 for compressed\binary files and up to 1:9 for
> documents\code\other files (and during mounting empty\not-full fs- the
> compression ratio is always very high since a lot of "empty" data is being
> sent). so basically the results seems pretty good for file systems (with the
> camera device the compression ratio was not high)
> The perf testing has shown that the cpu is not being significantly
> influenced by the compression.
Great to hear, thanks for the info!
>
> >>+ int compressed_data_count;
> >>+ SpiceMsgCompressedData *compressed_data_msg;
> >>+
> >>+ compressed_data_msg = (SpiceMsgCompressedData*)spice_malloc(sizeof(SpiceMsgCompressedData) + bound);
> >>+ compressed_data_msg->uncompressed_size = count;
> >>+ compressed_data_msg->type = SPICE_DATA_COMPRESSION_TYPE_LZ4;
> >>+ compressed_data_count = LZ4_compress_default((char*)data,
> >>+ (char*)compressed_data_msg->compressed_data, count, bound);
> >>+ if (compressed_data_count > 0) {
> >>+ compressed_data_msg->compressed_size = compressed_data_count;
> >>+ msg_out = spice_msg_out_new(SPICE_CHANNEL(channel),
> >>+ SPICE_MSGC_SPICEVMC_COMPRESSED_DATA);
> >>+ msg_out->marshallers->msg_SpiceMsgCompressedData(msg_out->marshaller, compressed_data_msg);
> >>+ spice_marshaller_add_ref_full(msg_out->marshaller,
> >>+ (uint8_t*)compressed_data_msg->compressed_data,
> >>+ compressed_data_count,
> >>+ usbredir_free_write_cb_compressed_data,
> >>+ channel);
> >>+ spice_msg_out_send(msg_out);
> >>+ return count;
> >>+ } else {/* free & fallback to sending the message uncompressed */
> >>+ free(compressed_data_msg);
> >>+ }
> >>+ }
> >>+#endif
> >> msg_out = spice_msg_out_new(SPICE_CHANNEL(channel),
> >> SPICE_MSGC_SPICEVMC_DATA);
> >> spice_marshaller_add_ref_full(msg_out->marshaller, data, count,
> >>@@ -639,19 +689,51 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
> >> SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
> >> SpiceUsbredirChannelPrivate *priv = channel->priv;
> >> device_error_data data;
> >>- int r, size;
> >>- uint8_t *buf;
> >>+ int r = 0;
> >> g_return_if_fail(priv->host != NULL);
> >> /* 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);
> >>+ uint32_t decompressed_size = 0;
> >>+ char* decompressed;
> >>+
> >>+ switch(compressed_data_msg->type) {
> >>+#ifdef USE_LZ4
> >>+ case SPICE_DATA_COMPRESSION_TYPE_LZ4:
> >>+ decompressed = (char*)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:
> >>+ decompressed = NULL;
> >>+ g_warning("Unknown Compression Type");
> >>+ r = usbredirhost_read_parse_error;
> >>+ }
> >>+ if (decompressed_size != compressed_data_msg->uncompressed_size) {
> >>+ g_warning("Decompress Error decompressed_size=%d expected=%d",
> >>+ decompressed_size, compressed_data_msg->uncompressed_size);
> >>+ r = usbredirhost_read_parse_error;
> >>+ } else {
> >>+ priv->read_buf_size = decompressed_size;
> >>+ priv->read_buf = (uint8_t*)decompressed;
> >>+ }
> >>+ } else { /*Regular SPICE_MSG_SPICEVMC_DATA msg*/
> >>+ int size;
> >>+ uint8_t *buf;
> >>+ buf = spice_msg_in_raw(in, &size);
> >>+ priv->read_buf_size = size;
> >>+ priv->read_buf = buf;
> >>+ }
> >>- 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;
> >> gchar *desc;
> >>--
> >>2.4.11
> >>
> >>_______________________________________________
> >>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