[Spice-devel] [PATCH] fix 16 bpp LZ image decompression

[Pavel Grunt]

Hi,

On Thu, 2016-04-14 at 17:56 +0100, Frediano Ziglio wrote:
> LZ image decompression was broken for 16 bpp:
> - stride was computed not computed correctly (as width*4). This
> caused
>   also a buffer underflow;
> - stride in pixman is always multiple of 4 bytes (so for 16 bpp is
>   ALIGN(width*2, 4)) so image decompressed by lz_decode as some
> missing
>   bytes to be fixed.
> 

It looks good. I wonder if the code has ever worked for lz 16bpp.

> The alignment code is reused from LZ4 function.
> 
> This fix also https://bugzilla.redhat.com/show_bug.cgi?id=1285469.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Pavel Grunt <pgrunt at redhat.com>

Thanks,
Pavel

> ---
>  common/canvas_base.c | 54 +++++++++++++++++++++++++++++++-----------
> ----------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/common/canvas_base.c b/common/canvas_base.c
> index fa4d373..45dd75f 100644
> --- a/common/canvas_base.c
> +++ b/common/canvas_base.c
> @@ -515,13 +515,30 @@ static pixman_image_t
> *canvas_get_jpeg(CanvasBase *canvas, SpiceImage *image)
>      return surface;
>  }
>  
> +static void canvas_fix_alignment(uint8_t *bits,
> +                                 int stride_encoded, int
> stride_pixman,
> +                                 int height)
> +{
> +    if (stride_pixman > stride_encoded) {
> +        // Fix the row alignment
> +        int row;
> +        uint8_t *dest = bits;
> +        for (row = height - 1; row > 0; --row) {
> +            uint32_t *dest_aligned, *dest_misaligned;
> +            dest_aligned = (uint32_t *)(dest + stride_pixman*row);
> +            dest_misaligned = (uint32_t*)(dest +
> stride_encoded*row);
> +            memmove(dest_aligned, dest_misaligned, stride_encoded);
> +        }
> +    }
> +}
> +
>  #ifdef USE_LZ4
>  static pixman_image_t *canvas_get_lz4(CanvasBase *canvas, SpiceImage
> *image)
>  {
>      pixman_image_t *surface = NULL;
>      int dec_size, enc_size, available;
>      int stride, stride_abs, stride_encoded;
> -    uint8_t *dest, *data, *data_end;
> +    uint8_t *dest, *data, *data_end, *bits;
>      int width, height, top_down;
>      LZ4_streamDecode_t *stream;
>      uint8_t spice_format;
> @@ -576,6 +593,7 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> *canvas, SpiceImage *image)
>      if (!top_down) {
>          dest -= (stride_abs * (height - 1));
>      }
> +    bits = dest;
>  
>      do {
>          // Read next compressed block
> @@ -594,20 +612,7 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> *canvas, SpiceImage *image)
>          data += enc_size;
>      } while (data < data_end);
>  
> -    if (stride_abs > stride_encoded) {
> -        // Fix the row alignment
> -        int row;
> -        dest = (uint8_t *)pixman_image_get_data(surface);
> -        if (!top_down) {
> -            dest -= (stride_abs * (height - 1));
> -        }
> -        for (row = height - 1; row > 0; --row) {
> -            uint32_t *dest_aligned, *dest_misaligned;
> -            dest_aligned = (uint32_t *)(dest + stride_abs*row);
> -            dest_misaligned = (uint32_t*)(dest +
> stride_encoded*row);
> -            memmove(dest_aligned, dest_misaligned, stride_encoded);
> -        }
> -    }
> +    canvas_fix_alignment(bits, stride_encoded, stride_abs, height);
>  
>      LZ4_freeStreamDecode(stream);
>      return surface;
> @@ -782,7 +787,6 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>      uint8_t *comp_buf = NULL;
>      int comp_size;
>      uint8_t    *decomp_buf = NULL;
> -    uint8_t    *src;
>      pixman_format_code_t pixman_format;
>      LzImageType type, as_type;
>      SpicePalette *palette;
> @@ -790,6 +794,7 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>      int width;
>      int height;
>      int top_down;
> +    int stride_encoded;
>      int stride;
>      int free_palette;
>  
> @@ -818,10 +823,12 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>      lz_decode_begin(lz_data->lz, comp_buf, comp_size, &type,
>                      &width, &height, &n_comp_pixels, &top_down,
> palette);
>  
> +    stride_encoded = width;
>      switch (type) {
>      case LZ_IMAGE_TYPE_RGBA:
>          as_type = LZ_IMAGE_TYPE_RGBA;
>          pixman_format = PIXMAN_LE_a8r8g8b8;
> +        stride_encoded *= 4;
>          break;
>      case LZ_IMAGE_TYPE_RGB32:
>      case LZ_IMAGE_TYPE_RGB24:
> @@ -832,6 +839,7 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>      case LZ_IMAGE_TYPE_PLT8:
>          as_type = LZ_IMAGE_TYPE_RGB32;
>          pixman_format = PIXMAN_LE_x8r8g8b8;
> +        stride_encoded *= 4;
>          break;
>      case LZ_IMAGE_TYPE_A8:
>          as_type = LZ_IMAGE_TYPE_A8;
> @@ -843,9 +851,11 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>               canvas->format == SPICE_SURFACE_FMT_32_ARGB)) {
>              as_type = LZ_IMAGE_TYPE_RGB32;
>              pixman_format = PIXMAN_LE_x8r8g8b8;
> +            stride_encoded *= 4;
>          } else {
>              as_type = LZ_IMAGE_TYPE_RGB16;
>              pixman_format = PIXMAN_x1r5g5b5;
> +            stride_encoded *= 2;
>          }
>          break;
>      default:
> @@ -865,18 +875,18 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> *canvas, SpiceImage *image,
>      alloc_lz_image_surface(&lz_data->decode_data, pixman_format,
>                             width, height, n_comp_pixels, top_down);
>  
> -    src = (uint8_t *)pixman_image_get_data(lz_data-
> >decode_data.out_surface);
> +    stride = pixman_image_get_stride(lz_data-
> >decode_data.out_surface);
> +    stride = abs(stride);
>  
> -    stride = (n_comp_pixels / height) * 4;
> +    decomp_buf = (uint8_t *)pixman_image_get_data(lz_data-
> >decode_data.out_surface);
>      if (!top_down) {
> -        stride = -stride;
> -        decomp_buf = src + stride * (height - 1);
> -    } else {
> -        decomp_buf = src;
> +        decomp_buf -= stride * (height - 1);
>      }
>  
>      lz_decode(lz_data->lz, as_type, decomp_buf);
>  
> +    canvas_fix_alignment(decomp_buf, stride_encoded, stride,
> height);
> +
>      if (free_palette)  {
>          free(palette);
>      }