[libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always set all four V4L2 colorspace fields
David Plowman
david.plowman at raspberrypi.com
Tue Oct 25 12:44:41 CEST 2022
Hi everyone
Yes, this is a dark little corner, isn't it? I guess the thing that
has tripped me up is the documentation here:
https://docs.kernel.org/userspace-api/media/v4l/colorspaces-defs.html
Of course, it turns out that's describing what you get told as an
output from set_format (I think?), rather than how it behaves as an
input when you try and set it. (Did I get that right?) Anyway, the
idea that these things behave differently certainly makes for a tricky
API which has indeed caught several of us out, and probably lots of
other folks we don't know about.
As regards the patch, I wasn't too keen on it because if you give
fromColorSpace the input ColorSpace::Sycc you'll end up with
V4L2_COLORSPACE_REC709. Now it's not wrong, but it seems slightly
surprising. And drivers (perhaps older ones) that live in a world
where they only look at the colorspace will go wrong. Needless to say,
we still have drivers in that category (mentioning no names), but I'm
not volunteering to go and do soul-destroying work on them.
So I'd prefer this instead:
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index c17b323f..e24bd1d7 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -915,30 +915,36 @@ int V4L2Device::fromColorSpace(const
std::optional<ColorSpace> &colorSpace, T &v
if (!colorSpace)
return 0;
+ int ret = 0;
+
auto itColor = std::find_if(colorSpaceToV4l2.begin(),
colorSpaceToV4l2.end(),
[&colorSpace](const auto &item) {
return colorSpace == item.first;
});
if (itColor != colorSpaceToV4l2.end()) {
+ /*
+ * If we can match the colorspace precisely, then use
that. The trouble
+ * with matching based on the primaries is that Sycc
will choose REC709
+ * as its V4L2 headline colorspace, not JPEG, and this
may seem surprising
+ * and even trip up some less careful drivers.
+ */
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);
- if (itPrimaries != primariesToV4l2.end()) {
- v4l2Format.colorspace = itPrimaries->second;
- } else {
- libcamera::LOG(V4L2, Warning)
- << "Unrecognised primaries in "
- << ColorSpace::toString(colorSpace);
- ret = -EINVAL;
+ else
+ {
+ /*
+ * If the colorSpace doesn't precisely match a
standard color space,
+ * then we must choose a V4L2 colorspace with matching
primaries.
+ */
+ auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);
+ if (itPrimaries != primariesToV4l2.end()) {
+ v4l2Format.colorspace = itPrimaries->second;
+ } else {
+ libcamera::LOG(V4L2, Warning)
+ << "Unrecognised primaries in "
+ << ColorSpace::toString(colorSpace);
+ ret = -EINVAL;
+ }
}
auto itTransfer =
transferFunctionToV4l2.find(colorSpace->transferFunction);
Does that seem reasonable?
Thanks
David
On Tue, 25 Oct 2022 at 10:51, Hans Verkuil via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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.
>
> > - 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);
> >
>
More information about the libcamera-devel
mailing list