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

[Umang Jain]

Hi Laurent, Hans,

On 10/25/22 3:21 PM, Hans Verkuil wrote:
> 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.

I think what Laurent meant is, these driver only accepts the colorspace 
field /only/ and the rest fields (xfer,  ycbcr_enc and quantization) are 
selected based on the pass-in colorspace field (using the 
V4L2_MAP_*_DEFAULT macro).

So, these 4 drivers 'accept colorspace only' and ignore the rest fields 
(xfer, ycbcr_enc and quantization)
>
>> - 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.

Makes sense to me now.

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

I am sure you have handled such situation previously as well and put 
usage on a corrective path over time ? What are the options here - other 
than introducing a warning on non v4l2-compliant  usage of 
colorspace/flags by non-libcamera users.

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