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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <seanpaul@chromium.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	linux-media@vger.kernel.org, Daniel Stone <daniels@collabora.com>
Subject: Re: [RFC PATCH 18/20] lib: image-formats: Add v4l2 formats support
Date: Thu, 21 Mar 2019 23:44:55 +0200	[thread overview]
Message-ID: <20190321214455.GL3888@intel.com> (raw)
In-Reply-To: <ac5329d77f83af2804c240ebe479ec323b60aec3.camel@ndufresne.ca>

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

> 
> 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

-- 
Ville Syrjälä
Intel