[Spice-devel] [PATCH 2/5] improve lz4 protocol

[Frediano Ziglio]

> 
> On 05/16/2016 06:59 PM, Frediano Ziglio wrote:
> > Reduce size if not compressed and compute compressed size using message
> > length
> >
> > This should be merged in previous patch
> > ---
> >  common/messages.h |  2 +-
> >  spice.proto       | 17 +++++++++++++----
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/common/messages.h b/common/messages.h
> > index d001850..516a345 100644
> > --- a/common/messages.h
> > +++ b/common/messages.h
> > @@ -59,7 +59,7 @@ typedef struct SpiceMsgCompressedData {
> >      uint8_t type;
> >      uint32_t uncompressed_size;
> >      uint32_t compressed_size;
> > -    uint8_t compressed_data[0];
> > +    uint8_t *compressed_data;
> >  } SpiceMsgCompressedData;
> >
> >  typedef struct SpiceMsgEmpty {
> > diff --git a/spice.proto b/spice.proto
> > index a515aef..0bfc515 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -121,16 +121,25 @@ message Data {
> >  } @nocopy;
> >
> >  enum8 data_compression_type {
> > -    INVALID,
> >      NONE,
> >      LZ4,
> >  };
> >
> > +struct EmptyStructure {
> > +};
> > +
> >  message CompressedData {
> >      data_compression_type type;
> > -    uint32 uncompressed_size;
> > -    uint32 compressed_size;
> > -    uint8 compressed_data[] @end;
> > +    switch (type) {
> > +    /* we cannot use !NONE (works only with flags) */
> > +    case NONE:
> > +        /* due to the way cases are defined after NONE we must have
> > something */
> > +        /* due to a bug we cannot use @virtual to write 0 to
> > compressed_size */
> > +        EmptyStructure empty;
> > +    default:
> > +        uint32 uncompressed_size;
> 
> Hi Frediano,
> 
> Is it better if we replace the order, meaning
> do not case NONE but case  LZ4
> 
>          case LZ4:
>              uint32 uncompressed_size;
> 
> 

However with this for every new possible compression you have to add a case.
In case of just one compression yes, will work. Doing that way you support
any compression. Probably the easier way is to just use


message CompressedData {
    data_compression_type type;
    uint32 uncompressed_size;
    uint8 compressed_data[] @as_ptr(compressed_size);
};

and if people are concerned with additional uncompressed_size
just don't use CompressedData for uncompressed data.

> 
> 
> I find it confusing that compressed_size is not a part
> of this definition but is used here.
> (defined in common/messages.h above)
> 

See @as_ptr usage, there are quite a lot in the protocol and reduce in
this case message size. The @as_ptr(field_name) uses field_name in the
structure. Reduces also memory copy as a pointer to message data
is stored.

> Thanks,
>      Uri.
> 
> 

Frediano