| Message ID | 20230502135548.2451309-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ui: Fix pixel colour channel order for PNG screenshots | expand |
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> > --- > Disclaimer: I don't have a BE system that I have convenient > graphics output from that I can use to test the screenshot > format there. > --- > ui/console.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ui/console.c b/ui/console.c > index 6e8a3cdc62d..e173731e205 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -311,7 +311,7 @@ static bool png_save(int fd, pixman_image_t *image, > Error **errp) > png_struct *png_ptr; > png_info *info_ptr; > g_autoptr(pixman_image_t) linebuf = > - qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, > width); > + qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width); > uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf); > FILE *f = fdopen(fd, "wb"); > int y; > @@ -341,7 +341,7 @@ static bool png_save(int fd, pixman_image_t *image, > Error **errp) > png_init_io(png_ptr, f); > > png_set_IHDR(png_ptr, info_ptr, width, height, 8, > - PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, > + PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE, > PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); > > png_write_info(png_ptr, info_ptr); > -- > 2.34.1 > >
On Tue, 2 May 2023, Peter Maydell 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, Typo: should be PIXMAN_a8r8g8b8 not a9 Regards, BALATON Zoltan > 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> > --- > Disclaimer: I don't have a BE system that I have convenient > graphics output from that I can use to test the screenshot > format there. > --- > ui/console.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ui/console.c b/ui/console.c > index 6e8a3cdc62d..e173731e205 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -311,7 +311,7 @@ static bool png_save(int fd, pixman_image_t *image, Error **errp) > png_struct *png_ptr; > png_info *info_ptr; > g_autoptr(pixman_image_t) linebuf = > - qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, width); > + qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width); > uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf); > FILE *f = fdopen(fd, "wb"); > int y; > @@ -341,7 +341,7 @@ static bool png_save(int fd, pixman_image_t *image, Error **errp) > png_init_io(png_ptr, f); > > png_set_IHDR(png_ptr, info_ptr, width, height, 8, > - PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, > + PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE, > PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); > > png_write_info(png_ptr, info_ptr); >
On Tue, 2 May 2023 at 21:36, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Tue, 2 May 2023, Peter Maydell 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, > > Typo: should be PIXMAN_a8r8g8b8 not a9 Thanks -- I super-carefully proof-read that to make sure I'd got the a r g and b in the right order and completely missed the 8 vs 9 :-) -- PMM
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
Hi Peter On Tue, May 9, 2023 at 5:12 PM Peter Maydell <peter.maydell@linaro.org> wrote: > 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) ? > > feel free to take it, thanks
diff --git a/ui/console.c b/ui/console.c index 6e8a3cdc62d..e173731e205 100644 --- a/ui/console.c +++ b/ui/console.c @@ -311,7 +311,7 @@ static bool png_save(int fd, pixman_image_t *image, Error **errp) png_struct *png_ptr; png_info *info_ptr; g_autoptr(pixman_image_t) linebuf = - qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, width); + qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width); uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf); FILE *f = fdopen(fd, "wb"); int y; @@ -341,7 +341,7 @@ static bool png_save(int fd, pixman_image_t *image, Error **errp) png_init_io(png_ptr, f); png_set_IHDR(png_ptr, info_ptr, width, height, 8, - PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, + PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); png_write_info(png_ptr, info_ptr);
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> --- Disclaimer: I don't have a BE system that I have convenient graphics output from that I can use to test the screenshot format there. --- ui/console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)