----- Original Message -----
> >
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > ---
> > 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;
> > };
>
> Note that adding fields before buf could make buf not aligned to 4 bytes
> while we want it aligned to 4 bytes. Quic code for portability require it.
> On GCC we could force the alignment with an attribute.
> Another portable way to make it aligned would be
Note g_slice_alloc() is aligned by default.
>
> union {
> uint8_t bytes[RED_COMPRESS_BUF_SIZE];
> uint32_t words[RED_COMPRESS_BUF_SIZE / 4];
> } buf;
>
looks good to me too.
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index cf72100..83e0fa0 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -3888,7 +3888,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)
> > {
>
> The declaration suggests io_ptr can be one byte aligned but this is false.
> A comment should be enough.
>
> > RedCompressBuf *buf;
> >
> > @@ -3899,31 +3899,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);
> > }
> >
>
> to remove just the shift here we could use G_N_ELEMENTS macro.
Why not
>
> > 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
> > @@ -3937,7 +3937,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)
> > @@ -4591,8 +4591,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));
> >
>
> Here G_N_ELEMENTS could be used. With the union it would became
>
> size = quic_encode(quic, type, src->x, src->y, NULL, 0, stride,
> quic_data->data.bufs_head->buf.words,
> G_N_ELEMENTS(quic_data->data.bufs_head->buf.words);
>
>
> > // the compressed buffer is bigger than the original data
> > if ((size << 2) > (src->y * src->stride)) {
> > --
> > 2.4.3
> >
>
> Beside the alignment I think other stuff are just style so very
> personal opinions.
>
> I won't suggest some definitive code, let's see other comments.
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel