[libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always set all four V4L2 colorspace fields

[Laurent Pinchart]

Hi Umang,

(CC'ing Hans)

On Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:
> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:
> > When setting a colorspace on a V4L2 video capture device or a subdev
> > source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc
> > and quantization fields as four independent values. Any field set to
> > V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current
> > configuration for that field. This behaviour, while not necessarily well
> > known, is clearly documented in [1] and [2], and is implemented by the
> > rkisp1 driver.
> >
> > The V4L2Device::fromColorSpace() function, when converting from a
> > libcamera ColorSpace to the V4L2 format colorspace-related fields, first
> > attempts to find a match for a colorspace preset. If found, it only sets
> > the colorspace field, and leaves the xfer_func, ycbcr_enc and
> > quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2
> > specification, and prevents setting the transfer function, YCbCr
> > encoding and quantization on at least rkisp1.
> >
> > Fix this issue by dropping the preset lookup, and set all four
> > colorspace-related fields explicitly.
> >
> > [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2
> > [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2
> 
> Neither of these flags are supported widely in the kernel by the 
> drivers, so that's something to think about as well

Yes, I was also surprised to realize this. I learnt about those flags
recently (when you added support for them in commit f094c2922ad57 :-)),
and previously thought that the driver would always honour the colour
space requested by userspace (provided the hardware supports that of
course).

We do set the flag unconditionally, which shouldn't hurt, but the fact
that only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and
another one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the
spec isn't widely followed. Hans, would this mean that a check is
missing in v4l2-compliance ?

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > I expect this change to cause breakages on Raspberry Pi, hence the RFC.
> >
> > The bcm2835-isp driver doesn't comply with the V4L2 API specification. I
> > certainly won't blame it, as I implemented the same non-compliant
> > behaviour in the rkisp1 driver initially, before realizing that V4L2
> > doesn't consider the colorspace field as a preset in set format calls
> > (this has been confirmed by Hans). The question is how to move forward
> 
> Yes, presets are not used although I am finding examples of colorspace 
> fields using
> 
> V4L2_MAP_*_DEFAULT (clrspace)
> 
> macros to determine the specific values for the field (in the case where 
> user-defined _DEFAULT is coming in from application's side)

I've had a quick look at the mainline kernel, and this is what I found:

Drivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set
the default values of xfer_func, ycbcr_enc and quantization (I believe
those are fine):

- imx214
- imx219
- st-mipid02
- imx-mipi-csi
- imx8mq-mipi-csi2

Drivers that accept colorspace only (without honouring the
V4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode
the xfer_func, ycbcr_enc and quantization to corresponding defaults:

- ov5640
- ov5648
- ov7251
- ov8865
- imx-pxp
- camss-video
- rcar-vin
- sun4i-csi

Drivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and
quantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT
(without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):

- dw100
- rcar-fdp1
- imx6-media
- imx7-media-csi

> (Side question: Isn't this DEFAULT behaviour close to selecting a 
> preset, based on primaries?)

If you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,
and xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the
documented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is
that colorspace should be modified, and xfer_func, ycbcr_enc and
quantization should keep their current value.  This is thus different
than using colorspace as a preset.

> > here. Fixing the driver to make it compliant with V4L2 wouldn't be
> > difficult, but I expect non-libcamera users to be surprised. I don't
> 
> Why would they be surprised. AFAICS, unless applications start using [1] 
> [2] they'll have the same behaviour no ?

If we implement support for [1] and/or [2] in the driver, and ignore the
colour space fields in S_FMT when the flags are not set, it could cause
issues with applications that attempt to configure the colour space but
don't set the flag. Those are not V4L2-compliant, but they work today.

> > expect many such users though, so if that's fine, the only possible
> > issue would be synchronizing the changes in the driver and in libcamera.
> >
> > Other options may be possible. I don't think updating the V4L2 API
> > specification to consider the colorspace field as a preset will be well
> > received upstream (and I don't think I would like that much myself to be
> > honest). Keeping a non-compliant implementation in the bcm2835-isp
> > driver with specific handling in libcamera could be done, but I don't
> > think it would be upstreamable.
> >
> > ---
> >   src/libcamera/v4l2_device.cpp | 14 --------------
> >   1 file changed, 14 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index c17b323f8af0..e60a9ae217de 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v
> >   	if (!colorSpace)
> >   		return 0;
> >   
> > -	auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),
> > -				    [&colorSpace](const auto &item) {
> > -					    return colorSpace == item.first;
> > -				    });
> > -	if (itColor != colorSpaceToV4l2.end()) {
> > -		v4l2Format.colorspace = itColor->second;
> > -		/* Leaving all the other fields as "default" should be fine. */
> > -		return 0;
> > -	}
> > -
> > -	/*
> > -	 * If the colorSpace doesn't precisely match a standard color space,
> > -	 * then we must choose a V4L2 colorspace with matching primaries.
> > -	 */
> >   	int ret = 0;
> >   
> >   	auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);

-- 
Regards,

Laurent Pinchart