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

[Christophe Fergeau]

On Mon, Jan 26, 2015 at 03:20:23PM +0100, Javier Celaya wrote:
> 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.

Ah, or you can just improve the comment "Fix row alignement which may be
wrong (not 32 bit aligned) after uncompressing 16bpp/24bpp data"
or something like that.

Christophe

> 
> > 
> > > +        // 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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150126/8168c24d/attachment.sig>