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

[Jonathon Jongsma]

On Wed, 2015-11-11 at 12:17 -0500, Frediano Ziglio wrote:
> > 
> > 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.htm
> > > l?
> > 
> > 
> > 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.
> > 
> 
> I'll try to came up with a proposed patch tomorrow
> 
> Frediano


I just noticed one more thing missing from this patch. All of the other
compression-format-specific functions were updated, but lz4_usr_more_space() was
not. So if lz4 is enabled, there is a build error. We just need to remove the
cast like we do in the other functions.



> 
> > > > 
> > > > 
> > > > 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)) {
> > > > 
> >