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

[Dave Stevenson]

Hi Laurent

On Tue, 25 Oct 2022 at 02:22, Laurent Pinchart via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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
>
> 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.

I don't expect there are any other significant users of bcm2835-isp at
this point, so I have no issues in fixing it once I understand the
correct behaviour.
There are a limited number of combinations currently supported within
the firmware, and I wouldn't want to extend that unless really pushed.
That means that there will be a fair number of cases where the format
requested gets altered in s_format.

There will be other users of bcm2835-codec which wraps the codecs, and
simple 1-in, 1-out ISP for resize/format convert. Largely that will be
used via FFmpeg or GStreamer though, so I wonder if those two
applications follow the spec....
Fixing that driver may have bigger implications. Once bcm2835-isp has
been reviewed and upstreamed, then I'll look at that one - there are
likely to be a fair number of common quirks.

  Dave

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