[Spice-devel] [PATCH 14/16] worker: don't use weird RedCompressedBuf nbytes shifting

[Jonathon Jongsma]

On Wed, 2015-11-11 at 06:47 -0500, Frediano Ziglio wrote:
> > 
> > Hmm, this code was fairly weird. It's still a bit weird that the
> > 'more_space'
> > vfunc for the quic encoder has different semantics than all of the rest of
> > them
> > (returning the number of uint32_t elements allocated instead of the number
> > of
> > bytes allocated). But the code is slightly less confusing now. ACK
> > 
> > 
> 
> Did you see 
> http://lists.freedesktop.org/archives/spice-devel/2015-November/023358.html?


Ah, I missed this one. Sorry.

> 
> On the same subject... is it ok to post again same patch if there are comments
> or is it confusing?


Yeah, it is a little confusing to repost the same patch when there are pending
comments. It would be slightly easier to follow if the reposted patch contained
an annotation that referenced the earlier discussion, perhaps, but that means
more work for the person posting patch series (which is you, currently).
Alternatively, I can just try to pay closer attention :)

> 
> I personally agree encoder_usr_more_space should return bytes and not uint32_t
> elements however I prefer to have uint32_t in the structure.

Yeah, now that I look at it a bit more closely, I think that the union approach
is not so bad.

> 
> Frediano
> 
> > 
> > 
> > On Tue, 2015-11-10 at 14:16 +0000, Frediano Ziglio wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > 
> > > ---
> > >  server/display-channel.h |  3 +--
> > >  server/red_worker.c      | 18 +++++++++---------
> > >  2 files changed, 10 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 7c62a62..f1f4e3a 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -70,10 +70,9 @@
> > >  #define NUM_STREAMS 50
> > >  #define NUM_SURFACES 10000
> > >  
> > > -#define RED_COMPRESS_BUF_SIZE (1024 * 64)
> > >  typedef struct RedCompressBuf RedCompressBuf;
> > >  struct RedCompressBuf {
> > > -    uint32_t buf[RED_COMPRESS_BUF_SIZE / 4];
> > > +    uint8_t buf[64 * 1024];
> > >      RedCompressBuf *next;
> > >      RedCompressBuf *send_next;
> > >  };
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 1b8951f..5daca87 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -3886,7 +3886,7 @@ static void glz_usr_free(GlzEncoderUsrContext *usr,
> > > void
> > > *ptr)
> > >      free(ptr);
> > >  }
> > >  
> > > -static inline int encoder_usr_more_space(EncoderData *enc_data, uint32_t
> > > **io_ptr)
> > > +static int encoder_usr_more_space(EncoderData *enc_data, uint8_t
> > > **io_ptr)
> > >  {
> > >      RedCompressBuf *buf;
> > >  
> > > @@ -3897,31 +3897,31 @@ static inline int
> > > encoder_usr_more_space(EncoderData
> > > *enc_data, uint32_t **io_pt
> > >      enc_data->bufs_tail = buf;
> > >      buf->send_next = NULL;
> > >      *io_ptr = buf->buf;
> > > -    return sizeof(buf->buf) >> 2;
> > > +    return sizeof(buf->buf);
> > >  }
> > >  
> > >  static int quic_usr_more_space(QuicUsrContext *usr, uint32_t **io_ptr,
> > > int
> > > rows_completed)
> > >  {
> > >      EncoderData *usr_data = &(((QuicData *)usr)->data);
> > > -    return encoder_usr_more_space(usr_data, io_ptr);
> > > +    return encoder_usr_more_space(usr_data, (uint8_t **)io_ptr) /
> > > sizeof(uint32_t);
> > >  }
> > >  
> > >  static int lz_usr_more_space(LzUsrContext *usr, uint8_t **io_ptr)
> > >  {
> > >      EncoderData *usr_data = &(((LzData *)usr)->data);
> > > -    return (encoder_usr_more_space(usr_data, (uint32_t **)io_ptr) << 2);
> > > +    return encoder_usr_more_space(usr_data, io_ptr);
> > >  }
> > >  
> > >  static int glz_usr_more_space(GlzEncoderUsrContext *usr, uint8_t
> > > **io_ptr)
> > >  {
> > >      EncoderData *usr_data = &(((GlzData *)usr)->data);
> > > -    return (encoder_usr_more_space(usr_data, (uint32_t **)io_ptr) << 2);
> > > +    return encoder_usr_more_space(usr_data, io_ptr);
> > >  }
> > >  
> > >  static int jpeg_usr_more_space(JpegEncoderUsrContext *usr, uint8_t
> > >  **io_ptr)
> > >  {
> > >      EncoderData *usr_data = &(((JpegData *)usr)->data);
> > > -    return (encoder_usr_more_space(usr_data, (uint32_t **)io_ptr) << 2);
> > > +    return encoder_usr_more_space(usr_data, io_ptr);
> > >  }
> > >  
> > >  #ifdef USE_LZ4
> > > @@ -3935,7 +3935,7 @@ static int lz4_usr_more_space(Lz4EncoderUsrContext
> > > *usr,
> > > uint8_t **io_ptr)
> > >  static int zlib_usr_more_space(ZlibEncoderUsrContext *usr, uint8_t
> > >  **io_ptr)
> > >  {
> > >      EncoderData *usr_data = &(((ZlibData *)usr)->data);
> > > -    return (encoder_usr_more_space(usr_data, (uint32_t **)io_ptr) << 2);
> > > +    return encoder_usr_more_space(usr_data, io_ptr);
> > >  }
> > >  
> > >  static inline int encoder_usr_more_lines(EncoderData *enc_data, uint8_t
> > > **lines)
> > > @@ -4589,8 +4589,8 @@ static inline int
> > > red_quic_compress_image(DisplayChannelClient *dcc, SpiceImage
> > >          stride = -src->stride;
> > >      }
> > >      size = quic_encode(quic, type, src->x, src->y, NULL, 0, stride,
> > > -                       quic_data->data.bufs_head->buf,
> > > -                       sizeof(quic_data->data.bufs_head->buf) >> 2);
> > > +                       (uint32_t*)quic_data->data.bufs_head->buf,
> > > +                       sizeof(quic_data->data.bufs_head->buf) /
> > > sizeof(uint32_t));
> > >  
> > >      // the compressed buffer is bigger than the original data
> > >      if ((size << 2) > (src->y * src->stride)) {
> >