[Spice-devel] [PATCH 2/2] Fix LZ4 supported image formats.

[Javier Celaya]

Hello,

El Miércoles, 21 de enero de 2015 17:27:01 Christophe Fergeau escribió:
> Hey,
> 
> On Thu, Jan 15, 2015 at 12:50:45PM +0100, Javier Celaya wrote:
> > - Read the original image format from the LZ4 stream. Only RGB formats
> > 
> >   are supported.
> > 
> > - Expand 24bit to 32bit image format.
> 
> This one would also have deserved to be split in several commits.
> 
> > ---
> > 
> >  common/canvas_base.c | 48
> >  +++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/common/canvas_base.c b/common/canvas_base.c
> > index bb7a424..d1a714c 100644
> > --- a/common/canvas_base.c
> > +++ b/common/canvas_base.c
> > @@ -554,22 +554,42 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >      int stride;
> >      int stride_abs;
> >      uint8_t *dest, *data, *data_end;
> > 
> > -    int width, height, direction;
> > +    int width, height, top_down;
> > 
> >      LZ4_streamDecode_t *stream;
> > 
> > +    uint8_t spice_format;
> > +    pixman_format_code_t format;
> > 
> > +    uint32_t * dest32, xrgb;
> > +    int i32, i24;
> 
> These are only used in the '// Expand 24 to 32 bits' block so they can
> be declared down there.
> 
> >      spice_chunks_linearize(image->u.lz4.data);
> >      data = image->u.lz4.data->chunk[0].data;
> >      data_end = data + image->u.lz4.data->chunk[0].len;
> >      width = image->descriptor.width;
> >      height = image->descriptor.height;
> > 
> > -    direction = *(data++);
> > +    top_down = *(data++);
> > +    spice_format = *(data++);
> > +    switch (spice_format) {
> > +        case SPICE_BITMAP_FMT_16BIT:
> > +            format =  PIXMAN_x1r5g5b5;
> > +            break;
> > +        case SPICE_BITMAP_FMT_24BIT:
> > +        case SPICE_BITMAP_FMT_32BIT:
> > +            format =  PIXMAN_x8r8g8b8;
> > +            break;
> > +        case SPICE_BITMAP_FMT_RGBA:
> > +            format =  PIXMAN_a8r8g8b8;
> > +            break;
> > +        default:
> > +            spice_error("Unsupported bitmap format %d with LZ4\n",
> > spice_format); +            return NULL;
> > +    }
> > 
> >      surface = surface_create(
> >  
> >  #ifdef WIN32
> >  
> >                               canvas->dc,
> >  
> >  #endif
> > 
> > -                             PIXMAN_a8r8g8b8,
> > -                             width, height, direction == 0);
> > +                             format,
> > +                             width, height, top_down);
> > 
> >      if (surface == NULL) {
> >      
> >          spice_warning("create surface failed");
> >          return NULL;
> > 
> > @@ -580,7 +600,7 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >      stride = pixman_image_get_stride(surface);
> >      stride_abs = abs(stride);
> >      available = height * stride_abs;
> > 
> > -    if (direction == 1) {
> > +    if (!top_down) {
> > 
> >          dest -= (stride_abs * (height - 1));
> >      
> >      }
> > 
> > @@ -602,6 +622,24 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> > *canvas, SpiceImage *image, int> 
> >      } while (data < data_end);
> >      
> >      LZ4_freeStreamDecode(stream);
> > 
> > +
> > +    if (spice_format == SPICE_BITMAP_FMT_24BIT) {
> > +        // Expand 24 to 32 bits
> > +        dest = (uint8_t *)pixman_image_get_data(surface);
> > +        if (!top_down) {
> > +            dest -= (stride_abs * (height - 1));
> > +        }
> > +        dest32 = (uint32_t *)dest;
> > +        i32 = height * width;
> > +        i24 = (height * stride_abs * 3) / 4;
> > +        while (i32 > 0) {
> > +            xrgb = dest[--i24] << 16;  // R
> > +            xrgb += dest[--i24] << 8;  // G
> > +            xrgb += dest[--i24];       // B
> > +            dest32[--i32] = xrgb;
> > +        }
> > +    }
> > +
> 
> Do you know why this is not needed for the other compression methods (or
> maybe it's hidden in the autogenerated template code)?

LZ does expand 24 to 32 bits on decode. Pixman supports 24 bit images, but for 
some reason LZ always outputs 32bit images. In particular, the function 
surface_create in spice-common/common/canvas_utils.c has a comment that says:

// NOTE: we assume here that the lz decoders always decode to RGB32.

Then, a switch on the image format only allows certain formats. Adding RGB24 
is not as simple, though, because it seems pixman expects lines to be aligned 
to 32bit. I'll investigate further and send another set of patches.

> 
> 
> Christophe