Re: [PATCH] ui: Fix pixel colour channel order for PNG screenshots

[Peter Maydell]

On Tue, 2 May 2023 at 20:36, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
>
>
> On Tue, May 2, 2023 at 5:56 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> When we take a PNG screenshot the ordering of the colour channels in
>> the data is not correct, resulting in the image having weird
>> colouring compared to the actual display.  (Specifically, on a
>> little-endian host the blue and red channels are swapped; on
>> big-endian everything is wrong.)
>>
>> This happens because the pixman idea of the pixel data and the libpng
>> idea differ.  PIXMAN_a9r8g8b8 defines that pixels are 32-bit values,
>> with A in bits 24-31, R in bits 16-23, G in bits 8-15 and B in bits
>> 0-7.  This means that on little-endian systems the bytes in memory
>> are
>>    B G R A
>> and on big-endian systems they are
>>    A R G B
>>
>> libpng, on the other hand, thinks of pixels as being a series of
>> values for each channel, so its format PNG_COLOR_TYPE_RGB_ALPHA
>> always wants bytes in the order
>>    R G B A
>>
>> This isn't the same as the pixman order for either big or little
>> endian hosts.
>>
>> The alpha channel is also unnecessary bulk in the output PNG file,
>> because there is no alpha information in a screenshot.
>>
>> To handle the endianness issue, we already define in ui/qemu-pixman.h
>> various PIXMAN_BE_* and PIXMAN_LE_* values that give consistent
>> byte-order pixel channel formats.  So we can use PIXMAN_BE_r8g8b8 and
>> PNG_COLOR_TYPE_RGB, which both have an in-memory byte order of
>>     R G B
>> and 3 bytes per pixel.
>>
>> (PPM format screenshots get this right; they already use the
>> PIXMAN_BE_r8g8b8 format.)
>>
>> Cc: qemu-stable@nongnu.org
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1622
>> Fixes: 9a0a119a382867 ("Added parameter to take screenshot with screendump 
>> as PNG")
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks; shall I take this via target-arm.next, or would you
prefer to take it via the UI queue (with the commit message
typo fixed) ?

-- PMM