Re: [RFC PATCH 18/20] lib: image-formats: Add v4l2 formats support

[Nicolas Dufresne]

Le vendredi 22 mars 2019 à 20:44 +0200, Ville SyrjÃlà a Ãcrit :
> On Fri, Mar 22, 2019 at 02:24:29PM -0400, Nicolas Dufresne wrote:
> > Le jeudi 21 mars 2019 à 23:44 +0200, Ville SyrjÃlà a Ãcrit :
> > > On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> > > > Le jeudi 21 mars 2019 à 18:35 +0200, Ville SyrjÃlà a Ãcrit :
> > > > > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > > > > project that is all about matching formats from one API to another: 
> > > > > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > > > > tool too).
> > > > > > 
> > > > > > On the page about DRM, it seems that they got the word that DRM formats
> > > > > > are the native pixel order in little-endian systems:
> > > > > > https://afrantzis.com/pixel-format-guide/drm.html
> > > > > 
> > > > > Looks consistent with the official word in drm_fourcc.h.
> > > > > 
> > > > > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > > > > Format: V4L2_PIX_FMT_XBGR32
> > > > > Is compatible on all systems with:
> > > > >         DRM_FORMAT_XRGB8888
> > > > > Is compatible on little-endian systems with:
> > > > > Is compatible on big-endian systems with:
> > > > > 
> > > > > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > > > > Format: DRM_FORMAT_XRGB8888
> > > > > Is compatible on all systems with:
> > > > >         V4L2_PIX_FMT_XBGR32
> > > > > Is compatible on little-endian systems with:
> > > > > Is compatible on big-endian systems with:
> > > > > 
> > > > > Even works both ways :)
> > > > > 
> > > > > > Perhaps some drivers have been abusing the format definitions, leading
> > > > > > to the inconsistencies that Nicolas could witness?
> > > > > 
> > > > > That is quite possible, perhaps even likely. No one really
> > > > > seems interested in making sure big endian systems actually
> > > > > work properly. I believe the usual approach is to hack
> > > > > around semi-rnadomly until the correct colors accidentally
> > > > > appear on the screen.
> > > > 
> > > > We did not hack around randomly. The code in GStreamer is exactly what
> > > > the DRM and Wayland dev told us to do (they provided the initial
> > > > patches to make it work). These are initially patches from Intel for
> > > > what it's worth (see the wlvideoformat.c header [0]). Even though I
> > > > just notice that in this file, it seems that we ended up with a
> > > > different mapping order for WL and DRM format in 24bit RGB (no
> > > > padding), clearly is a bug. That being said, there is no logical
> > > > meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
> > > > single op.
> > > 
> > > To me little endian just means "little end comes first". So if
> > > you think of things as just a stream of bytes CPU word size
> > > etc. doesn't matter. And I guess if you really wanted to you
> > > could even make a CPU with 24bit word size. 
> > > 
> > > Anyways, to make things more clear drm_fourcc.h could probably
> > > document things better by showing explicitly which bits go into
> > > which byte.
> > > 
> > > I don't know who did what patches for whatever project, but
> > > clearly something has been lost in translation at some point.
> > > 
> > > > Just saying since I would not know which one of the two
> > > > mapping here is right. I would probably have to go testing what DRM
> > > > drivers do, which may not mean anything. I would also ask and get
> > > > contradictory answers.
> > > > 
> > > > I have never myself tested these on big endian drivers though, as you
> > > > say, nobody seems to care about graphics on those anymore. So the easy
> > > > statement is to say they are little endian, like you just did, and
> > > > ignore the legacy, but there is some catch. YUV formats has been added
> > > > without applying this rules.
> > > 
> > > All drm formats follow the same rule (ignoring the recently added
> > > non-byte aligned stuff which I guess don't really follow any rules).
> > > 
> > > > So V4L2 YUYV match YUYV in DRM format name
> > > > instead of little endian UYVY. (at least 4 tested drivers implements it
> > > > this way). Same for NV12, for which little endian CPU representation
> > > > would swap the UV paid on a 16bit word.
> > > 
> > > DRM NV12 and YUYV (YUY2) match the NV12 and YUY2 defined here
> > > https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering
> > 
> > Problem is that these are all expressed MSB first like (the way V4L2
> > and GStreamer do appart for the padding X, which is usually first in
> > V4L2). Let's say you have 2 pixels of YUYV in a 32bit buffer.
> > 
> >    buf[0] = Y
> >    buf[1] = U
> >    buf[2] = Y
> >    buf[3] = V
> 
> This is drm YUYV (aka. YUY2). Well, assuming buf[] is made up of bytes.
> 
> > While with LSB first (was this what you wanted to say ?), this format
> > would be VYUY as:
> > 
> >   buf[0] = V
> >   buf[1] = Y
> >   buf[2] = U
> >   buf[3] = Y
> 
> This is VYUY as far as drm is concerned.
> 
> > But I tested this format on i915, xlnx, msm and imx drm drivers, and
> > they are all mapped as MSB. The LSB version of NV12 is called NV21 in
> > MS pixel formats. It would be really confusing if the LSB rule was to
> > be applied to these format in my opinion, because the names don't
> > explicitly express the placement for the components.
> 
> The names don't really mean anything. Don't try to give any any special
> meaning. They're just that, names. The fact that we used fourccs for
> them was a mistake IMO. IIRC I objected but ended up going with them
> to get the bikeshed painted at all.

I can only agree with you. Note, it was not obvious to me that when you
said the format are little endian you refered to the description next
to the format name inside drm_fourcc.h.

> 
> And yes, the fact that we used a different naming scheme for packed YUV
> vs. RGB was probably a mistake by me. But it was done long ago and we
> have to live with it. Fortunately the mess has gotten much worse since
> then and we are even more inconsistent now. We now YUV formats where
> the A vs. X variants use totally different naming schemes.

Ok, so now that we are set, I'll retake this patch and simply comment
next to each badly mapped format.

> 
> > Anyway, to cut short, as per currently tested implementation of DRM
> > driver, we can keep the RGB mapping static (reversed) for now, but for
> > Maxime, who probably have no clue about all this, the YUYV mapping is
> > correct in this patch (in as of currently tested drivers).
> > 
> > > > To me, all the YUV stuff is the right way, because this is what the
> > > > rest of the world is doing, it's not ambiguous.
> > > > 
> > > > [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86
> > > > 
> > > > 
> > > > 
> > > > Nicolas
> 
>

Attachment: signature.asc
Description: This is a digitally signed message part