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

[Frediano Ziglio]

> 
> On Thu, Nov 12, 2015 at 3:01 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > ---
> >  server/display-channel.h |  3 +--
> >  server/red_worker.c      | 20 ++++++++++----------
> >  2 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 12ef60a..599cce7 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 af85591..82d39a9 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -3758,7 +3758,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;
> >
> > @@ -3769,45 +3769,45 @@ 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
> >  static int lz4_usr_more_space(Lz4EncoderUsrContext *usr, uint8_t **io_ptr)
> >  {
> >      EncoderData *usr_data = &(((Lz4Data *)usr)->data);
> > -    return (encoder_usr_more_space(usr_data, (uint32_t **)io_ptr) << 2);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> >  }
> >  #endif
> >
> >  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)
> > @@ -4461,8 +4461,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)) {
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> I thought you would come up with the patch already using a union, as
> discussed here:
> http://lists.freedesktop.org/archives/spice-devel/2015-November/023494.html
> and
> http://lists.freedesktop.org/archives/spice-devel/2015-November/023488.html
> Also, this Jonathon's suggestions should also be considered:
> http://lists.freedesktop.org/archives/spice-devel/2015-November/023505.html
> 

Me too but this morning I had to merge some changes for statistics from
Jonathon (we didn't check with statistics enabled and this feature was not
even compiling), the regression Pavel and you discovered.

So the patches (even some already posted) changed quite a lot and I didn't find
time to prepare it. I'll post it, in the meantime I don't stop other patches.

Frediano