[{"id":25568,"web_url":"https://patchwork.libcamera.org/comment/25568/","msgid":"","date":"2022-10-25T06:58:52","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch.\n\nOn 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n> When setting a colorspace on a V4L2 video capture device or a subdev\n> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> and quantization fields as four independent values. Any field set to\n> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> configuration for that field. This behaviour, while not necessarily well\n> known, is clearly documented in [1] and [2], and is implemented by the\n> rkisp1 driver.\n>\n> The V4L2Device::fromColorSpace() function, when converting from a\n> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> attempts to find a match for a colorspace preset. If found, it only sets\n> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> specification, and prevents setting the transfer function, YCbCr\n> encoding and quantization on at least rkisp1.\n>\n> Fix this issue by dropping the preset lookup, and set all four\n> colorspace-related fields explicitly.\n>\n> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n\nNeither of these flags are supported widely in the kernel by the \ndrivers, so that's something to think about as well\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n>\n> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> certainly won't blame it, as I implemented the same non-compliant\n> behaviour in the rkisp1 driver initially, before realizing that V4L2\n> doesn't consider the colorspace field as a preset in set format calls\n> (this has been confirmed by Hans). The question is how to move forward\n\nYes, presets are not used although I am finding examples of colorspace \nfields using\n\nV4L2_MAP_*_DEFAULT (clrspace)\n\nmacros to determine the specific values for the field (in the case where \nuser-defined _DEFAULT is coming in from application's side)\n\n(Side question: Isn't this DEFAULT behaviour close to selecting a \npreset, based on primaries?)\n> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> difficult, but I expect non-libcamera users to be surprised. I don't\n\nWhy would they be surprised. AFAICS, unless applications start using [1] \n[2] they'll have the same behaviour no ?\n> expect many such users though, so if that's fine, the only possible\n> issue would be synchronizing the changes in the driver and in libcamera.\n>\n> Other options may be possible. I don't think updating the V4L2 API\n> specification to consider the colorspace field as a preset will be well\n> received upstream (and I don't think I would like that much myself to be\n> honest). Keeping a non-compliant implementation in the bcm2835-isp\n> driver with specific handling in libcamera could be done, but I don't\n> think it would be upstreamable.\n>\n> ---\n> src/libcamera/v4l2_device.cpp | 14 --------------\n> 1 file changed, 14 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c17b323f8af0..e60a9ae217de 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> \tif (!colorSpace)\n> \t\treturn 0;\n> \n> -\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> -\t\t\t\t [&colorSpace](const auto &item) {\n> -\t\t\t\t\t return colorSpace == item.first;\n> -\t\t\t\t });\n> -\tif (itColor != colorSpaceToV4l2.end()) {\n> -\t\tv4l2Format.colorspace = itColor->second;\n> -\t\t/* Leaving all the other fields as \"default\" should be fine. */\n> -\t\treturn 0;\n> -\t}\n> -\n> -\t/*\n> -\t * If the colorSpace doesn't precisely match a standard color space,\n> -\t * then we must choose a V4L2 colorspace with matching primaries.\n> -\t */\n> \tint ret = 0;\n> \n> \tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3700CBD16B\n\tfor ;\n\tTue, 25 Oct 2022 06:59:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6ACFA61F4B;\n\tTue, 25 Oct 2022 08:59:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3279F61F4B\n\tfor ;\n\tTue, 25 Oct 2022 08:58:59 +0200 (CEST)","from [192.168.1.103] (unknown [103.238.109.19])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E1F6E8BF;\n\tTue, 25 Oct 2022 08:58:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666681141;\n\tbh=Ek+NLwmnunrVhScbY0QNl0WXCItr/ape+H1VUevJ0VA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=2xUigRRm7DKa4sGFEbyL6WksVKSlLPs6Lu0VAU6MUQ8dDuReAeOfEtMuVDk7CarCS\n\thT1fJZzw39Zks6++VoWKpAW7udQf3OdeXYDZrf1jsv9V+SCgqaA7VgfPY3LsdQA8bs\n\tnhLQOF3TMp2wk759OLGbLkRl1XDxLkcC8OSizaH2XrJKSYF9S4xDyYnCBk9nIO3tHo\n\tbPM9M+6P7WbDOJaFyvOPfGD91uZ/HV/RcA/IZS3ZSZKAWmsPYXTimRgiEgn2v0V9UD\n\tS4buOEFE9ZyfdOGoO1sSA6Z/8uWZgsiTUctHtpdJxrKIs/ghaCNNo9u0WY1leE6ZAp\n\tf5nR6h81tbvhw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666681138;\n\tbh=Ek+NLwmnunrVhScbY0QNl0WXCItr/ape+H1VUevJ0VA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=TlgwVOuoYJ17SpU0LKjY2hHguTnGAv6sQ3S42bma5apa27BQsU4io6e4IH+bOeb1x\n\t97NQBLKMkZHRjpZQYw1i3jTNwpczfv107uZOLhliYGA4++1WWxRxdxV0PoxNcvO9tI\n\ttk6iJMsjtBXEkYAMQRmCfy7D0GrLkSMqAEYx/ojI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TlgwVOuo\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 25 Oct 2022 12:28:52 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25570,"web_url":"https://patchwork.libcamera.org/comment/25570/","msgid":"<20221025080117.xiwyk7hgfponepah@uno.localdomain>","date":"2022-10-25T08:01:17","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 25, 2022 at 04:21:31AM +0300, Laurent Pinchart wrote:\n> When setting a colorspace on a V4L2 video capture device or a subdev\n> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> and quantization fields as four independent values. Any field set to\n> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> configuration for that field. This behaviour, while not necessarily well\n> known, is clearly documented in [1] and [2], and is implemented by the\n> rkisp1 driver.\n>\n> The V4L2Device::fromColorSpace() function, when converting from a\n> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> attempts to find a match for a colorspace preset. If found, it only sets\n> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> specification, and prevents setting the transfer function, YCbCr\n> encoding and quantization on at least rkisp1.\n>\n> Fix this issue by dropping the preset lookup, and set all four\n> colorspace-related fields explicitly.\n>\n> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n>\n> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> certainly won't blame it, as I implemented the same non-compliant\n> behaviour in the rkisp1 driver initially, before realizing that V4L2\n> doesn't consider the colorspace field as a preset in set format calls\n> (this has been confirmed by Hans). The question is how to move forward\n> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> difficult, but I expect non-libcamera users to be surprised. I don't\n> expect many such users though, so if that's fine, the only possible\n> issue would be synchronizing the changes in the driver and in libcamera.\n>\n> Other options may be possible. I don't think updating the V4L2 API\n> specification to consider the colorspace field as a preset will be well\n> received upstream (and I don't think I would like that much myself to be\n> honest). Keeping a non-compliant implementation in the bcm2835-isp\n> driver with specific handling in libcamera could be done, but I don't\n> think it would be upstreamable.\n>\n> ---\n\nRegardless of the general issue, I can confirm with this patch I can\ncapture full range YUV data on RkISP1.\n\nInterestingly, I don't even need to specify a colorspace to cam on the\ncommand line. I will investigate if that's intended or not.\n\nTested-by: Jacopo Mondi \n\nThanks\n j\n\n> src/libcamera/v4l2_device.cpp | 14 --------------\n> 1 file changed, 14 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c17b323f8af0..e60a9ae217de 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> \tif (!colorSpace)\n> \t\treturn 0;\n>\n> -\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> -\t\t\t\t [&colorSpace](const auto &item) {\n> -\t\t\t\t\t return colorSpace == item.first;\n> -\t\t\t\t });\n> -\tif (itColor != colorSpaceToV4l2.end()) {\n> -\t\tv4l2Format.colorspace = itColor->second;\n> -\t\t/* Leaving all the other fields as \"default\" should be fine. */\n> -\t\treturn 0;\n> -\t}\n> -\n> -\t/*\n> -\t * If the colorSpace doesn't precisely match a standard color space,\n> -\t * then we must choose a V4L2 colorspace with matching primaries.\n> -\t */\n> \tint ret = 0;\n>\n> \tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D18BBD16B\n\tfor ;\n\tTue, 25 Oct 2022 08:01:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB9A262F32;\n\tTue, 25 Oct 2022 10:01:21 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70D6A61F52\n\tfor ;\n\tTue, 25 Oct 2022 10:01:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4E4FA240005;\n\tTue, 25 Oct 2022 08:01:18 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666684881;\n\tbh=+PzP5tqAJEFi21e3d94aQRYkVk4DHzoXrbFTRk08sU8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=y7yFEMQu3viecW+rPxd4eqIlheHOIVT1+Wtc1/qxWh5blbtvc8MxfOrrLSmWMK5No\n\tXRdaFzjVrF/QJGOk1L3AFwv7Kr4M/gOB3A3sfTg5qdQHdVdIQ6ptkg58ULb/3BxMdP\n\tPTGB9OfeZ+sh8I54Id95LRRvsPqsfwZxurkclNNGYPWSy184XHOjcK7OWn08MuR9dB\n\t92DmEG8M/vWxMosKyoxb2uYJ4jn5LrLVh1f5/lYqN0joAKptq0jHmfiKJcDL1ECd6L\n\ta3rPBkaby7g/jQa2927wJ2SGc0g/JvNlbGxYCzPStYYhdic5gduONt7APFmTD0tZot\n\t09bBaEaCOOpgA==","Date":"Tue, 25 Oct 2022 10:01:17 +0200","To":"Laurent Pinchart ","Message-ID":"<20221025080117.xiwyk7hgfponepah@uno.localdomain>","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25571,"web_url":"https://patchwork.libcamera.org/comment/25571/","msgid":"","date":"2022-10-25T09:25:22","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\n(CC'ing Hans)\n\nOn Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:\n> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n> > When setting a colorspace on a V4L2 video capture device or a subdev\n> > source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> > and quantization fields as four independent values. Any field set to\n> > V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> > configuration for that field. This behaviour, while not necessarily well\n> > known, is clearly documented in [1] and [2], and is implemented by the\n> > rkisp1 driver.\n> >\n> > The V4L2Device::fromColorSpace() function, when converting from a\n> > libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> > attempts to find a match for a colorspace preset. If found, it only sets\n> > the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> > quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> > specification, and prevents setting the transfer function, YCbCr\n> > encoding and quantization on at least rkisp1.\n> >\n> > Fix this issue by dropping the preset lookup, and set all four\n> > colorspace-related fields explicitly.\n> >\n> > [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> > [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n> \n> Neither of these flags are supported widely in the kernel by the \n> drivers, so that's something to think about as well\n\nYes, I was also surprised to realize this. I learnt about those flags\nrecently (when you added support for them in commit f094c2922ad57 :-)),\nand previously thought that the driver would always honour the colour\nspace requested by userspace (provided the hardware supports that of\ncourse).\n\nWe do set the flag unconditionally, which shouldn't hurt, but the fact\nthat only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and\nanother one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the\nspec isn't widely followed. Hans, would this mean that a check is\nmissing in v4l2-compliance ?\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n> >\n> > The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> > certainly won't blame it, as I implemented the same non-compliant\n> > behaviour in the rkisp1 driver initially, before realizing that V4L2\n> > doesn't consider the colorspace field as a preset in set format calls\n> > (this has been confirmed by Hans). The question is how to move forward\n> \n> Yes, presets are not used although I am finding examples of colorspace \n> fields using\n> \n> V4L2_MAP_*_DEFAULT (clrspace)\n> \n> macros to determine the specific values for the field (in the case where \n> user-defined _DEFAULT is coming in from application's side)\n\nI've had a quick look at the mainline kernel, and this is what I found:\n\nDrivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set\nthe default values of xfer_func, ycbcr_enc and quantization (I believe\nthose are fine):\n\n- imx214\n- imx219\n- st-mipid02\n- imx-mipi-csi\n- imx8mq-mipi-csi2\n\nDrivers that accept colorspace only (without honouring the\nV4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode\nthe xfer_func, ycbcr_enc and quantization to corresponding defaults:\n\n- ov5640\n- ov5648\n- ov7251\n- ov8865\n- imx-pxp\n- camss-video\n- rcar-vin\n- sun4i-csi\n\nDrivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and\nquantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT\n(without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):\n\n- dw100\n- rcar-fdp1\n- imx6-media\n- imx7-media-csi\n\n> (Side question: Isn't this DEFAULT behaviour close to selecting a \n> preset, based on primaries?)\n\nIf you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,\nand xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the\ndocumented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is\nthat colorspace should be modified, and xfer_func, ycbcr_enc and\nquantization should keep their current value. This is thus different\nthan using colorspace as a preset.\n\n> > here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> > difficult, but I expect non-libcamera users to be surprised. I don't\n> \n> Why would they be surprised. AFAICS, unless applications start using [1] \n> [2] they'll have the same behaviour no ?\n\nIf we implement support for [1] and/or [2] in the driver, and ignore the\ncolour space fields in S_FMT when the flags are not set, it could cause\nissues with applications that attempt to configure the colour space but\ndon't set the flag. Those are not V4L2-compliant, but they work today.\n\n> > expect many such users though, so if that's fine, the only possible\n> > issue would be synchronizing the changes in the driver and in libcamera.\n> >\n> > Other options may be possible. I don't think updating the V4L2 API\n> > specification to consider the colorspace field as a preset will be well\n> > received upstream (and I don't think I would like that much myself to be\n> > honest). Keeping a non-compliant implementation in the bcm2835-isp\n> > driver with specific handling in libcamera could be done, but I don't\n> > think it would be upstreamable.\n> >\n> > ---\n> > src/libcamera/v4l2_device.cpp | 14 --------------\n> > 1 file changed, 14 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index c17b323f8af0..e60a9ae217de 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> > \tif (!colorSpace)\n> > \t\treturn 0;\n> > \n> > -\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> > -\t\t\t\t [&colorSpace](const auto &item) {\n> > -\t\t\t\t\t return colorSpace == item.first;\n> > -\t\t\t\t });\n> > -\tif (itColor != colorSpaceToV4l2.end()) {\n> > -\t\tv4l2Format.colorspace = itColor->second;\n> > -\t\t/* Leaving all the other fields as \"default\" should be fine. */\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > -\t/*\n> > -\t * If the colorSpace doesn't precisely match a standard color space,\n> > -\t * then we must choose a V4L2 colorspace with matching primaries.\n> > -\t */\n> > \tint ret = 0;\n> > \n> > \tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5E3CBDB16\n\tfor ;\n\tTue, 25 Oct 2022 09:25:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5FE162F34;\n\tTue, 25 Oct 2022 11:25:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 412F661F52\n\tfor ;\n\tTue, 25 Oct 2022 11:25:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 891338BF;\n\tTue, 25 Oct 2022 11:25:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666689949;\n\tbh=MvnMIxGuUdRO4Buavt3xGyfHvGSkbrEUzhr7f5FYh+c=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=DTf1ucOyCjT33FAy2hn53BPNhBj2gio0p/2iTLLEfFVsrsPpvuA4oSPurGtzcUKrr\n\t7qQgzHPiD5TVBnqV8Im5aDZpyuC/2b4fpbhgA7YLwpw8FSczEzCaCBopaScH3PX+5T\n\tW+CEMuecQDKHi1RRld6T0s25wleZEUlyxdzslcBzNQip35wcoGnZXAb7qSV3ZYvW6D\n\tGGERoufzx6DDaeaunq5CUmTUCt8Wvvafl9UxsuCPtGA79UhlfH3XyXf8NZoHqoQrhF\n\t6o1Ygqeg54bLmOMMdhKX0Vhd9kw5ssWmXWHlrxuqFmGPZ0ZjUBQG+t/ezR+mOEZHI1\n\t2+F7p5jrg+OFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666689947;\n\tbh=MvnMIxGuUdRO4Buavt3xGyfHvGSkbrEUzhr7f5FYh+c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K4+QbU1UnAu6oSXSmH6oDDRuIqiVKSBJkN+dvpckEO5QTFD0Nz9BOfzoFeg3vvkHx\n\ty8mJNBkPx7DjmskpQ4UfHBtwgF3YxJl6r8OGysPTc7y8G16fiJCo85E5tn3xy5sIMl\n\t1SQWCpf/qczKOC8LEo7UOzf4niy5u2H0giy9ghc0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K4+QbU1U\"; dkim-atps=neutral","Date":"Tue, 25 Oct 2022 12:25:22 +0300","To":"Umang Jain ","Message-ID":"","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Hans Verkuil ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25572,"web_url":"https://patchwork.libcamera.org/comment/25572/","msgid":"<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","date":"2022-10-25T09:51:19","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":43,"url":"https://patchwork.libcamera.org/api/people/43/","name":"Hans Verkuil","email":"hverkuil-cisco@xs4all.nl"},"content":"On 10/25/22 11:25, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> (CC'ing Hans)\n> \n> On Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:\n>> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n>>> When setting a colorspace on a V4L2 video capture device or a subdev\n>>> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n>>> and quantization fields as four independent values. Any field set to\n>>> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n>>> configuration for that field. This behaviour, while not necessarily well\n>>> known, is clearly documented in [1] and [2], and is implemented by the\n>>> rkisp1 driver.\n>>>\n>>> The V4L2Device::fromColorSpace() function, when converting from a\n>>> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n>>> attempts to find a match for a colorspace preset. If found, it only sets\n>>> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n>>> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n>>> specification, and prevents setting the transfer function, YCbCr\n>>> encoding and quantization on at least rkisp1.\n>>>\n>>> Fix this issue by dropping the preset lookup, and set all four\n>>> colorspace-related fields explicitly.\n>>>\n>>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n>>> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n>>\n>> Neither of these flags are supported widely in the kernel by the \n>> drivers, so that's something to think about as well\n> \n> Yes, I was also surprised to realize this. I learnt about those flags\n> recently (when you added support for them in commit f094c2922ad57 :-)),\n> and previously thought that the driver would always honour the colour\n> space requested by userspace (provided the hardware supports that of\n> course).\n> \n> We do set the flag unconditionally, which shouldn't hurt, but the fact\n> that only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and\n> another one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the\n> spec isn't widely followed. Hans, would this mean that a check is\n> missing in v4l2-compliance ?\n\nCorrect, there is no test for this. Very few drivers support this because\nit is rarely needed (so no pressure on driver developers to support this\nfeature) and because there are not many people who understand how to do\nthis, i.e. there is a learning curve involved.\n\nAlso, historically when V4L2 started none of the capture devices had\nsupport for CSC at all, so support for this was never considered.\n\n> \n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n>>>\n>>> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n>>> certainly won't blame it, as I implemented the same non-compliant\n>>> behaviour in the rkisp1 driver initially, before realizing that V4L2\n>>> doesn't consider the colorspace field as a preset in set format calls\n>>> (this has been confirmed by Hans). The question is how to move forward\n>>\n>> Yes, presets are not used although I am finding examples of colorspace \n>> fields using\n>>\n>> V4L2_MAP_*_DEFAULT (clrspace)\n>>\n>> macros to determine the specific values for the field (in the case where \n>> user-defined _DEFAULT is coming in from application's side)\n> \n> I've had a quick look at the mainline kernel, and this is what I found:\n> \n> Drivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set\n> the default values of xfer_func, ycbcr_enc and quantization (I believe\n> those are fine):\n> \n> - imx214\n> - imx219\n> - st-mipid02\n> - imx-mipi-csi\n> - imx8mq-mipi-csi2\n> \n> Drivers that accept colorspace only (without honouring the\n> V4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode\n> the xfer_func, ycbcr_enc and quantization to corresponding defaults:\n> \n> - ov5640\n> - ov5648\n> - ov7251\n> - ov8865\n\nAt least these 4 drivers just ignore what userspace sets and set the\ncolorimetry fields themselves. So I'm not sure what you mean with\n'accept colorspace only'. I haven't looked at the next 4.\n\n> - imx-pxp\n> - camss-video\n> - rcar-vin\n> - sun4i-csi\n> \n> Drivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and\n> quantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT\n> (without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):\n> \n> - dw100\n\nThat's a m2m device, they work differently and copy the colorimetry\ninformation from the output format to the capture format (unless it is\na CSC converter).\n\n> - rcar-fdp1\n> - imx6-media\n> - imx7-media-csi\n> \n>> (Side question: Isn't this DEFAULT behaviour close to selecting a \n>> preset, based on primaries?)\n> \n> If you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,\n> and xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the\n> documented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is\n> that colorspace should be modified, and xfer_func, ycbcr_enc and\n> quantization should keep their current value. This is thus different\n> than using colorspace as a preset.\n\nCorrect.\n\n> \n>>> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n>>> difficult, but I expect non-libcamera users to be surprised. I don't\n>>\n>> Why would they be surprised. AFAICS, unless applications start using [1] \n>> [2] they'll have the same behaviour no ?\n> \n> If we implement support for [1] and/or [2] in the driver, and ignore the\n> colour space fields in S_FMT when the flags are not set, it could cause\n> issues with applications that attempt to configure the colour space but\n> don't set the flag. Those are not V4L2-compliant, but they work today.\n\nRight.\n\nRegards,\n\n\tHans\n\n> \n>>> expect many such users though, so if that's fine, the only possible\n>>> issue would be synchronizing the changes in the driver and in libcamera.\n>>>\n>>> Other options may be possible. I don't think updating the V4L2 API\n>>> specification to consider the colorspace field as a preset will be well\n>>> received upstream (and I don't think I would like that much myself to be\n>>> honest). Keeping a non-compliant implementation in the bcm2835-isp\n>>> driver with specific handling in libcamera could be done, but I don't\n>>> think it would be upstreamable.\n>>>\n>>> ---\n>>> src/libcamera/v4l2_device.cpp | 14 --------------\n>>> 1 file changed, 14 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>> index c17b323f8af0..e60a9ae217de 100644\n>>> --- a/src/libcamera/v4l2_device.cpp\n>>> +++ b/src/libcamera/v4l2_device.cpp\n>>> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n>>> \tif (!colorSpace)\n>>> \t\treturn 0;\n>>> \n>>> -\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n>>> -\t\t\t\t [&colorSpace](const auto &item) {\n>>> -\t\t\t\t\t return colorSpace == item.first;\n>>> -\t\t\t\t });\n>>> -\tif (itColor != colorSpaceToV4l2.end()) {\n>>> -\t\tv4l2Format.colorspace = itColor->second;\n>>> -\t\t/* Leaving all the other fields as \"default\" should be fine. */\n>>> -\t\treturn 0;\n>>> -\t}\n>>> -\n>>> -\t/*\n>>> -\t * If the colorSpace doesn't precisely match a standard color space,\n>>> -\t * then we must choose a V4L2 colorspace with matching primaries.\n>>> -\t */\n>>> \tint ret = 0;\n>>> \n>>> \tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3C164BD16B\n\tfor ;\n\tTue, 25 Oct 2022 09:51:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8654E62F35;\n\tTue, 25 Oct 2022 11:51:22 +0200 (CEST)","from ewsoutbound.kpnmail.nl (ewsoutbound.kpnmail.nl\n\t[195.121.94.170])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 581DF61F52\n\tfor ;\n\tTue, 25 Oct 2022 11:51:21 +0200 (CEST)","from smtp.kpnmail.nl (unknown [10.31.155.39])\n\tby ewsoutbound.so.kpn.org (Halon) with ESMTPS\n\tid 92c74e88-544a-11ed-8a67-005056ab378f;\n\tTue, 25 Oct 2022 11:51:18 +0200 (CEST)","from [10.47.77.219] (unknown [173.38.220.59])\n\tby smtp.xs4all.nl (Halon) with ESMTPSA\n\tid 938fac21-544a-11ed-b8b1-005056ab7447;\n\tTue, 25 Oct 2022 11:51:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666691482;\n\tbh=nxukUzw3m1+Z7H6+rf2klJC6mO2UOyEhxRwKcbueiyQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iCxI83YO/0OlF74F/Rvk7CuZhxKjAFnalMr03pF/D1Zhfz5y1/O1TjPUPqwqZhiqq\n\tA9vXxASY/39qH8bPSDuJQiTvgRRNqtii9wsYKG5j2Sp2ZLmXbYbGNjlZCc7v3MIuDS\n\tfqX4Riz3Yb6t5kcyUaY1G4jHhO4bX/M0KjImPgO91efTtsi6xBjInBeEi2m0he4ySR\n\t7gJXok12WekVlDe28PvDCN3czNbLUwIZwwQO6C1yt0jHSC+vFL+dsn7N56cpswZ4iH\n\tbUoNgNqQn+bBJ3wuQwSS/rVR3ia1HYyp0VlYCqih0HPcuhG/y230vNnh9633+7+ebr\n\twyhgC1UZMko4Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=xs4all01;\n\th=content-type:from:to:subject:mime-version:date:message-id;\n\tbh=lgWLtbMqngyidyohCuv2DUmt5F1Hwg8On2nJTSRz1Xw=;\n\tb=bzsb+bFyaTzFPtvDtcHv10vwEthVGYbS35JoB/G2i5X1znfZZoafMAqdD/B9M8pqvg4ppodYaxAie\n\tmAwo34DrRrYaHDbjizynBErHOVFkUm9C5NjK0XKLTEJPFuSIDo2z0dUZSGNWMPzWjFlaNzh3qLULvp\n\t+6gI/QGAiDQAZW+clH4a4bSODN7CBjJWooQAOZh1UBqcqi/47GmftqKejyAIhTafEBu4zg6WFWrhgm\n\tM8WIdduiuksHqrltPDpDqNkoEBjTtHEri7URu3DMLlM2E5RUh0rnaGIyxqAu1eWpJXbzw1rQFdDCdp\n\t2zNvx2AMo+iKkLEihEKpPIgie+gQf9A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"bzsb+bFy\"; dkim-atps=neutral","X-KPN-MessageId":"92c74e88-544a-11ed-8a67-005056ab378f","X-KPN-MID":"33|NI+ddtbBnAwpsxfpUwI6odsiLuDg689BD4OBPix7fItvC34DXGdlsMwNy9sMnIw\n\thoUFOnKjnszAtk0A8j9NW9hqgL8P1KsLZ1zxyWV+5Q/U=","X-KPN-VerifiedSender":"Yes","X-CMASSUN":"33|AWnGot7Eb8JHxRpmCVARtf8isSFiOL5WlOTYFb6v/aewLmxvcNxznTGP6v9ilaJ\n\tvRH3aQV7mrcTc2egXI0ktTg==","X-Originating-IP":"173.38.220.59","Message-ID":"<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","Date":"Tue, 25 Oct 2022 11:51:19 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.3.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Hans Verkuil via libcamera-devel ","Reply-To":"Hans Verkuil ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25573,"web_url":"https://patchwork.libcamera.org/comment/25573/","msgid":"","date":"2022-10-25T10:44:41","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nYes, this is a dark little corner, isn't it? I guess the thing that\nhas tripped me up is the documentation here:\nhttps://docs.kernel.org/userspace-api/media/v4l/colorspaces-defs.html\n\nOf course, it turns out that's describing what you get told as an\noutput from set_format (I think?), rather than how it behaves as an\ninput when you try and set it. (Did I get that right?) Anyway, the\nidea that these things behave differently certainly makes for a tricky\nAPI which has indeed caught several of us out, and probably lots of\nother folks we don't know about.\n\nAs regards the patch, I wasn't too keen on it because if you give\nfromColorSpace the input ColorSpace::Sycc you'll end up with\nV4L2_COLORSPACE_REC709. Now it's not wrong, but it seems slightly\nsurprising. And drivers (perhaps older ones) that live in a world\nwhere they only look at the colorspace will go wrong. Needless to say,\nwe still have drivers in that category (mentioning no names), but I'm\nnot volunteering to go and do soul-destroying work on them.\n\nSo I'd prefer this instead:\n\ndiff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\nindex c17b323f..e24bd1d7 100644\n--- a/src/libcamera/v4l2_device.cpp\n+++ b/src/libcamera/v4l2_device.cpp\n@@ -915,30 +915,36 @@ int V4L2Device::fromColorSpace(const\nstd::optional &colorSpace, T &v\n if (!colorSpace)\n return 0;\n\n+ int ret = 0;\n+\n auto itColor = std::find_if(colorSpaceToV4l2.begin(),\ncolorSpaceToV4l2.end(),\n [&colorSpace](const auto &item) {\n return colorSpace == item.first;\n });\n if (itColor != colorSpaceToV4l2.end()) {\n+ /*\n+ * If we can match the colorspace precisely, then use\nthat. The trouble\n+ * with matching based on the primaries is that Sycc\nwill choose REC709\n+ * as its V4L2 headline colorspace, not JPEG, and this\nmay seem surprising\n+ * and even trip up some less careful drivers.\n+ */\n v4l2Format.colorspace = itColor->second;\n- /* Leaving all the other fields as \"default\" should be fine. */\n- return 0;\n }\n-\n- /*\n- * If the colorSpace doesn't precisely match a standard color space,\n- * then we must choose a V4L2 colorspace with matching primaries.\n- */\n- int ret = 0;\n-\n- auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n- if (itPrimaries != primariesToV4l2.end()) {\n- v4l2Format.colorspace = itPrimaries->second;\n- } else {\n- libcamera::LOG(V4L2, Warning)\n- << \"Unrecognised primaries in \"\n- << ColorSpace::toString(colorSpace);\n- ret = -EINVAL;\n+ else\n+ {\n+ /*\n+ * If the colorSpace doesn't precisely match a\nstandard color space,\n+ * then we must choose a V4L2 colorspace with matching\nprimaries.\n+ */\n+ auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n+ if (itPrimaries != primariesToV4l2.end()) {\n+ v4l2Format.colorspace = itPrimaries->second;\n+ } else {\n+ libcamera::LOG(V4L2, Warning)\n+ << \"Unrecognised primaries in \"\n+ << ColorSpace::toString(colorSpace);\n+ ret = -EINVAL;\n+ }\n }\n\n auto itTransfer =\ntransferFunctionToV4l2.find(colorSpace->transferFunction);\n\nDoes that seem reasonable?\n\nThanks\nDavid\n\nOn Tue, 25 Oct 2022 at 10:51, Hans Verkuil via libcamera-devel\n wrote:\n>\n> On 10/25/22 11:25, Laurent Pinchart wrote:\n> > Hi Umang,\n> >\n> > (CC'ing Hans)\n> >\n> > On Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:\n> >> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n> >>> When setting a colorspace on a V4L2 video capture device or a subdev\n> >>> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> >>> and quantization fields as four independent values. Any field set to\n> >>> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> >>> configuration for that field. This behaviour, while not necessarily well\n> >>> known, is clearly documented in [1] and [2], and is implemented by the\n> >>> rkisp1 driver.\n> >>>\n> >>> The V4L2Device::fromColorSpace() function, when converting from a\n> >>> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> >>> attempts to find a match for a colorspace preset. If found, it only sets\n> >>> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> >>> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> >>> specification, and prevents setting the transfer function, YCbCr\n> >>> encoding and quantization on at least rkisp1.\n> >>>\n> >>> Fix this issue by dropping the preset lookup, and set all four\n> >>> colorspace-related fields explicitly.\n> >>>\n> >>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> >>> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n> >>\n> >> Neither of these flags are supported widely in the kernel by the\n> >> drivers, so that's something to think about as well\n> >\n> > Yes, I was also surprised to realize this. I learnt about those flags\n> > recently (when you added support for them in commit f094c2922ad57 :-)),\n> > and previously thought that the driver would always honour the colour\n> > space requested by userspace (provided the hardware supports that of\n> > course).\n> >\n> > We do set the flag unconditionally, which shouldn't hurt, but the fact\n> > that only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and\n> > another one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the\n> > spec isn't widely followed. Hans, would this mean that a check is\n> > missing in v4l2-compliance ?\n>\n> Correct, there is no test for this. Very few drivers support this because\n> it is rarely needed (so no pressure on driver developers to support this\n> feature) and because there are not many people who understand how to do\n> this, i.e. there is a learning curve involved.\n>\n> Also, historically when V4L2 started none of the capture devices had\n> support for CSC at all, so support for this was never considered.\n>\n> >\n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n> >>>\n> >>> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> >>> certainly won't blame it, as I implemented the same non-compliant\n> >>> behaviour in the rkisp1 driver initially, before realizing that V4L2\n> >>> doesn't consider the colorspace field as a preset in set format calls\n> >>> (this has been confirmed by Hans). The question is how to move forward\n> >>\n> >> Yes, presets are not used although I am finding examples of colorspace\n> >> fields using\n> >>\n> >> V4L2_MAP_*_DEFAULT (clrspace)\n> >>\n> >> macros to determine the specific values for the field (in the case where\n> >> user-defined _DEFAULT is coming in from application's side)\n> >\n> > I've had a quick look at the mainline kernel, and this is what I found:\n> >\n> > Drivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set\n> > the default values of xfer_func, ycbcr_enc and quantization (I believe\n> > those are fine):\n> >\n> > - imx214\n> > - imx219\n> > - st-mipid02\n> > - imx-mipi-csi\n> > - imx8mq-mipi-csi2\n> >\n> > Drivers that accept colorspace only (without honouring the\n> > V4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode\n> > the xfer_func, ycbcr_enc and quantization to corresponding defaults:\n> >\n> > - ov5640\n> > - ov5648\n> > - ov7251\n> > - ov8865\n>\n> At least these 4 drivers just ignore what userspace sets and set the\n> colorimetry fields themselves. So I'm not sure what you mean with\n> 'accept colorspace only'. I haven't looked at the next 4.\n>\n> > - imx-pxp\n> > - camss-video\n> > - rcar-vin\n> > - sun4i-csi\n> >\n> > Drivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and\n> > quantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT\n> > (without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):\n> >\n> > - dw100\n>\n> That's a m2m device, they work differently and copy the colorimetry\n> information from the output format to the capture format (unless it is\n> a CSC converter).\n>\n> > - rcar-fdp1\n> > - imx6-media\n> > - imx7-media-csi\n> >\n> >> (Side question: Isn't this DEFAULT behaviour close to selecting a\n> >> preset, based on primaries?)\n> >\n> > If you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,\n> > and xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the\n> > documented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is\n> > that colorspace should be modified, and xfer_func, ycbcr_enc and\n> > quantization should keep their current value. This is thus different\n> > than using colorspace as a preset.\n>\n> Correct.\n>\n> >\n> >>> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> >>> difficult, but I expect non-libcamera users to be surprised. I don't\n> >>\n> >> Why would they be surprised. AFAICS, unless applications start using [1]\n> >> [2] they'll have the same behaviour no ?\n> >\n> > If we implement support for [1] and/or [2] in the driver, and ignore the\n> > colour space fields in S_FMT when the flags are not set, it could cause\n> > issues with applications that attempt to configure the colour space but\n> > don't set the flag. Those are not V4L2-compliant, but they work today.\n>\n> Right.\n>\n> Regards,\n>\n> Hans\n>\n> >\n> >>> expect many such users though, so if that's fine, the only possible\n> >>> issue would be synchronizing the changes in the driver and in libcamera.\n> >>>\n> >>> Other options may be possible. I don't think updating the V4L2 API\n> >>> specification to consider the colorspace field as a preset will be well\n> >>> received upstream (and I don't think I would like that much myself to be\n> >>> honest). Keeping a non-compliant implementation in the bcm2835-isp\n> >>> driver with specific handling in libcamera could be done, but I don't\n> >>> think it would be upstreamable.\n> >>>\n> >>> ---\n> >>> src/libcamera/v4l2_device.cpp | 14 --------------\n> >>> 1 file changed, 14 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index c17b323f8af0..e60a9ae217de 100644\n> >>> --- a/src/libcamera/v4l2_device.cpp\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> >>> if (!colorSpace)\n> >>> return 0;\n> >>>\n> >>> - auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> >>> - [&colorSpace](const auto &item) {\n> >>> - return colorSpace == item.first;\n> >>> - });\n> >>> - if (itColor != colorSpaceToV4l2.end()) {\n> >>> - v4l2Format.colorspace = itColor->second;\n> >>> - /* Leaving all the other fields as \"default\" should be fine. */\n> >>> - return 0;\n> >>> - }\n> >>> -\n> >>> - /*\n> >>> - * If the colorSpace doesn't precisely match a standard color space,\n> >>> - * then we must choose a V4L2 colorspace with matching primaries.\n> >>> - */\n> >>> int ret = 0;\n> >>>\n> >>> auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF5DCBDB16\n\tfor ;\n\tTue, 25 Oct 2022 10:44:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FB8B62EFC;\n\tTue, 25 Oct 2022 12:44:56 +0200 (CEST)","from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com\n\t[IPv6:2607:f8b0:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCDC461F4C\n\tfor ;\n\tTue, 25 Oct 2022 12:44:54 +0200 (CEST)","by mail-pg1-x52d.google.com with SMTP id s196so11113240pgs.3\n\tfor ;\n\tTue, 25 Oct 2022 03:44:54 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666694696;\n\tbh=67bhkuZHIEqru7O++aSsinTuUIkYYRagK0wiTmspqP0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mdhyE2iW2ozw5/0f53CeLg/9HKsPVNpZyaJ34yq+v3E25MeVfNDpmNW3CQtgTTYc7\n\tVeMctTnBRpR94gMdrNYw1PApyRqlNsX8fl1rfBz3ZEFjPdsHlVmhNL2GBKK8jZ8Afw\n\tW8AkDaaCzILAH2aHcF+apbnEHQusRjFFAhlJGVdOjIK584Jya90nqYrSrykIOgd3Aa\n\t9pDYhuvwbyUUdVPcmvLNK7FGTj66FGOtstKBkA70cob8X4i6BrvR/ubioZ9BdzGcbW\n\t+vg7roFUay3HA9vmrr2GTIKWfLJwLRHjss3YOrvhSMvwj5Tlb9iGoWHPJEm76HOJHM\n\t4PG8fNGbYcRxg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=1wztcbNdRDI8K0u84lVqal9wQYWKW/3/MgBXBwUO1vg=;\n\tb=ivePiA6QVI0NcOYPk7JapEVaWV04goDr/BSy6nky8puLF3Mvu3mnnl23YleE1A92iu\n\t0XrF8u6IxIvhRzSl8TgLFedMqNehlts0mFblnJZRrL51Bs5XxxqLSEusTQ3UUOEHfD7u\n\td/lhN3siATvXh7y4kJw6CyQytkyzgLZ1F1iPJC3WqF6pnRHEPGdyveyslMqFYx2wIWiN\n\tc9rAKKWItEyJRaVGqNa0E4rD7OPTXzBtUkIDt7jdTweuIeoVnP2vS6zHtYmjx1+YdKJX\n\tMy8ooeg5/yhq+nStHm/cAsyxHiZqSJbR3mUjonr+35sY8xoT2y3qJKqHJ3vfLN5TduyR\n\tWthA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ivePiA6Q\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=1wztcbNdRDI8K0u84lVqal9wQYWKW/3/MgBXBwUO1vg=;\n\tb=OB89eGMOWurp3mhSJZXVwB8QSURCkbRbXW3P3fwSZ9TFQkroMGt8/jXtbmmJ1yNJTz\n\t8PAYB+TcGG85e6MWiIoeH6i6hizIIHG8roxYPtBreKr82eZx57zJWDEG4tTQD9PB+J2P\n\tBIZF3O2LaX9jO2UsA9qcwPQcnYlUwrUpLNmmEdbPXoSl4I4dnY6NlTyvDYxjBwVaFbLa\n\tYilmsj6PNFh8yGafcjJ5DIDkVuAL36jtZeuViV2xzJLGT3Qp1rkFyM+V2ULw4Cb7aG+f\n\tlC2cwrfeKpZkKdIm6wK1gRMUvUpJKH+p1EXGm/COU6i7pvHjsvp+LKSopjBaTrXkpHts\n\tgFpQ==","X-Gm-Message-State":"ACrzQf2W7Mat6Gj7qZO07m1C7GYcepEZiV5JfFPfcb3JqdlPxJj5PZgI\n\t12L8QrPN6HsejhGHa7YgjllxqrXxQzJlVKyy5hS1vA==","X-Google-Smtp-Source":"AMsMyM6TS8T1+Jf1ame3UE5u7JjcvcRF7muouMSFTJnL7uB9wem14iJiWDWYi1J6RB+gOFk2Nk9sc0bt+r97672rL5A=","X-Received":"by 2002:a63:d90c:0:b0:462:cfa2:285b with SMTP id\n\tr12-20020a63d90c000000b00462cfa2285bmr32229596pgg.202.1666694693128;\n\tTue, 25 Oct 2022 03:44:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","In-Reply-To":"<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","Date":"Tue, 25 Oct 2022 11:44:41 +0100","Message-ID":"","To":"Hans Verkuil ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25577,"web_url":"https://patchwork.libcamera.org/comment/25577/","msgid":"<20221025111531.peuq6ucvpawgu25o@uno.localdomain>","date":"2022-10-25T11:15:31","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Oct 25, 2022 at 11:44:41AM +0100, David Plowman via libcamera-devel wrote:\n> Hi everyone\n>\n> Yes, this is a dark little corner, isn't it? I guess the thing that\n> has tripped me up is the documentation here:\n> https://docs.kernel.org/userspace-api/media/v4l/colorspaces-defs.html\n>\n> Of course, it turns out that's describing what you get told as an\n> output from set_format (I think?), rather than how it behaves as an\n> input when you try and set it. (Did I get that right?) Anyway, the\n> idea that these things behave differently certainly makes for a tricky\n> API which has indeed caught several of us out, and probably lots of\n> other folks we don't know about.\n>\n> As regards the patch, I wasn't too keen on it because if you give\n> fromColorSpace the input ColorSpace::Sycc you'll end up with\n> V4L2_COLORSPACE_REC709. Now it's not wrong, but it seems slightly\n> surprising. And drivers (perhaps older ones) that live in a world\n> where they only look at the colorspace will go wrong. Needless to say,\n> we still have drivers in that category (mentioning no names), but I'm\n> not volunteering to go and do soul-destroying work on them.\n>\n> So I'd prefer this instead:\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c17b323f..e24bd1d7 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -915,30 +915,36 @@ int V4L2Device::fromColorSpace(const\n> std::optional &colorSpace, T &v\n> if (!colorSpace)\n> return 0;\n>\n> + int ret = 0;\n> +\n> auto itColor = std::find_if(colorSpaceToV4l2.begin(),\n> colorSpaceToV4l2.end(),\n> [&colorSpace](const auto &item) {\n> return colorSpace == item.first;\n> });\n> if (itColor != colorSpaceToV4l2.end()) {\n> + /*\n> + * If we can match the colorspace precisely, then use\n> that. The trouble\n> + * with matching based on the primaries is that Sycc\n> will choose REC709\n> + * as its V4L2 headline colorspace, not JPEG, and this\n> may seem surprising\n> + * and even trip up some less careful drivers.\n> + */\n> v4l2Format.colorspace = itColor->second;\n> - /* Leaving all the other fields as \"default\" should be fine. */\n> - return 0;\n> }\n> -\n> - /*\n> - * If the colorSpace doesn't precisely match a standard color space,\n> - * then we must choose a V4L2 colorspace with matching primaries.\n> - */\n> - int ret = 0;\n> -\n> - auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> - if (itPrimaries != primariesToV4l2.end()) {\n> - v4l2Format.colorspace = itPrimaries->second;\n> - } else {\n> - libcamera::LOG(V4L2, Warning)\n> - << \"Unrecognised primaries in \"\n> - << ColorSpace::toString(colorSpace);\n> - ret = -EINVAL;\n> + else\n> + {\n> + /*\n> + * If the colorSpace doesn't precisely match a\n> standard color space,\n> + * then we must choose a V4L2 colorspace with matching\n> primaries.\n> + */\n> + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> + if (itPrimaries != primariesToV4l2.end()) {\n> + v4l2Format.colorspace = itPrimaries->second;\n> + } else {\n> + libcamera::LOG(V4L2, Warning)\n> + << \"Unrecognised primaries in \"\n> + << ColorSpace::toString(colorSpace);\n> + ret = -EINVAL;\n> + }\n> }\n>\n> auto itTransfer =\n> transferFunctionToV4l2.find(colorSpace->transferFunction);\n>\n> Does that seem reasonable?\n\nTested on RkISP1 and it seems to correctly handle setting the\nquantization, in the same way as Laurent's patch did.\n\nAs the previous patch, it requires all fields of the ColorSpace\ninstance supplied in the StreamConfiguration to be specified, but as\nwe can't rely on \"presets\" to fix all fields in the driver I guess there's\nnothing much we can do here (and, as libcamera actually has preset of\ncolorspaces available to applications, this doesn't seem a big as a concern\nto me).\n\nThanks\n j\n\n>\n> Thanks\n> David\n>\n> On Tue, 25 Oct 2022 at 10:51, Hans Verkuil via libcamera-devel\n> wrote:\n> >\n> > On 10/25/22 11:25, Laurent Pinchart wrote:\n> > > Hi Umang,\n> > >\n> > > (CC'ing Hans)\n> > >\n> > > On Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:\n> > >> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n> > >>> When setting a colorspace on a V4L2 video capture device or a subdev\n> > >>> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> > >>> and quantization fields as four independent values. Any field set to\n> > >>> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> > >>> configuration for that field. This behaviour, while not necessarily well\n> > >>> known, is clearly documented in [1] and [2], and is implemented by the\n> > >>> rkisp1 driver.\n> > >>>\n> > >>> The V4L2Device::fromColorSpace() function, when converting from a\n> > >>> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> > >>> attempts to find a match for a colorspace preset. If found, it only sets\n> > >>> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> > >>> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> > >>> specification, and prevents setting the transfer function, YCbCr\n> > >>> encoding and quantization on at least rkisp1.\n> > >>>\n> > >>> Fix this issue by dropping the preset lookup, and set all four\n> > >>> colorspace-related fields explicitly.\n> > >>>\n> > >>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> > >>> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n> > >>\n> > >> Neither of these flags are supported widely in the kernel by the\n> > >> drivers, so that's something to think about as well\n> > >\n> > > Yes, I was also surprised to realize this. I learnt about those flags\n> > > recently (when you added support for them in commit f094c2922ad57 :-)),\n> > > and previously thought that the driver would always honour the colour\n> > > space requested by userspace (provided the hardware supports that of\n> > > course).\n> > >\n> > > We do set the flag unconditionally, which shouldn't hurt, but the fact\n> > > that only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and\n> > > another one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the\n> > > spec isn't widely followed. Hans, would this mean that a check is\n> > > missing in v4l2-compliance ?\n> >\n> > Correct, there is no test for this. Very few drivers support this because\n> > it is rarely needed (so no pressure on driver developers to support this\n> > feature) and because there are not many people who understand how to do\n> > this, i.e. there is a learning curve involved.\n> >\n> > Also, historically when V4L2 started none of the capture devices had\n> > support for CSC at all, so support for this was never considered.\n> >\n> > >\n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> ---\n> > >>> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n> > >>>\n> > >>> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> > >>> certainly won't blame it, as I implemented the same non-compliant\n> > >>> behaviour in the rkisp1 driver initially, before realizing that V4L2\n> > >>> doesn't consider the colorspace field as a preset in set format calls\n> > >>> (this has been confirmed by Hans). The question is how to move forward\n> > >>\n> > >> Yes, presets are not used although I am finding examples of colorspace\n> > >> fields using\n> > >>\n> > >> V4L2_MAP_*_DEFAULT (clrspace)\n> > >>\n> > >> macros to determine the specific values for the field (in the case where\n> > >> user-defined _DEFAULT is coming in from application's side)\n> > >\n> > > I've had a quick look at the mainline kernel, and this is what I found:\n> > >\n> > > Drivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set\n> > > the default values of xfer_func, ycbcr_enc and quantization (I believe\n> > > those are fine):\n> > >\n> > > - imx214\n> > > - imx219\n> > > - st-mipid02\n> > > - imx-mipi-csi\n> > > - imx8mq-mipi-csi2\n> > >\n> > > Drivers that accept colorspace only (without honouring the\n> > > V4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode\n> > > the xfer_func, ycbcr_enc and quantization to corresponding defaults:\n> > >\n> > > - ov5640\n> > > - ov5648\n> > > - ov7251\n> > > - ov8865\n> >\n> > At least these 4 drivers just ignore what userspace sets and set the\n> > colorimetry fields themselves. So I'm not sure what you mean with\n> > 'accept colorspace only'. I haven't looked at the next 4.\n> >\n> > > - imx-pxp\n> > > - camss-video\n> > > - rcar-vin\n> > > - sun4i-csi\n> > >\n> > > Drivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and\n> > > quantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT\n> > > (without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):\n> > >\n> > > - dw100\n> >\n> > That's a m2m device, they work differently and copy the colorimetry\n> > information from the output format to the capture format (unless it is\n> > a CSC converter).\n> >\n> > > - rcar-fdp1\n> > > - imx6-media\n> > > - imx7-media-csi\n> > >\n> > >> (Side question: Isn't this DEFAULT behaviour close to selecting a\n> > >> preset, based on primaries?)\n> > >\n> > > If you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,\n> > > and xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the\n> > > documented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is\n> > > that colorspace should be modified, and xfer_func, ycbcr_enc and\n> > > quantization should keep their current value. This is thus different\n> > > than using colorspace as a preset.\n> >\n> > Correct.\n> >\n> > >\n> > >>> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> > >>> difficult, but I expect non-libcamera users to be surprised. I don't\n> > >>\n> > >> Why would they be surprised. AFAICS, unless applications start using [1]\n> > >> [2] they'll have the same behaviour no ?\n> > >\n> > > If we implement support for [1] and/or [2] in the driver, and ignore the\n> > > colour space fields in S_FMT when the flags are not set, it could cause\n> > > issues with applications that attempt to configure the colour space but\n> > > don't set the flag. Those are not V4L2-compliant, but they work today.\n> >\n> > Right.\n> >\n> > Regards,\n> >\n> > Hans\n> >\n> > >\n> > >>> expect many such users though, so if that's fine, the only possible\n> > >>> issue would be synchronizing the changes in the driver and in libcamera.\n> > >>>\n> > >>> Other options may be possible. I don't think updating the V4L2 API\n> > >>> specification to consider the colorspace field as a preset will be well\n> > >>> received upstream (and I don't think I would like that much myself to be\n> > >>> honest). Keeping a non-compliant implementation in the bcm2835-isp\n> > >>> driver with specific handling in libcamera could be done, but I don't\n> > >>> think it would be upstreamable.\n> > >>>\n> > >>> ---\n> > >>> src/libcamera/v4l2_device.cpp | 14 --------------\n> > >>> 1 file changed, 14 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > >>> index c17b323f8af0..e60a9ae217de 100644\n> > >>> --- a/src/libcamera/v4l2_device.cpp\n> > >>> +++ b/src/libcamera/v4l2_device.cpp\n> > >>> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> > >>> if (!colorSpace)\n> > >>> return 0;\n> > >>>\n> > >>> - auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> > >>> - [&colorSpace](const auto &item) {\n> > >>> - return colorSpace == item.first;\n> > >>> - });\n> > >>> - if (itColor != colorSpaceToV4l2.end()) {\n> > >>> - v4l2Format.colorspace = itColor->second;\n> > >>> - /* Leaving all the other fields as \"default\" should be fine. */\n> > >>> - return 0;\n> > >>> - }\n> > >>> -\n> > >>> - /*\n> > >>> - * If the colorSpace doesn't precisely match a standard color space,\n> > >>> - * then we must choose a V4L2 colorspace with matching primaries.\n> > >>> - */\n> > >>> int ret = 0;\n> > >>>\n> > >>> auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97A48BDB16\n\tfor ;\n\tTue, 25 Oct 2022 11:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52A7562F3B;\n\tTue, 25 Oct 2022 13:15:35 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F05AF61F4C\n\tfor ;\n\tTue, 25 Oct 2022 13:15:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 43674E0007;\n\tTue, 25 Oct 2022 11:15:33 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666696535;\n\tbh=mKqTDgePoOB+s7rchmAvjeuRNrLhz4mQqJgHMP85YMw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=hjiLfzXA9LtyiL8Rmzy8c3yyxE2lDDZNhqFzttn+PGkMdd1m6DaF0vAoVmURwY+xG\n\t9hMH01Qbok5JktmRpnNbt54l0P0qvtrVyyzj/P26MwVQ/WiNy0zrm1yYR6Wh6q3fgR\n\t9IKPVb2EeFy2Ll6dDy++rvdz2A5wyPOAwmrVzoizUa8k+bKyuAn2eW3ZO+oQ8uyvWx\n\t+t4Eu48TK6W0Zup5sO4kAluQ33CVLoLEAKGgy804o5OVcfHCLON9HCgj06GGsblKHk\n\t2qjKJfv1j4Ma/KiCfknx+jh93FF8qvJ810Nq4cLyRdaUYx7OvoBBL5YT+0kL5mN2Tc\n\tzYaQh2aXt7ugg==","Date":"Tue, 25 Oct 2022 13:15:31 +0200","To":"David Plowman ","Message-ID":"<20221025111531.peuq6ucvpawgu25o@uno.localdomain>","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Hans Verkuil ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25578,"web_url":"https://patchwork.libcamera.org/comment/25578/","msgid":"","date":"2022-10-25T12:35:36","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Laurent\n\nOn Tue, 25 Oct 2022 at 02:22, Laurent Pinchart via libcamera-devel\n wrote:\n>\n> When setting a colorspace on a V4L2 video capture device or a subdev\n> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n> and quantization fields as four independent values. Any field set to\n> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n> configuration for that field. This behaviour, while not necessarily well\n> known, is clearly documented in [1] and [2], and is implemented by the\n> rkisp1 driver.\n>\n> The V4L2Device::fromColorSpace() function, when converting from a\n> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n> attempts to find a match for a colorspace preset. If found, it only sets\n> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n> specification, and prevents setting the transfer function, YCbCr\n> encoding and quantization on at least rkisp1.\n>\n> Fix this issue by dropping the preset lookup, and set all four\n> colorspace-related fields explicitly.\n>\n> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n>\n> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n> certainly won't blame it, as I implemented the same non-compliant\n> behaviour in the rkisp1 driver initially, before realizing that V4L2\n> doesn't consider the colorspace field as a preset in set format calls\n> (this has been confirmed by Hans). The question is how to move forward\n> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n> difficult, but I expect non-libcamera users to be surprised. I don't\n> expect many such users though, so if that's fine, the only possible\n> issue would be synchronizing the changes in the driver and in libcamera.\n\nI don't expect there are any other significant users of bcm2835-isp at\nthis point, so I have no issues in fixing it once I understand the\ncorrect behaviour.\nThere are a limited number of combinations currently supported within\nthe firmware, and I wouldn't want to extend that unless really pushed.\nThat means that there will be a fair number of cases where the format\nrequested gets altered in s_format.\n\nThere will be other users of bcm2835-codec which wraps the codecs, and\nsimple 1-in, 1-out ISP for resize/format convert. Largely that will be\nused via FFmpeg or GStreamer though, so I wonder if those two\napplications follow the spec....\nFixing that driver may have bigger implications. Once bcm2835-isp has\nbeen reviewed and upstreamed, then I'll look at that one - there are\nlikely to be a fair number of common quirks.\n\n Dave\n\n> Other options may be possible. I don't think updating the V4L2 API\n> specification to consider the colorspace field as a preset will be well\n> received upstream (and I don't think I would like that much myself to be\n> honest). Keeping a non-compliant implementation in the bcm2835-isp\n> driver with specific handling in libcamera could be done, but I don't\n> think it would be upstreamable.\n>\n> ---\n> src/libcamera/v4l2_device.cpp | 14 --------------\n> 1 file changed, 14 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c17b323f8af0..e60a9ae217de 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n> if (!colorSpace)\n> return 0;\n>\n> - auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> - [&colorSpace](const auto &item) {\n> - return colorSpace == item.first;\n> - });\n> - if (itColor != colorSpaceToV4l2.end()) {\n> - v4l2Format.colorspace = itColor->second;\n> - /* Leaving all the other fields as \"default\" should be fine. */\n> - return 0;\n> - }\n> -\n> - /*\n> - * If the colorSpace doesn't precisely match a standard color space,\n> - * then we must choose a V4L2 colorspace with matching primaries.\n> - */\n> int ret = 0;\n>\n> auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 712B2BD16B\n\tfor ;\n\tTue, 25 Oct 2022 12:35:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B91EA62F3C;\n\tTue, 25 Oct 2022 14:35:54 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6425B61F4C\n\tfor ;\n\tTue, 25 Oct 2022 14:35:52 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id sc25so12008341ejc.12\n\tfor ;\n\tTue, 25 Oct 2022 05:35:52 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666701354;\n\tbh=iYqmbRqpnat9sZigZZoYvtO8McApx5XdNvbH+CdgbaU=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=o4zX0cQG8UNPwMD5KmnfNWLpBexjgLUZtqySzQsf7xwJnUzWLLK1DeGCRbEUSdfg5\n\tVKJtlwNTQuKbVBl+H+T3jVMEc69FH/6x2O6S7IOS7F+WGT/X8tLKiXGTJWinlqIq3a\n\tBltmOUH0i1wxFY0xj+nJp0KK7/SxFxGIinrwcxuQ0Hto99azOafJYld9yNSWinCHyV\n\tnXGquVxYhVHQzy7dIKnF6k5Zn0v5ZaUutkvd2VsH/Y6CFsOBqjo0jB52Az59/RJJUc\n\tcF+TsgrEpp544N5pkQTPLFyj7nqyhV/Qd17DlZOkHn1dpYHBILSCL4O2FhFJW2enYX\n\tOYYKonY0VufcQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ZwtKAktXafUSjF2lTO0r2vBSbgB4oVc9GMLvjk82XqA=;\n\tb=r6WRQ0hlDJ/cEtvuSe3YCT3yQ8SLQ2ZhRKOQ947uZRMSiFKSizPQxTunK42AKj0mKf\n\tiDwOURXUzQrepJosdyMAIIiiJDOM3mGVCEK8KeTY5BNC+jMm2HeTrF65NuxLmn9yaA7o\n\tfczdAv35qCQcfXB4JUcWm8WBlTezVhcVlQzNjTs7pcni70tzh4Vslw7fQNAfc9i9epzh\n\tt8fZjtf9gfTn79BToPW7HA2n+m24I0FYJN9BON4B/yAxc8jxnqLUhwPNWRV1ZkgI25zD\n\tScchzAa8qS33ut3UiwgttqBcSeMO4XyYi9c/IptdYpEPEDcK+FeEqldsbN2gMnpMH3TY\n\tk1/g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"r6WRQ0hl\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=ZwtKAktXafUSjF2lTO0r2vBSbgB4oVc9GMLvjk82XqA=;\n\tb=SiISkafCyVWzBxcbclGUeHfq6zd/jUTwJqLI+YbSsy1q3B3TUHRVyWmI0r8jx4gZxM\n\tAicK8bUZCIho0yGw0HyVAuAf3IimMpUfW6v2GcE90LyaXDAg6jXnBwNr9JcG6aHEQwgq\n\t6NRNZpNphVlMvBRGJQ/KlHP+4yoIBPLuet22d29mcLcXAqTUPNeeqQ4ptrmtjylVyMU8\n\tbLp/S1EiXijlnkElOKIv+8QjhEHma85IMRNe1H4o7HvU/HMIUvqwQnj/fi85y+Sj/hxq\n\ttYZBvxd0NiEF4Hn/dCsAfQ4qOM25OB0dkgnzQXtIi5O360tLSU2rLak+2mr9GQ1/F8+h\n\tJbfg==","X-Gm-Message-State":"ACrzQf2bb/m+0ynEivcoIYOTfd/n4tKd9HrDQNiAQMeJILi9DAn4FeTc\n\tsf8X737i9I3q/jp8KhHRv0Ingkx6r/QeiZ5H/jCtDGJo9UA=","X-Google-Smtp-Source":"AMsMyM5Gn2HzurMSNyvn1tXpD2oisP9i1pjcMKLzX9j1+WEsyV07LMYcNfhWBlnB+l4kXvXliWCLl7w7bZmsvv5PB7w=","X-Received":"by 2002:a17:907:1c98:b0:78d:3b08:33ef with SMTP id\n\tnb24-20020a1709071c9800b0078d3b0833efmr33239629ejc.175.1666701351854;\n\tTue, 25 Oct 2022 05:35:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>","Date":"Tue, 25 Oct 2022 13:35:36 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25579,"web_url":"https://patchwork.libcamera.org/comment/25579/","msgid":"<34393297-689c-056d-22f2-88b3e1753b6a@ideasonboard.com>","date":"2022-10-25T12:40:51","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent, Hans,\n\nOn 10/25/22 3:21 PM, Hans Verkuil wrote:\n> On 10/25/22 11:25, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> (CC'ing Hans)\n>>\n>> On Tue, Oct 25, 2022 at 12:28:52PM +0530, Umang Jain wrote:\n>>> On 10/25/22 6:51 AM, Laurent Pinchart via libcamera-devel wrote:\n>>>> When setting a colorspace on a V4L2 video capture device or a subdev\n>>>> source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc\n>>>> and quantization fields as four independent values. Any field set to\n>>>> V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current\n>>>> configuration for that field. This behaviour, while not necessarily well\n>>>> known, is clearly documented in [1] and [2], and is implemented by the\n>>>> rkisp1 driver.\n>>>>\n>>>> The V4L2Device::fromColorSpace() function, when converting from a\n>>>> libcamera ColorSpace to the V4L2 format colorspace-related fields, first\n>>>> attempts to find a match for a colorspace preset. If found, it only sets\n>>>> the colorspace field, and leaves the xfer_func, ycbcr_enc and\n>>>> quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2\n>>>> specification, and prevents setting the transfer function, YCbCr\n>>>> encoding and quantization on at least rkisp1.\n>>>>\n>>>> Fix this issue by dropping the preset lookup, and set all four\n>>>> colorspace-related fields explicitly.\n>>>>\n>>>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2\n>>>> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2\n>>> Neither of these flags are supported widely in the kernel by the\n>>> drivers, so that's something to think about as well\n>> Yes, I was also surprised to realize this. I learnt about those flags\n>> recently (when you added support for them in commit f094c2922ad57 :-)),\n>> and previously thought that the driver would always honour the colour\n>> space requested by userspace (provided the hardware supports that of\n>> course).\n>>\n>> We do set the flag unconditionally, which shouldn't hurt, but the fact\n>> that only one driver implements V4L2_MBUS_FRAMEFMT_SET_CSC (rkisp1) and\n>> another one V4L2_PIX_FMT_FLAG_SET_CSC (vivid) probably means that the\n>> spec isn't widely followed. Hans, would this mean that a check is\n>> missing in v4l2-compliance ?\n> Correct, there is no test for this. Very few drivers support this because\n> it is rarely needed (so no pressure on driver developers to support this\n> feature) and because there are not many people who understand how to do\n> this, i.e. there is a learning curve involved.\n>\n> Also, historically when V4L2 started none of the capture devices had\n> support for CSC at all, so support for this was never considered.\n>\n>>>> Signed-off-by: Laurent Pinchart \n>>>> ---\n>>>> I expect this change to cause breakages on Raspberry Pi, hence the RFC.\n>>>>\n>>>> The bcm2835-isp driver doesn't comply with the V4L2 API specification. I\n>>>> certainly won't blame it, as I implemented the same non-compliant\n>>>> behaviour in the rkisp1 driver initially, before realizing that V4L2\n>>>> doesn't consider the colorspace field as a preset in set format calls\n>>>> (this has been confirmed by Hans). The question is how to move forward\n>>> Yes, presets are not used although I am finding examples of colorspace\n>>> fields using\n>>>\n>>> V4L2_MAP_*_DEFAULT (clrspace)\n>>>\n>>> macros to determine the specific values for the field (in the case where\n>>> user-defined _DEFAULT is coming in from application's side)\n>> I've had a quick look at the mainline kernel, and this is what I found:\n>>\n>> Drivers that use V4L2_MAP_*_DEFAULT at initialization time only, to set\n>> the default values of xfer_func, ycbcr_enc and quantization (I believe\n>> those are fine):\n>>\n>> - imx214\n>> - imx219\n>> - st-mipid02\n>> - imx-mipi-csi\n>> - imx8mq-mipi-csi2\n>>\n>> Drivers that accept colorspace only (without honouring the\n>> V4L2_MBUS_FRAMEFMT_SET_CSC flag), and use V4L2_MAP_*_DEFAULT to hardcode\n>> the xfer_func, ycbcr_enc and quantization to corresponding defaults:\n>>\n>> - ov5640\n>> - ov5648\n>> - ov7251\n>> - ov8865\n> At least these 4 drivers just ignore what userspace sets and set the\n> colorimetry fields themselves. So I'm not sure what you mean with\n> 'accept colorspace only'. I haven't looked at the next 4.\n\nI think what Laurent meant is, these driver only accepts the colorspace \nfield /only/ and the rest fields (xfer,  ycbcr_enc and quantization) are \nselected based on the pass-in colorspace field (using the \nV4L2_MAP_*_DEFAULT macro).\n\nSo, these 4 drivers 'accept colorspace only' and ignore the rest fields \n(xfer, ycbcr_enc and quantization)\n>\n>> - imx-pxp\n>> - camss-video\n>> - rcar-vin\n>> - sun4i-csi\n>>\n>> Drivers that use V4L2_MAP_*_DEFAULT to set xfer_func, ycbcr_enc and\n>> quantization in S_FMT when they are set by userspace to V4L2_*_DEFAULT\n>> (without honouring the V4L2_MBUS_FRAMEFMT_SET_CSC flag):\n>>\n>> - dw100\n> That's a m2m device, they work differently and copy the colorimetry\n> information from the output format to the capture format (unless it is\n> a CSC converter).\n>\n>> - rcar-fdp1\n>> - imx6-media\n>> - imx7-media-csi\n>>\n>>> (Side question: Isn't this DEFAULT behaviour close to selecting a\n>>> preset, based on primaries?)\n>> If you set colorspace to a value different than V4L2_COLORSPACE_DEFAULT,\n>> and xfer_func, ycbcr_enc and quantization to V4L2_*_DEFAULT, the\n>> documented behaviour (assuming you also set the V4L2_*_SET_CSC flag) is\n>> that colorspace should be modified, and xfer_func, ycbcr_enc and\n>> quantization should keep their current value. This is thus different\n>> than using colorspace as a preset.\n> Correct.\n\nMakes sense to me now.\n\n>\n>>>> here. Fixing the driver to make it compliant with V4L2 wouldn't be\n>>>> difficult, but I expect non-libcamera users to be surprised. I don't\n>>> Why would they be surprised. AFAICS, unless applications start using [1]\n>>> [2] they'll have the same behaviour no ?\n>> If we implement support for [1] and/or [2] in the driver, and ignore the\n>> colour space fields in S_FMT when the flags are not set, it could cause\n>> issues with applications that attempt to configure the colour space but\n>> don't set the flag. Those are not V4L2-compliant, but they work today.\n\nI am sure you have handled such situation previously as well and put \nusage on a corrective path over time ? What are the options here - other \nthan introducing a warning on non v4l2-compliant  usage of \ncolorspace/flags by non-libcamera users.\n\n> Right.\n>\n> Regards,\n>\n> \tHans\n>\n>>>> expect many such users though, so if that's fine, the only possible\n>>>> issue would be synchronizing the changes in the driver and in libcamera.\n>>>>\n>>>> Other options may be possible. I don't think updating the V4L2 API\n>>>> specification to consider the colorspace field as a preset will be well\n>>>> received upstream (and I don't think I would like that much myself to be\n>>>> honest). Keeping a non-compliant implementation in the bcm2835-isp\n>>>> driver with specific handling in libcamera could be done, but I don't\n>>>> think it would be upstreamable.\n>>>>\n>>>> ---\n>>>> src/libcamera/v4l2_device.cpp | 14 --------------\n>>>> 1 file changed, 14 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>>> index c17b323f8af0..e60a9ae217de 100644\n>>>> --- a/src/libcamera/v4l2_device.cpp\n>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>> @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v\n>>>> \tif (!colorSpace)\n>>>> \t\treturn 0;\n>>>> \n>>>> -\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n>>>> -\t\t\t\t [&colorSpace](const auto &item) {\n>>>> -\t\t\t\t\t return colorSpace == item.first;\n>>>> -\t\t\t\t });\n>>>> -\tif (itColor != colorSpaceToV4l2.end()) {\n>>>> -\t\tv4l2Format.colorspace = itColor->second;\n>>>> -\t\t/* Leaving all the other fields as \"default\" should be fine. */\n>>>> -\t\treturn 0;\n>>>> -\t}\n>>>> -\n>>>> -\t/*\n>>>> -\t * If the colorSpace doesn't precisely match a standard color space,\n>>>> -\t * then we must choose a V4L2 colorspace with matching primaries.\n>>>> -\t */\n>>>> \tint ret = 0;\n>>>> \n>>>> \tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7EC3BDB16\n\tfor ;\n\tTue, 25 Oct 2022 12:41:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D789662F3D;\n\tTue, 25 Oct 2022 14:40:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC01A61F4C\n\tfor ;\n\tTue, 25 Oct 2022 14:40:58 +0200 (CEST)","from [192.168.1.103] (unknown [103.238.109.19])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12A508BF;\n\tTue, 25 Oct 2022 14:40:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666701659;\n\tbh=qBh7SO3+FTuLOPzUVfkbQdaVLAM8R1l12EenN/bpGfs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=skQR/e8+qTvn5W8de2BrC2rdWULW8gIUM9ut5OaTDUzCdASzjnGAZp2hHpzFJ0ePO\n\tuORzNzDxXyZn7VmrPz53/sD210lgE/pRB57mz4KeqgynGxHowH/PYq5W/bmFi3JLlT\n\tMSRloOjrWP2LKdI/INpMGuUziVvLra+bsPxJhaBTLIhzogRzp+SdeKdiJgGPjkylMF\n\tys+1rZd400eFiJQUWcVA7KEWOn32wo+TjWTT5SSxUbEWf0D1OI0qVY1P29P9TDKm22\n\twEr2SVmfJbgJX1y75h4HT20BDg3JTlAAMVTE2Cms+kG068+KAlcl28vD5U4OzAa0Rw\n\tiR64D7Pb+S1yw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666701658;\n\tbh=qBh7SO3+FTuLOPzUVfkbQdaVLAM8R1l12EenN/bpGfs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=K6GrJ4ytPf+iwWi6OM5FcWyB08Sk1KXttKysrd3jYWUfnk1QrqAq3suvkzGMRaZ1h\n\tIBR5w78LdhWi/15n/cnqpiuMA0bTnaOsGbRQUGz4HXQg+YlaRfNk++U7UGWsyHXVV4\n\t+WDS5CWf5UiZ2xtlQF52NOLbwgWFtAXiuLZrpFCc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K6GrJ4yt\"; dkim-atps=neutral","Message-ID":"<34393297-689c-056d-22f2-88b3e1753b6a@ideasonboard.com>","Date":"Tue, 25 Oct 2022 18:10:51 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","Content-Language":"en-US","To":"Hans Verkuil ,\n\tLaurent Pinchart ","References":"<20221025012131.12943-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","In-Reply-To":"<3018905f-19cf-40dc-a145-0b85590a15f2@xs4all.nl>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always\n\tset all four V4L2 colorspace fields","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]