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

[Laurent Pinchart]

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

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