[Spice-devel] [PATCH 5/6] LZ4: Fix the row alignment when it is not on a 32bit boundary

[Javier Celaya]

Hello,

El Lunes, 26 de enero de 2015 15:13:57 Christophe Fergeau escribió:
> Hey,
> 
> On Thu, Jan 22, 2015 at 05:21:23PM +0100, Javier Celaya wrote:
> > ---
> > 
> >  common/canvas_base.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/canvas_base.c b/common/canvas_base.c
> > index c45d535..170def1 100644
> > --- a/common/canvas_base.c
> > +++ b/common/canvas_base.c
> > @@ -563,8 +563,7 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >  {
> >  
> >      pixman_image_t *surface = NULL;
> >      int dec_size, enc_size, available;
> > 
> > -    int stride;
> > -    int stride_abs;
> > +    int stride, stride_abs, stride_encoded;
> > 
> >      uint8_t *dest, *data, *data_end;
> >      int width, height, top_down;
> >      LZ4_streamDecode_t *stream;
> > 
> > @@ -575,19 +574,23 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >      data = image->u.lz4.data->chunk[0].data;
> >      data_end = data + image->u.lz4.data->chunk[0].len;
> >      width = image->descriptor.width;
> > 
> > +    stride_encoded = width;
> > 
> >      height = image->descriptor.height;
> >      top_down = *(data++);
> >      spice_format = *(data++);
> >      switch (spice_format) {
> >      
> >          case SPICE_BITMAP_FMT_16BIT:
> >              format = PIXMAN_x1r5g5b5;
> > 
> > +            stride_encoded *= 2;
> > 
> >              break;
> >          
> >          case SPICE_BITMAP_FMT_24BIT:
> >          
> >          case SPICE_BITMAP_FMT_32BIT:
> >              format = PIXMAN_x8r8g8b8;
> > 
> > +            stride_encoded *= 4;
> > 
> >              break;
> >          
> >          case SPICE_BITMAP_FMT_RGBA:
> >              format = PIXMAN_a8r8g8b8;
> > 
> > +            stride_encoded *= 4;
> > 
> >              break;
> >          
> >          default:
> >              spice_warning("Unsupported bitmap format %d with LZ4\n",
> >              spice_format);
> > 
> > @@ -631,6 +634,24 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >          data += enc_size;
> >      
> >      } while (data < data_end);
> > 
> > +    if (stride_abs > stride_encoded) {
> 
> My understanding is that this will only trigger in the 24bpp case when
> the next commit adds
>           case SPICE_BITMAP_FMT_24BIT:
>               stride_encoded *= 3;
> ?
> If so, it should be mentioned in the commit log that this only add some
> helper code which will be used when 24bpp support is added or something
> like this.

It also triggers with 16bpp when width is an odd number, so the commit makes 
sense by itself. But I can say something about the "future" support to 24bpp.

> 
> > +        // Fix the row alignment
> > +        int row, col;
> > +        uint32_t *dest_aligned, *dest_misaligned;
> > +        dest = (uint8_t *)pixman_image_get_data(surface);
> > +        if (!top_down) {
> > +            dest -= (stride_abs * (height - 1));
> > +        }
> > +        stride_abs /= 4;
> > +        dest_aligned = (uint32_t *)dest + stride_abs*height - 1;
> > +        for (row = height - 1; row > 0; --row) {
> > +            dest_misaligned = (uint32_t*)(dest + stride_encoded*row) +
> > stride_abs - 1; +            for (col = 0; col < stride_abs; ++col) {
> > +                *dest_aligned-- = *dest_misaligned--;
> > +            }
> 
> Couldn't it be handled with a bunch of memmove?
> for (row = height - 1; row > 0; --row) {
>     dest_aligned = (uint32_t *)(dest + stride_abs*row)
>     dest_misaligned = (uint32_t*)(dest + stride_encoded*row);
>     memmove(dest_aligned, dest_misaligned, stride_encoded*sizeof(uint32_t));
> /* set the padding bytes to 0 if needed */
> }

Sure, I was thinking in terms of memcpy (which wouldn't work). I'll change it.

> 
> Christophe