diff mbox series

[FFmpeg-devel] avdevice/xcbgrab: don't assume Xserver endianness

Message ID 20210131194147.30246-1-andriy.gelman@gmail.com
State Accepted
Commit 8e0a4d2d4d1cc56a12b3c231e444fd2e94f32ce0
Headers show
Series [FFmpeg-devel] avdevice/xcbgrab: don't assume Xserver endianness | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andriy Gelman Jan. 31, 2021, 7:41 p.m. UTC 
From: Andriy Gelman <andriy.gelman@gmail.com>

Xserver defines the endianness of the grabbed images. Use this information
to set the correct pixel format.

This also fixes format selection in configuration depth=32/bpp=32 with
Xserver on a little endian machine. Before the patch, the big endian
layout 0RGB was always selected which is incorrect because BGR0 should
be used. RGB24 was also incorrectly assumed (but this format was removed
in Xserver 1.20).

The big-endian settings can be tested using docker+qemu from a little-endian
machine:

$ docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
$ docker run --rm -it --privileged  -v /tmp:/tmp -v $(pwd):/ffmpeg powerpc64/debian /bin/bash

In docker container
$ apt-get update
$ apt-get install xvfb
$ apt-get install x11-apps

To test AV_PIX_FMT_0RGB32
$ Xvfb :2 -screen 0 1600x1200x24 &
$ export DISPLAY=:2
$ xclock -geometry 1600x1200 -bg green #test different colors

On your host machine grab the frames using the following
command. View output to check that colors are rendered correctly
$ ./ffmpeg -y -f x11grab -i :2.0 -codec:v mpeg2video out.mp4

Other pixel formats can be tested by modifying how Xvfb is started in the docker
container:

AV_PIX_FMT_RGB565
$ Xvfb :2 -screen 0 1600x1200x16

AV_PIX_FMT_RGB555
$ Xvfb :2 -screen 0 1600x1200x15

AV_PIX_FMT_BGR24 / AV_PIX_FMT_RGB24
This is difficult to test because bpp=24 support was removed in Xserver 1.20
https://lists.x.org/archives/xorg-devel/2018-February/056175.html?hmsr=joyk.com&utm_source=joyk.com&utm_medium=referral
However, I was able to run previous version of Xvfb (with some
modifications to force 24bpp) to check that images are rendered correctly.

Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
---
 libavdevice/xcbgrab.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Carl Eugen Hoyos Jan. 31, 2021, 10:28 p.m. UTC | #1 
Am So., 31. Jan. 2021 um 20:51 Uhr schrieb Andriy Gelman
<andriy.gelman@gmail.com>:
>
> From: Andriy Gelman <andriy.gelman@gmail.com>
>
> Xserver defines the endianness of the grabbed images. Use this information
> to set the correct pixel format.

lgtm if tested.

Could you look at pal8? This used to work fine with x11grab...

Thank you, Carl Eugen
Andriy Gelman Feb. 2, 2021, 3:16 a.m. UTC | #2 
On Sun, 31. Jan 23:28, Carl Eugen Hoyos wrote:
> Am So., 31. Jan. 2021 um 20:51 Uhr schrieb Andriy Gelman
> <andriy.gelman@gmail.com>:
> >
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> >
> > Xserver defines the endianness of the grabbed images. Use this information
> > to set the correct pixel format.
> 
> lgtm if tested.

Thanks, will apply in a few days.
Do you think I should keep the docker testing instructions in the commit
message?

> 
> Could you look at pal8? This used to work fine with x11grab...

Yes sure, will have a look later this week.
Carl Eugen Hoyos Feb. 2, 2021, 9:32 a.m. UTC | #3 
Am Di., 2. Feb. 2021 um 04:26 Uhr schrieb Andriy Gelman
<andriy.gelman@gmail.com>:
>
> On Sun, 31. Jan 23:28, Carl Eugen Hoyos wrote:
> > Am So., 31. Jan. 2021 um 20:51 Uhr schrieb Andriy Gelman
> > <andriy.gelman@gmail.com>:
> > >
> > > From: Andriy Gelman <andriy.gelman@gmail.com>
> > >
> > > Xserver defines the endianness of the grabbed images. Use this information
> > > to set the correct pixel format.
> >
> > lgtm if tested.
>
> Thanks, will apply in a few days.
> Do you think I should keep the docker testing instructions in the commit
> message?

If you believe that testing is not trivial (I agree), then this sounds
like a good idea.

> > Could you look at pal8? This used to work fine with x11grab...
>
> Yes sure, will have a look later this week.

Thank you, Carl Eugen
diff mbox series

Patch

diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
index 95bdc8ab9d..be5d5ea2cf 100644
--- a/libavdevice/xcbgrab.c
+++ b/libavdevice/xcbgrab.c
@@ -513,21 +513,26 @@  static int pixfmt_from_pixmap_format(AVFormatContext *s, int depth,
             switch (depth) {
             case 32:
                 if (fmt->bits_per_pixel == 32)
-                    *pix_fmt = AV_PIX_FMT_0RGB;
+                    *pix_fmt = setup->image_byte_order == XCB_IMAGE_ORDER_LSB_FIRST ?
+                               AV_PIX_FMT_BGR0 : AV_PIX_FMT_0RGB;
                 break;
             case 24:
                 if (fmt->bits_per_pixel == 32)
-                    *pix_fmt = AV_PIX_FMT_0RGB32;
+                    *pix_fmt = setup->image_byte_order == XCB_IMAGE_ORDER_LSB_FIRST ?
+                               AV_PIX_FMT_BGR0 : AV_PIX_FMT_0RGB;
                 else if (fmt->bits_per_pixel == 24)
-                    *pix_fmt = AV_PIX_FMT_RGB24;
+                    *pix_fmt = setup->image_byte_order == XCB_IMAGE_ORDER_LSB_FIRST ?
+                               AV_PIX_FMT_BGR24 : AV_PIX_FMT_RGB24;
                 break;
             case 16:
                 if (fmt->bits_per_pixel == 16)
-                    *pix_fmt = AV_PIX_FMT_RGB565;
+                    *pix_fmt = setup->image_byte_order == XCB_IMAGE_ORDER_LSB_FIRST ?
+                               AV_PIX_FMT_RGB565LE : AV_PIX_FMT_RGB565BE;
                 break;
             case 15:
                 if (fmt->bits_per_pixel == 16)
-                    *pix_fmt = AV_PIX_FMT_RGB555;
+                    *pix_fmt = setup->image_byte_order == XCB_IMAGE_ORDER_LSB_FIRST ?
+                               AV_PIX_FMT_RGB555LE : AV_PIX_FMT_RGB555BE;
                 break;
             case 8:
                 if (fmt->bits_per_pixel == 8)