[Spice-devel] [PATCH 1/3] Add support for LZ compression of A8 images

[Søren Sandmann]

Hans de Goede <hdegoede at redhat.com> writes:

>> @@ -5882,7 +5888,8 @@ static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
>>       LZ_IMAGE_TYPE_RGB16,
>>       LZ_IMAGE_TYPE_RGB24,
>>       LZ_IMAGE_TYPE_RGB32,
>> -    LZ_IMAGE_TYPE_RGBA
>> +    LZ_IMAGE_TYPE_RGBA,
>> +    LZ_IMAGE_TYPE_A8
>>   };
>>
>>   typedef struct compress_send_data_t {
>
> Here you are using A8 again, versus 8BIT_A in some other patches, please pick one and
> be consistent about it. Other then that this and the other 2 patches
> look good.

Note that there are two enums here, and the I chose the names to be
consistent with the existing entries. One is this:

    SPICE_BITMAP_FMT_INVALID,
    SPICE_BITMAP_FMT_1BIT_LE,
    SPICE_BITMAP_FMT_1BIT_BE,
    SPICE_BITMAP_FMT_4BIT_LE,
    SPICE_BITMAP_FMT_4BIT_BE,
    SPICE_BITMAP_FMT_8BIT,
    SPICE_BITMAP_FMT_16BIT,
    SPICE_BITMAP_FMT_24BIT,
    SPICE_BITMAP_FMT_32BIT,
    SPICE_BITMAP_FMT_RGBA,
    SPICE_BITMAP_FMT_8BIT_A,

where I chose to use 8BIT_A to be consistent with the rest of the
entries (but distinct from 8BIT). The other is this:

    LZ_IMAGE_TYPE_INVALID,
    LZ_IMAGE_TYPE_PLT1_LE,
    LZ_IMAGE_TYPE_PLT1_BE,      // PLT stands for palette
    LZ_IMAGE_TYPE_PLT4_LE,
    LZ_IMAGE_TYPE_PLT4_BE,
    LZ_IMAGE_TYPE_PLT8,
    LZ_IMAGE_TYPE_RGB16,
    LZ_IMAGE_TYPE_RGB24,
    LZ_IMAGE_TYPE_RGB32,
    LZ_IMAGE_TYPE_RGBA,
    LZ_IMAGE_TYPE_XXXA,
    LZ_IMAGE_TYPE_A8

where A8 seemed most consistent. 

You're right that for the BITMAP_FMT enum, the first patch had A8 and
the second changed it to 8BIT_A. I have fixed that in the following
patch series along with your other comments.


Thanks for the review,
Søren