| Message ID | 20221025012131.12943-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch. 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 > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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) (Side question: Isn't this DEFAULT behaviour close to selecting a preset, based on primaries?) > 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 ? > 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);
Hi Laurent, On Tue, Oct 25, 2022 at 04:21:31AM +0300, Laurent Pinchart 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@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. > > --- Regardless of the general issue, I can confirm with this patch I can capture full range YUV data on RkISP1. Interestingly, I don't even need to specify a colorspace to cam on the command line. I will investigate if that's intended or not. Tested-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > 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 >
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 ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 - 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 - 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. > > 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. > > 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);
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@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); >
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@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@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);
> >
>
Hi David, On Tue, Oct 25, 2022 at 11:44:41AM +0100, David Plowman via libcamera-devel wrote: > 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? Tested on RkISP1 and it seems to correctly handle setting the quantization, in the same way as Laurent's patch did. As the previous patch, it requires all fields of the ColorSpace instance supplied in the StreamConfiguration to be specified, but as we can't rely on "presets" to fix all fields in the driver I guess there's nothing much we can do here (and, as libcamera actually has preset of colorspaces available to applications, this doesn't seem a big as a concern to me). Thanks j > > Thanks > David > > On Tue, 25 Oct 2022 at 10:51, Hans Verkuil via libcamera-devel > <libcamera-devel@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@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); > > > > >
Hi Laurent On Tue, 25 Oct 2022 at 02:22, Laurent Pinchart via libcamera-devel <libcamera-devel@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@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 >
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@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);
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);
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@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(-)