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

[Hans Verkuil]

On 10/25/22 11:25, Laurent Pinchart wrote:
> 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 ?

Correct, there is no test for this. Very few drivers support this because
it is rarely needed (so no pressure on driver developers to support this
feature) and because there are not many people who understand how to do
this, i.e. there is a learning curve involved.

Also, historically when V4L2 started none of the capture devices had
support for CSC at all, so support for this was never considered.

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

At least these 4 drivers just ignore what userspace sets and set the
colorimetry fields themselves. So I'm not sure what you mean with
'accept colorspace only'. I haven't looked at the next 4.

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

That's a m2m device, they work differently and copy the colorimetry
information from the output format to the capture format (unless it is
a CSC converter).

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

Correct.

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

Right.

Regards,

	Hans

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