[{"id":21138,"web_url":"https://patchwork.libcamera.org/comment/21138/","msgid":"","date":"2021-11-23T14:49:10","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nThe patch looks good to me overall, just one comment\n\nOn 11/18/21 8:49 PM, David Plowman wrote:\n> These methods are added to the base V4L2Device class so that they can\n> be shared both by the video device class and subdevices.\n>\n> With the ColorSpace class, the color space and related other fields\n> are stored together, corresponding to a number of fields in the\n> various different V4L2 format structures. Template methods are\n> therefore a convenient implementation, and we must explicitly\n> instantiate the templates that will be needed.\n>\n> Note that unset color spaces are converted to requests for the\n> device's \"default\" color space.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_device.h | 7 +\n> src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> 2 files changed, 197 insertions(+)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index f21bc370..89ff6120 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -44,6 +45,12 @@ public:\n> \n> \tvoid updateControlInfo();\n> \n> +\ttemplate\n> +\tstatic std::optional toColorSpace(const T &v4l2Format);\n> +\n> +\ttemplate\n> +\tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 9c783c9c..4d6985aa 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -16,6 +16,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> \tframeStart.emit(event.u.frame_sync.frame_sequence);\n> }\n> \n> +static const std::map v4l2ToColorSpace = {\n> +\t{ V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> +\t{ V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> +\t{ V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> +\t{ V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> +\t{ V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> +\t{ V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> +};\n> +\n> +static const std::map v4l2ToYcbcrEncoding = {\n> +\t{ V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> +\t{ V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> +\t{ V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> +};\n> +\n> +static const std::map v4l2ToTransferFunction = {\n> +\t{ V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> +\t{ V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> +\t{ V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> +};\n> +\n> +static const std::map v4l2ToRange = {\n> +\t{ V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> +\t{ V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> +};\n> +\n> +static const std::vector> colorSpaceToV4l2 = {\n> +\t{ ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> +\t{ ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> +\t{ ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> +\t{ ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> +\t{ ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> +\t{ ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> +};\n> +\n> +static const std::map primariesToV4l2 = {\n> +\t{ ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> +\t{ ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> +\t{ ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> +\t{ ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> +};\n> +\n> +static const std::map ycbcrEncodingToV4l2 = {\n> +\t{ ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> +\t{ ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> +\t{ ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> +};\n> +\n> +static const std::map transferFunctionToV4l2 = {\n> +\t{ ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> +\t{ ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> +\t{ ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> +};\n> +\n> +static const std::map rangeToV4l2 = {\n> +\t{ ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> +\t{ ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> +};\n> +\n> +/**\n> + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> + * \\param[in] v4l2Format A V4L2 format containing color space information\n> + *\n> + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> + * V4L2 format structure are converted to a corresponding ColorSpace.\n> + *\n> + * If any V4L2 fields are not recognised then we return an \"unset\"\n> + * color space.\n> + *\n> + * \\return The ColorSpace corresponding to the input V4L2 format\n> + */\n> +template\n> +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> +{\n> +\tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> +\tif (itColor == v4l2ToColorSpace.end())\n> +\t\treturn std::nullopt;\n> +\n> +\t/* This sets all the color space fields to the correct \"default\" values. */\n> +\tColorSpace colorSpace = itColor->second;\n> +\n> +\tif (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> +\t\tauto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> +\t\tif (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> +\t}\n> +\n> +\tif (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> +\t\tauto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> +\t\tif (itTransfer == v4l2ToTransferFunction.end())\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tcolorSpace.transferFunction = itTransfer->second;\n> +\t}\n> +\n> +\tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> +\t\tauto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> +\t\tif (itRange == v4l2ToRange.end())\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tcolorSpace.range = itRange->second;\n> +\t}\n> +\n> +\treturn colorSpace;\n> +}\n> +\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> +\n> +/**\n> + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> + * \\param[in] colorSpace The ColorSpace to be converted\n> + * \\param[out] v4l2Format A V4L2 format containing color space information\n> + *\n> + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> + *\n> + * An error is returned if any of the V4L2 fields do not support the\n> + * value given in the ColorSpace. Such fields are set to the V4L2\n> + * \"default\" values, but all other fields are still filled in where\n> + * possible.\n> + *\n> + * If the color space is completely unset, \"default\" V4L2 values are used\n> + * everywhere, so a driver would then choose its preferred color space.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n\nI think we are not returning any error codes (ergo errno here) here. \nAll  I see -1 returned on error paths.\n\n\n> + */\n> +template\n> +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> +{\n> +\tint ret = 0;\n> +\n> +\tv4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> +\tv4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> +\tv4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> +\tv4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> +\n> +\tif (!colorSpace)\n> +\t\treturn ret;\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 ret;\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> +\tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> +\tif (itPrimaries != primariesToV4l2.end())\n> +\t\tv4l2Format.colorspace = itPrimaries->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> +\tif (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> +\t\tv4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> +\tif (itTransfer != transferFunctionToV4l2.end())\n> +\t\tv4l2Format.xfer_func = itTransfer->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itRange = rangeToV4l2.find(colorSpace->range);\n> +\tif (itRange != rangeToV4l2.end())\n> +\t\tv4l2Format.quantization = itRange->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\treturn ret;\n> +}\n> +\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> +\n> } /* namespace libcamera */","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 73466BF415\n\tfor ;\n\tTue, 23 Nov 2021 14:49:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B645F60230;\n\tTue, 23 Nov 2021 15:49:19 +0100 (CET)","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 3892560121\n\tfor ;\n\tTue, 23 Nov 2021 15:49:18 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8EEB993;\n\tTue, 23 Nov 2021 15:49:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LyvhL99d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637678957;\n\tbh=kfVIQTGuadEaaFx0iSSC5BSk8XkHu8dTKzDzccZG0YA=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=LyvhL99dIhi3tYtkMkzzvk/TlqZcIq5SWOkfB0yQHxHcAH+H5fN8XJDJmAP+Pv5Rl\n\tcDuQxXf2tC9GMlJ1M+xoZcAKxiPJ1jok/n8gNlEn0GSyQjhOa0ayUkT3eAbWEm4Wbp\n\tsWaeJYDlWADq23+9dXa6PL89PdXTwCNCIiohAFVE=","To":"David Plowman ,\n\tlaurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com,\n\thverkuil-cisco@xs4all.nl, tfiga@google.com, jacopo@jmondi.org,\n\tnaush@raspberrypi.com, libcamera-devel@lists.libcamera.org","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 23 Nov 2021 20:19:10 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211118151933.15627-4-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21190,"web_url":"https://patchwork.libcamera.org/comment/21190/","msgid":"","date":"2021-11-24T11:08:23","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Umang\n\nThanks for reviewing this!\n\nOn Tue, 23 Nov 2021 at 14:49, Umang Jain wrote:\n>\n> Hi David,\n>\n> The patch looks good to me overall, just one comment\n>\n> On 11/18/21 8:49 PM, David Plowman wrote:\n> > These methods are added to the base V4L2Device class so that they can\n> > be shared both by the video device class and subdevices.\n> >\n> > With the ColorSpace class, the color space and related other fields\n> > are stored together, corresponding to a number of fields in the\n> > various different V4L2 format structures. Template methods are\n> > therefore a convenient implementation, and we must explicitly\n> > instantiate the templates that will be needed.\n> >\n> > Note that unset color spaces are converted to requests for the\n> > device's \"default\" color space.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 7 +\n> > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > 2 files changed, 197 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index f21bc370..89ff6120 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -44,6 +45,12 @@ public:\n> >\n> > void updateControlInfo();\n> >\n> > + template\n> > + static std::optional toColorSpace(const T &v4l2Format);\n> > +\n> > + template\n> > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > +\n> > protected:\n> > V4L2Device(const std::string &deviceNode);\n> > ~V4L2Device();\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 9c783c9c..4d6985aa 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -16,6 +16,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > }\n> >\n> > +static const std::map v4l2ToColorSpace = {\n> > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > +};\n> > +\n> > +static const std::map v4l2ToYcbcrEncoding = {\n> > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > +};\n> > +\n> > +static const std::map v4l2ToTransferFunction = {\n> > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > +};\n> > +\n> > +static const std::map v4l2ToRange = {\n> > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > +};\n> > +\n> > +static const std::vector> colorSpaceToV4l2 = {\n> > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > +};\n> > +\n> > +static const std::map primariesToV4l2 = {\n> > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > +};\n> > +\n> > +static const std::map ycbcrEncodingToV4l2 = {\n> > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > +};\n> > +\n> > +static const std::map transferFunctionToV4l2 = {\n> > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > +};\n> > +\n> > +static const std::map rangeToV4l2 = {\n> > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > +};\n> > +\n> > +/**\n> > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > + *\n> > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > + *\n> > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > + * color space.\n> > + *\n> > + * \\return The ColorSpace corresponding to the input V4L2 format\n> > + */\n> > +template\n> > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > +{\n> > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > + if (itColor == v4l2ToColorSpace.end())\n> > + return std::nullopt;\n> > +\n> > + /* This sets all the color space fields to the correct \"default\" values. */\n> > + ColorSpace colorSpace = itColor->second;\n> > +\n> > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > + return std::nullopt;\n> > +\n> > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > + }\n> > +\n> > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > + if (itTransfer == v4l2ToTransferFunction.end())\n> > + return std::nullopt;\n> > +\n> > + colorSpace.transferFunction = itTransfer->second;\n> > + }\n> > +\n> > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > + if (itRange == v4l2ToRange.end())\n> > + return std::nullopt;\n> > +\n> > + colorSpace.range = itRange->second;\n> > + }\n> > +\n> > + return colorSpace;\n> > +}\n> > +\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > +\n> > +/**\n> > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > + * \\param[in] colorSpace The ColorSpace to be converted\n> > + * \\param[out] v4l2Format A V4L2 format containing color space information\n> > + *\n> > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > + *\n> > + * An error is returned if any of the V4L2 fields do not support the\n> > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > + * \"default\" values, but all other fields are still filled in where\n> > + * possible.\n> > + *\n> > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > + * everywhere, so a driver would then choose its preferred color space.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n>\n> I think we are not returning any error codes (ergo errno here) here.\n> All I see -1 returned on error paths.\n\nJust to clarify, are you simply implying a change to the documentation\nhere, i.e. \"\\return 0 on success or -1 otherwise\" ?\n\nThanks!\nDavid\n\n>\n>\n> > + */\n> > +template\n> > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > +{\n> > + int ret = 0;\n> > +\n> > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > +\n> > + if (!colorSpace)\n> > + return ret;\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 ret;\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> > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > + if (itPrimaries != primariesToV4l2.end())\n> > + v4l2Format.colorspace = itPrimaries->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > + if (itTransfer != transferFunctionToV4l2.end())\n> > + v4l2Format.xfer_func = itTransfer->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > + if (itRange != rangeToV4l2.end())\n> > + v4l2Format.quantization = itRange->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > +\n> > } /* namespace libcamera */","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 92EB3BF415\n\tfor ;\n\tWed, 24 Nov 2021 11:08:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FE5A60371;\n\tWed, 24 Nov 2021 12:08:37 +0100 (CET)","from mail-wm1-x330.google.com (mail-wm1-x330.google.com\n\t[IPv6:2a00:1450:4864:20::330])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F04FD60128\n\tfor ;\n\tWed, 24 Nov 2021 12:08:34 +0100 (CET)","by mail-wm1-x330.google.com with SMTP id 137so1937955wma.1\n\tfor ;\n\tWed, 24 Nov 2021 03:08:34 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"YJAYP9qf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oO4C+5av53WSaUGl2/cl9hA7n1UTB1GlMEs8d69veeM=;\n\tb=YJAYP9qfTAvZAuOiBO6PfnsbrH/xa0zh/eLtxNdl/zF9CCQGfMXuMh5CgOkyGimI/z\n\tZG1R2atJeCpwLqyYm/vqP+/u+4JnfZlZuxyUbq7JmAxh7zVtikvsyu7JdAt+QDmoELFc\n\twlzDVXjrXASgT9GV+t2+LS7TBMsA2H3jZA7wFBP75JAZuf23R7cZDEDvJw9WJlu88dKq\n\tP8xSKb0K168Glv7r/X/DK/7phQBE5p3bOp6SHbE8fQpxtS0pL2obD7ZlVDpxiFX9c2i5\n\tWkoPBLyFz1cg26qA+UePDZ47EgcfWrMY6srRnH/crWZXhBBYlYigxgbV6N+Eksy79yP4\n\tAdAw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oO4C+5av53WSaUGl2/cl9hA7n1UTB1GlMEs8d69veeM=;\n\tb=lUNpaIEaiT9qvugG/BfuaOKfYbuL6cf3a9c/fOqpVuAiNDmEj7bL3PWwpmj26J0mk0\n\tf4XRxp3VCUZTiZ2gPrqMRvTLmuQlOZH3XT3Gat4rRWWUjx+nfM0BWONhov6l+pqdh6Vi\n\twCbNF6saRxbXtC3gyOL+N5i0j/PcbrFzWWwpILPf4zwmY2x7RBMlUXAtPJwwwP34g+kH\n\tAZXzJLZlLr9WjZQ2AAoaFMvAPmkZEjNHZs1gcBu5W58IGbjEzd0jOwHG//boEpPrOr6Q\n\thHYlF5q/wXdwcmwHCkXTQf2CdUIR5X53DxRJ1CUcUs4x7i/MyrdzmF+X3zANw1OgrdaD\n\tRssg==","X-Gm-Message-State":"AOAM531WUBlfmP19YWwQtuHQaDsW9p1KjLI/eWLIz1YOSmEihcVn0Wxs\n\tSiAmDgccs1IVgFNRaZUvrYHTbsENyEQaGUr7IExa6A==","X-Google-Smtp-Source":"ABdhPJyxsm3CclMnuGLKSvb3s9evQR7848LNi35ZJr8I7DjDPZsiNOMtCmK43WZbL1pwLAvwZYKQiXTVczjF59JgRd4=","X-Received":"by 2002:a1c:1d42:: with SMTP id\n\td63mr13734793wmd.184.1637752114533; \n\tWed, 24 Nov 2021 03:08:34 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 24 Nov 2021 11:08:23 +0000","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21198,"web_url":"https://patchwork.libcamera.org/comment/21198/","msgid":"<163775918398.3059017.1815417399230349498@Monstersaurus>","date":"2021-11-24T13:06:23","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-24 11:08:23)\n> Hi Umang\n> \n> Thanks for reviewing this!\n> \n> On Tue, 23 Nov 2021 at 14:49, Umang Jain wrote:\n> >\n> > Hi David,\n> >\n> > The patch looks good to me overall, just one comment\n> >\n> > On 11/18/21 8:49 PM, David Plowman wrote:\n> > > These methods are added to the base V4L2Device class so that they can\n> > > be shared both by the video device class and subdevices.\n> > >\n> > > With the ColorSpace class, the color space and related other fields\n> > > are stored together, corresponding to a number of fields in the\n> > > various different V4L2 format structures. Template methods are\n> > > therefore a convenient implementation, and we must explicitly\n> > > instantiate the templates that will be needed.\n> > >\n> > > Note that unset color spaces are converted to requests for the\n> > > device's \"default\" color space.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 7 +\n> > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > > 2 files changed, 197 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index f21bc370..89ff6120 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -44,6 +45,12 @@ public:\n> > >\n> > > void updateControlInfo();\n> > >\n> > > + template\n> > > + static std::optional toColorSpace(const T &v4l2Format);\n> > > +\n> > > + template\n> > > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > > +\n> > > protected:\n> > > V4L2Device(const std::string &deviceNode);\n> > > ~V4L2Device();\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 9c783c9c..4d6985aa 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -16,6 +16,8 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > #include \n> > > #include \n> > > #include \n> > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > > }\n> > >\n> > > +static const std::map v4l2ToColorSpace = {\n> > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToYcbcrEncoding = {\n> > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToTransferFunction = {\n> > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToRange = {\n> > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > > +};\n> > > +\n> > > +static const std::vector> colorSpaceToV4l2 = {\n> > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map primariesToV4l2 = {\n> > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map ycbcrEncodingToV4l2 = {\n> > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map transferFunctionToV4l2 = {\n> > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > +};\n> > > +\n> > > +static const std::map rangeToV4l2 = {\n> > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > > + *\n> > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > > + *\n> > > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > > + * color space.\n> > > + *\n> > > + * \\return The ColorSpace corresponding to the input V4L2 format\n> > > + */\n> > > +template\n> > > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > > +{\n> > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > > + if (itColor == v4l2ToColorSpace.end())\n> > > + return std::nullopt;\n> > > +\n> > > + /* This sets all the color space fields to the correct \"default\" values. */\n> > > + ColorSpace colorSpace = itColor->second;\n> > > +\n> > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > > + return std::nullopt;\n> > > +\n> > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > > + }\n> > > +\n> > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > > + if (itTransfer == v4l2ToTransferFunction.end())\n> > > + return std::nullopt;\n> > > +\n> > > + colorSpace.transferFunction = itTransfer->second;\n> > > + }\n> > > +\n> > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > > + if (itRange == v4l2ToRange.end())\n> > > + return std::nullopt;\n> > > +\n> > > + colorSpace.range = itRange->second;\n> > > + }\n> > > +\n> > > + return colorSpace;\n> > > +}\n> > > +\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > > +\n> > > +/**\n> > > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > > + * \\param[in] colorSpace The ColorSpace to be converted\n> > > + * \\param[out] v4l2Format A V4L2 format containing color space information\n> > > + *\n> > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > > + *\n> > > + * An error is returned if any of the V4L2 fields do not support the\n> > > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > > + * \"default\" values, but all other fields are still filled in where\n> > > + * possible.\n> > > + *\n> > > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > > + * everywhere, so a driver would then choose its preferred color space.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> >\n> > I think we are not returning any error codes (ergo errno here) here.\n> > All I see -1 returned on error paths.\n> \n> Just to clarify, are you simply implying a change to the documentation\n> here, i.e. \"\\return 0 on success or -1 otherwise\" ?\n\nGenerally thoughout libcamera we use errno values to return errors.\n\nCan we change the -1's to something more explicit like -EINVAL? or\n-ENOENT? or something that conveys what happened in some form?\n\nWe often try to use strerror() to print 'more helpful' error reports.\n\nIn this instance, it means we tried to construct a V4L2 colourspace from\nthe defined types but couldn't make an exact match ... so I think\n-EINVAL works?\n\nAt which point, the documentation could say:\n\n\\return 0 on success or a negative error code otherwise\n\\return -EINVAL A valid colour space could not be constructed\n\n\nOr something along those lines.\n\n--\nKieran\n\n\n> \n> Thanks!\n> David\n> \n> >\n> >\n> > > + */\n> > > +template\n> > > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > > +{\n> > > + int ret = 0;\n> > > +\n> > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > > +\n> > > + if (!colorSpace)\n> > > + return ret;\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 ret;\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> > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > > + if (itPrimaries != primariesToV4l2.end())\n> > > + v4l2Format.colorspace = itPrimaries->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > > + if (itTransfer != transferFunctionToV4l2.end())\n> > > + v4l2Format.xfer_func = itTransfer->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > > + if (itRange != rangeToV4l2.end())\n> > > + v4l2Format.quantization = itRange->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + return ret;\n> > > +}\n> > > +\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > > +\n> > > } /* namespace libcamera */","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 A23B1BDB13\n\tfor ;\n\tWed, 24 Nov 2021 13:06:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 086D66038A;\n\tWed, 24 Nov 2021 14:06:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43A6160128\n\tfor ;\n\tWed, 24 Nov 2021 14:06:27 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B10CE993;\n\tWed, 24 Nov 2021 14:06:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sCcvfjLj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637759186;\n\tbh=gKFbKO0h9jdDitiW4Go8V4V1QwDdii2nYmq/mrUjFBs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sCcvfjLj+GvlKhDN7MFofyqfYLWlZCRZtBgqsqUbsas7rrG0VtkobknjSCxeKg+fl\n\tkMEChAuwkPk7kAb6qnqUq5tDinBI6LCywNaWeMkAae0cP8e8pWBAK7kNzzdJUKsA0J\n\tShTc69VgcbCFexd6e0dJEbduuhRAK/tn8eiKILZE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tUmang Jain ","Date":"Wed, 24 Nov 2021 13:06:23 +0000","Message-ID":"<163775918398.3059017.1815417399230349498@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21202,"web_url":"https://patchwork.libcamera.org/comment/21202/","msgid":"<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>","date":"2021-11-24T16:06:12","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n\nOn Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote:\n> These methods are added to the base V4L2Device class so that they can\n> be shared both by the video device class and subdevices.\n>\n> With the ColorSpace class, the color space and related other fields\n> are stored together, corresponding to a number of fields in the\n> various different V4L2 format structures. Template methods are\n> therefore a convenient implementation, and we must explicitly\n> instantiate the templates that will be needed.\n>\n> Note that unset color spaces are converted to requests for the\n> device's \"default\" color space.\n\n You seem to use \"unset\" in place of default in this version.\n\n Also, this is a nice explanation about the implementation but\n what something like the following as first paragraph ?\n\n Add functions to the V4L2Device class to convert to and from\n libcamera ColorSpace.\n\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_device.h | 7 +\n> src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> 2 files changed, 197 insertions(+)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index f21bc370..89ff6120 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n>\n> namespace libcamera {\n> @@ -44,6 +45,12 @@ public:\n>\n> \tvoid updateControlInfo();\n>\n> +\ttemplate\n> +\tstatic std::optional toColorSpace(const T &v4l2Format);\n> +\n> +\ttemplate\n> +\tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> +\n\nCan't we have this protected and the template specializations as\npublic in the appropriate classes ? Not sure if it's possible, but I\nguess it's desirable ?\n\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 9c783c9c..4d6985aa 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -16,6 +16,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n\nIs this just for the template specialization ?\n\n> #include \n> #include \n> #include \n> @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> \tframeStart.emit(event.u.frame_sync.frame_sequence);\n> }\n>\n> +static const std::map v4l2ToColorSpace = {\n> +\t{ V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> +\t{ V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> +\t{ V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> +\t{ V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> +\t{ V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> +\t{ V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> +};\n> +\n> +static const std::map v4l2ToYcbcrEncoding = {\n> +\t{ V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> +\t{ V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> +\t{ V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> +};\n> +\n> +static const std::map v4l2ToTransferFunction = {\n> +\t{ V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> +\t{ V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> +\t{ V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> +};\n> +\n> +static const std::map v4l2ToRange = {\n> +\t{ V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> +\t{ V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> +};\n> +\n> +static const std::vector> colorSpaceToV4l2 = {\n> +\t{ ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> +\t{ ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> +\t{ ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> +\t{ ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> +\t{ ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> +\t{ ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> +};\n> +\n> +static const std::map primariesToV4l2 = {\n> +\t{ ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> +\t{ ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> +\t{ ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> +\t{ ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> +};\n> +\n> +static const std::map ycbcrEncodingToV4l2 = {\n> +\t{ ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> +\t{ ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> +\t{ ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> +};\n> +\n> +static const std::map transferFunctionToV4l2 = {\n> +\t{ ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> +\t{ ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> +\t{ ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> +};\n> +\n> +static const std::map rangeToV4l2 = {\n> +\t{ ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> +\t{ ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> +};\n> +\n> +/**\n> + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> + * \\param[in] v4l2Format A V4L2 format containing color space information\n> + *\n> + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> + * V4L2 format structure are converted to a corresponding ColorSpace.\n> + *\n> + * If any V4L2 fields are not recognised then we return an \"unset\"\n> + * color space.\n> + *\n> + * \\return The ColorSpace corresponding to the input V4L2 format\n\nYou can return nullptr, this should be documented\n\n> + */\n> +template\n> +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> +{\n> +\tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> +\tif (itColor == v4l2ToColorSpace.end())\n> +\t\treturn std::nullopt;\n> +\n> +\t/* This sets all the color space fields to the correct \"default\" values. */\n> +\tColorSpace colorSpace = itColor->second;\n> +\n> +\tif (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> +\t\tauto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> +\t\tif (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> +\t}\n> +\n> +\tif (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> +\t\tauto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> +\t\tif (itTransfer == v4l2ToTransferFunction.end())\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tcolorSpace.transferFunction = itTransfer->second;\n> +\t}\n> +\n> +\tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> +\t\tauto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> +\t\tif (itRange == v4l2ToRange.end())\n> +\t\t\treturn std::nullopt;\n\nShould all of these cases be associated with an error message, asking\nto update the ColorSpace class to add the new\nencoding/xfer_func/quantization methods not yet supported ?\n> +\n> +\t\tcolorSpace.range = itRange->second;\n> +\t}\n> +\n> +\treturn colorSpace;\n> +}\n> +\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> +\n> +/**\n> + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> + * \\param[in] colorSpace The ColorSpace to be converted\n> + * \\param[out] v4l2Format A V4L2 format containing color space information\n\nI'm a bit uncertain about the two functions having different\nprototypes...\n\n> + *\n> + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> + *\n> + * An error is returned if any of the V4L2 fields do not support the\n> + * value given in the ColorSpace. Such fields are set to the V4L2\n> + * \"default\" values, but all other fields are still filled in where\n> + * possible.\n> + *\n> + * If the color space is completely unset, \"default\" V4L2 values are used\n> + * everywhere, so a driver would then choose its preferred color space.\n\nWe're translating to libcamera::ColorSpace to V4L2 fields.\n\nThe returned color space is adjusted if nothing in V4L2 matches a\nlibcamera defined colorspace/ecoding/quantization/xfer_func. Do you\nthink this could ever happen ?\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +template\n> +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> +{\n> +\tint ret = 0;\n> +\n> +\tv4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> +\tv4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> +\tv4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> +\tv4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> +\n> +\tif (!colorSpace)\n> +\t\treturn ret;\n\n\nreturn 0 and declare ret later ?\n\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 ret;\n> +\t}\n\nI again wonder when this could happen (no matching V4L2 colorspace).\nI understand if we were matching on the device supported colorospace,\nbut we're comparing against the ones defined by v4l2 in general.\n\nThanks\n j\n\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> +\tauto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> +\tif (itPrimaries != primariesToV4l2.end())\n> +\t\tv4l2Format.colorspace = itPrimaries->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> +\tif (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> +\t\tv4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> +\tif (itTransfer != transferFunctionToV4l2.end())\n> +\t\tv4l2Format.xfer_func = itTransfer->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\tauto itRange = rangeToV4l2.find(colorSpace->range);\n> +\tif (itRange != rangeToV4l2.end())\n> +\t\tv4l2Format.quantization = itRange->second;\n> +\telse\n> +\t\tret = -1;\n> +\n> +\treturn ret;\n> +}\n> +\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> +\n> } /* namespace libcamera */\n> --\n> 2.20.1\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 4AB79BDB13\n\tfor ;\n\tWed, 24 Nov 2021 16:05:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A257A60371;\n\tWed, 24 Nov 2021 17:05:22 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F358D60128\n\tfor ;\n\tWed, 24 Nov 2021 17:05:21 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 21314100014;\n\tWed, 24 Nov 2021 16:05:19 +0000 (UTC)"],"Date":"Wed, 24 Nov 2021 17:06:12 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211118151933.15627-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\thverkuil-cisco@xs4all.nl","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21226,"web_url":"https://patchwork.libcamera.org/comment/21226/","msgid":"","date":"2021-11-25T11:06:08","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for reviewing this!\n\nOn Wed, 24 Nov 2021 at 16:05, Jacopo Mondi wrote:\n>\n> Hi David\n>\n> On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote:\n> > These methods are added to the base V4L2Device class so that they can\n> > be shared both by the video device class and subdevices.\n> >\n> > With the ColorSpace class, the color space and related other fields\n> > are stored together, corresponding to a number of fields in the\n> > various different V4L2 format structures. Template methods are\n> > therefore a convenient implementation, and we must explicitly\n> > instantiate the templates that will be needed.\n> >\n> > Note that unset color spaces are converted to requests for the\n> > device's \"default\" color space.\n>\n> You seem to use \"unset\" in place of default in this version.\n\nYes, as a result of our discussion I went back to using std::optional\nso there is a genuine notion of it being \"unset\". Of course, there's\nthen the question of what that means and it's true that when you\nrequest a ColorSpace that is \"unset\", our V4L2 layer will ask the\ndrivers for their \"default\" colour space. So it's the same. Does that\nmake sense?\n\n>\n> Also, this is a nice explanation about the implementation but\n> what something like the following as first paragraph ?\n>\n> Add functions to the V4L2Device class to convert to and from\n> libcamera ColorSpace.\n\nYes, good idea.\n\n>\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 7 +\n> > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > 2 files changed, 197 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index f21bc370..89ff6120 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -44,6 +45,12 @@ public:\n> >\n> > void updateControlInfo();\n> >\n> > + template\n> > + static std::optional toColorSpace(const T &v4l2Format);\n> > +\n> > + template\n> > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > +\n>\n> Can't we have this protected and the template specializations as\n> public in the appropriate classes ? Not sure if it's possible, but I\n> guess it's desirable ?\n\nI'll see if I can make those protected.\n\n>\n> > protected:\n> > V4L2Device(const std::string &deviceNode);\n> > ~V4L2Device();\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 9c783c9c..4d6985aa 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -16,6 +16,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n>\n> Is this just for the template specialization ?\n\nNot sure that I remember any more! But I'll double-check if I really need it.\n\n>\n> > #include \n> > #include \n> > #include \n> > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > }\n> >\n> > +static const std::map v4l2ToColorSpace = {\n> > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > +};\n> > +\n> > +static const std::map v4l2ToYcbcrEncoding = {\n> > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > +};\n> > +\n> > +static const std::map v4l2ToTransferFunction = {\n> > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > +};\n> > +\n> > +static const std::map v4l2ToRange = {\n> > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > +};\n> > +\n> > +static const std::vector> colorSpaceToV4l2 = {\n> > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > +};\n> > +\n> > +static const std::map primariesToV4l2 = {\n> > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > +};\n> > +\n> > +static const std::map ycbcrEncodingToV4l2 = {\n> > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > +};\n> > +\n> > +static const std::map transferFunctionToV4l2 = {\n> > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > +};\n> > +\n> > +static const std::map rangeToV4l2 = {\n> > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > +};\n> > +\n> > +/**\n> > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > + *\n> > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > + *\n> > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > + * color space.\n> > + *\n> > + * \\return The ColorSpace corresponding to the input V4L2 format\n>\n> You can return nullptr, this should be documented\n\nIt can return a nullopt. If it couldn't return a nullopt I wouldn't\nhave returned a std::optional at all, so I'm thinking this is OK?\n\n>\n> > + */\n> > +template\n> > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > +{\n> > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > + if (itColor == v4l2ToColorSpace.end())\n> > + return std::nullopt;\n> > +\n> > + /* This sets all the color space fields to the correct \"default\" values. */\n> > + ColorSpace colorSpace = itColor->second;\n> > +\n> > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > + return std::nullopt;\n> > +\n> > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > + }\n> > +\n> > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > + if (itTransfer == v4l2ToTransferFunction.end())\n> > + return std::nullopt;\n> > +\n> > + colorSpace.transferFunction = itTransfer->second;\n> > + }\n> > +\n> > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > + if (itRange == v4l2ToRange.end())\n> > + return std::nullopt;\n>\n> Should all of these cases be associated with an error message, asking\n> to update the ColorSpace class to add the new\n> encoding/xfer_func/quantization methods not yet supported ?\n\nCurrently I'm signalling errors where these values get used. I'm\nalways a bit nervous about outputting error messages at the lowest\nlevel unless you're really sure, on the grounds that they can be hard\nto turn off! But I don't feel very strongly...\n\n> > +\n> > + colorSpace.range = itRange->second;\n> > + }\n> > +\n> > + return colorSpace;\n> > +}\n> > +\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > +\n> > +/**\n> > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > + * \\param[in] colorSpace The ColorSpace to be converted\n> > + * \\param[out] v4l2Format A V4L2 format containing color space information\n>\n> I'm a bit uncertain about the two functions having different\n> prototypes...\n\nYes, the catch with filling in V4L2 fields is that there's no way to\nsay \"I couldn't do it\", so there's an extra return code to signal\nthat.\n\n>\n> > + *\n> > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > + *\n> > + * An error is returned if any of the V4L2 fields do not support the\n> > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > + * \"default\" values, but all other fields are still filled in where\n> > + * possible.\n> > + *\n> > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > + * everywhere, so a driver would then choose its preferred color space.\n>\n> We're translating to libcamera::ColorSpace to V4L2 fields.\n>\n> The returned color space is adjusted if nothing in V4L2 matches a\n> libcamera defined colorspace/ecoding/quantization/xfer_func. Do you\n> think this could ever happen ?\n\nNot for the time being, though possibly further in the future. I think\nit's probably fair enough if you can't get the colour space that you\nwant?\n\n>\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +template\n> > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > +{\n> > + int ret = 0;\n> > +\n> > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > +\n> > + if (!colorSpace)\n> > + return ret;\n>\n>\n> return 0 and declare ret later ?\n>\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 ret;\n> > + }\n>\n> I again wonder when this could happen (no matching V4L2 colorspace).\n> I understand if we were matching on the device supported colorospace,\n> but we're comparing against the ones defined by v4l2 in general.\n\nAs above, at the moment this can't happen, though maybe one day...\nReturning an error code seems reasonable to me in this case so that a\nproblem gets flagged up. It's unlikely that you're going to want to\n\"fix\" V4L2, or a driver, to give you the colour space you want, but\nyou should still know that there are some problems that may need\nattention.\n\nThanks!\nDavid\n\n>\n> Thanks\n> j\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> > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > + if (itPrimaries != primariesToV4l2.end())\n> > + v4l2Format.colorspace = itPrimaries->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > + if (itTransfer != transferFunctionToV4l2.end())\n> > + v4l2Format.xfer_func = itTransfer->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > + if (itRange != rangeToV4l2.end())\n> > + v4l2Format.quantization = itRange->second;\n> > + else\n> > + ret = -1;\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > +\n> > } /* namespace libcamera */\n> > --\n> > 2.20.1\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 70C53BF415\n\tfor ;\n\tThu, 25 Nov 2021 11:06:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22FD76036F;\n\tThu, 25 Nov 2021 12:06:22 +0100 (CET)","from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com\n\t[IPv6:2a00:1450:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C11860231\n\tfor ;\n\tThu, 25 Nov 2021 12:06:20 +0100 (CET)","by mail-wm1-x32c.google.com with SMTP id 137so5236554wma.1\n\tfor ;\n\tThu, 25 Nov 2021 03:06:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"DFi6U/+b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=eUpnl3B0rb4j2TfnUCqGOzvTj6dVRopj1WBFGwqq3vg=;\n\tb=DFi6U/+bdA/if63boqSoC0hvibj1j3E7UVdgBeWO4uTNoxcI6XFaICWRokh++NL683\n\tP/ILHnDtfCBe8f/gd3LDIrFKfC+1gmczWAb1flSv/xZeG43hpy8+xX+3C4fVy7q9/8HB\n\t3kuDb0qcoRkzbglkvpJUOonllSsaEpGBEmbsta+GS3TrQJwwQSfj+dHDRK5xjJsgB6TC\n\tNaIw37qI172pTdG1D7qPNHgjueQFIqHXc96ispAo3kWTXoU7WMmsdoc1f9R9h1+A8r9q\n\tCNRpm5ilkY9uFFziPiS1WTHNsNDDLCfqrR+f4cJ8VIvEFm+pV9h+As0Lk9byZX6CDqih\n\t5+lA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=eUpnl3B0rb4j2TfnUCqGOzvTj6dVRopj1WBFGwqq3vg=;\n\tb=HiJ/69MEXetr4/KRHbdBRamzWQkTpA7ieaLyORLtac/j4J2T+0hqiHE3ulc8sy4GlC\n\tAMHnnULczQeFdCV6+gIRzuUvOW/OTRFh4wGfrjGdAIfD/lNx3RNeRHVTFHrF2OjDcwX9\n\tYgOOcgcXtPwlbFajbxtb42gh0ZwtoEUNbZhiblPksqKt/FdG5QEnOPBfJWCblC4gfGNa\n\tziYAgNcJBKf+Kfdvf2vY7p2K+JuteeYvTFMUxffEOB4RKGsABvsz+gkMH1W3fArBMB96\n\tXj6yRNuuUPFBjgb5FeTZp8xFgil6341KGFm0rozo8NdoQIAVJSIj5T1KQDXxBqi8zTDL\n\tbdyQ==","X-Gm-Message-State":"AOAM530YYYXelq7hF7LlZ8JY4VXb5UMnw8neMzly52o19YjP7jh0BvKT\n\tODe4/Y1ho+91SXURmb7P6S+k7JEgAtyr9jLfH90oEQ==","X-Google-Smtp-Source":"ABdhPJwJ8+aoHrqGoWBLdP+h6SZL6wq3YqXxuGubD+aBOc+1oJEb3+5JbCIDnSPRf050zIf/tTfjsR+pLIm1XHPIyp8=","X-Received":"by 2002:a05:600c:3b0a:: with SMTP id\n\tm10mr6121281wms.130.1637838379852; \n\tThu, 25 Nov 2021 03:06:19 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>","In-Reply-To":"<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 11:06:08 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21234,"web_url":"https://patchwork.libcamera.org/comment/21234/","msgid":"<163784040367.3059017.15937905094444936814@Monstersaurus>","date":"2021-11-25T11:40:03","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-25 11:06:08)\n> Hi Jacopo\n> \n> Thanks for reviewing this!\n> \n> On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi wrote:\n> >\n> > Hi David\n> >\n> > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote:\n> > > These methods are added to the base V4L2Device class so that they can\n> > > be shared both by the video device class and subdevices.\n> > >\n> > > With the ColorSpace class, the color space and related other fields\n> > > are stored together, corresponding to a number of fields in the\n> > > various different V4L2 format structures. Template methods are\n> > > therefore a convenient implementation, and we must explicitly\n> > > instantiate the templates that will be needed.\n> > >\n> > > Note that unset color spaces are converted to requests for the\n> > > device's \"default\" color space.\n> >\n> > You seem to use \"unset\" in place of default in this version.\n> \n> Yes, as a result of our discussion I went back to using std::optional\n> so there is a genuine notion of it being \"unset\". Of course, there's\n> then the question of what that means and it's true that when you\n> request a ColorSpace that is \"unset\", our V4L2 layer will ask the\n> drivers for their \"default\" colour space. So it's the same. Does that\n> make sense?\n> \n> >\n> > Also, this is a nice explanation about the implementation but\n> > what something like the following as first paragraph ?\n> >\n> > Add functions to the V4L2Device class to convert to and from\n> > libcamera ColorSpace.\n> \n> Yes, good idea.\n> \n> >\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 7 +\n> > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > > 2 files changed, 197 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index f21bc370..89ff6120 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -44,6 +45,12 @@ public:\n> > >\n> > > void updateControlInfo();\n> > >\n> > > + template\n> > > + static std::optional toColorSpace(const T &v4l2Format);\n> > > +\n> > > + template\n> > > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > > +\n> >\n> > Can't we have this protected and the template specializations as\n> > public in the appropriate classes ? Not sure if it's possible, but I\n> > guess it's desirable ?\n> \n> I'll see if I can make those protected.\n> \n> >\n> > > protected:\n> > > V4L2Device(const std::string &deviceNode);\n> > > ~V4L2Device();\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 9c783c9c..4d6985aa 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -16,6 +16,8 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +\n> >\n> > Is this just for the template specialization ?\n> \n> Not sure that I remember any more! But I'll double-check if I really need it.\n> \n> >\n> > > #include \n> > > #include \n> > > #include \n> > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > > }\n> > >\n> > > +static const std::map v4l2ToColorSpace = {\n> > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToYcbcrEncoding = {\n> > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToTransferFunction = {\n> > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > > +};\n> > > +\n> > > +static const std::map v4l2ToRange = {\n> > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > > +};\n> > > +\n> > > +static const std::vector> colorSpaceToV4l2 = {\n> > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map primariesToV4l2 = {\n> > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map ycbcrEncodingToV4l2 = {\n> > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > +};\n> > > +\n> > > +static const std::map transferFunctionToV4l2 = {\n> > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > +};\n> > > +\n> > > +static const std::map rangeToV4l2 = {\n> > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > > + *\n> > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > > + *\n> > > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > > + * color space.\n> > > + *\n> > > + * \\return The ColorSpace corresponding to the input V4L2 format\n> >\n> > You can return nullptr, this should be documented\n> \n> It can return a nullopt. If it couldn't return a nullopt I wouldn't\n> have returned a std::optional at all, so I'm thinking this is OK?\n\nI think something like this should be added (after the existing return)\n\n\t\\retval std::nullopt One or more V4L2 color space fields were not recognised\n\nIt duplicates what you've written above, but it directly documents the\nreturn values in the Doxygen output.\n\n> >\n> > > + */\n> > > +template\n> > > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > > +{\n> > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > > + if (itColor == v4l2ToColorSpace.end())\n> > > + return std::nullopt;\n> > > +\n> > > + /* This sets all the color space fields to the correct \"default\" values. */\n> > > + ColorSpace colorSpace = itColor->second;\n> > > +\n> > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > > + return std::nullopt;\n> > > +\n> > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > > + }\n> > > +\n> > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > > + if (itTransfer == v4l2ToTransferFunction.end())\n> > > + return std::nullopt;\n> > > +\n> > > + colorSpace.transferFunction = itTransfer->second;\n> > > + }\n> > > +\n> > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > > + if (itRange == v4l2ToRange.end())\n> > > + return std::nullopt;\n> >\n> > Should all of these cases be associated with an error message, asking\n> > to update the ColorSpace class to add the new\n> > encoding/xfer_func/quantization methods not yet supported ?\n> \n> Currently I'm signalling errors where these values get used. I'm\n> always a bit nervous about outputting error messages at the lowest\n> level unless you're really sure, on the grounds that they can be hard\n> to turn off! But I don't feel very strongly...\n> \n> > > +\n> > > + colorSpace.range = itRange->second;\n> > > + }\n> > > +\n> > > + return colorSpace;\n> > > +}\n> > > +\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > > +\n> > > +/**\n> > > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > > + * \\param[in] colorSpace The ColorSpace to be converted\n> > > + * \\param[out] v4l2Format A V4L2 format containing color space information\n> >\n> > I'm a bit uncertain about the two functions having different\n> > prototypes...\n> \n> Yes, the catch with filling in V4L2 fields is that there's no way to\n> say \"I couldn't do it\", so there's an extra return code to signal\n> that.\n> \n> >\n> > > + *\n> > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > > + *\n> > > + * An error is returned if any of the V4L2 fields do not support the\n> > > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > > + * \"default\" values, but all other fields are still filled in where\n> > > + * possible.\n> > > + *\n> > > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > > + * everywhere, so a driver would then choose its preferred color space.\n> >\n> > We're translating to libcamera::ColorSpace to V4L2 fields.\n> >\n> > The returned color space is adjusted if nothing in V4L2 matches a\n> > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you\n> > think this could ever happen ?\n> \n> Not for the time being, though possibly further in the future. I think\n> it's probably fair enough if you can't get the colour space that you\n> want?\n> \n> >\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +template\n> > > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > > +{\n> > > + int ret = 0;\n> > > +\n> > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > > +\n> > > + if (!colorSpace)\n> > > + return ret;\n> >\n> >\n> > return 0 and declare ret later ?\n> >\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 ret;\n> > > + }\n> >\n> > I again wonder when this could happen (no matching V4L2 colorspace).\n> > I understand if we were matching on the device supported colorospace,\n> > but we're comparing against the ones defined by v4l2 in general.\n> \n> As above, at the moment this can't happen, though maybe one day...\n> Returning an error code seems reasonable to me in this case so that a\n> problem gets flagged up. It's unlikely that you're going to want to\n> \"fix\" V4L2, or a driver, to give you the colour space you want, but\n\nI disagree ;-)\n\nIf the Kernel has a fault, or is missing a feature - it should be fixed\nor added. That's the point of it being open source. We're not reliant on\nwaiting for some external company to provide it to us. The\nuser/developer who hits the issue is exactly the one who should be\npushing the kernel side too.\n\nI know lots of people get scared of the kernel, but even just reporting\nit to the linux-media list (or libcamera) would be enough to make it\nnoticed.\n\nIn this instance, if somehow we have added a colour space that is not\nsupported by V4L2 ... then either libcamera needs to convert it\n(correctly?) or V4L2 needs to support it if it gets used?\n\nAt this part, we're only mapping a libcamera colour space to a v4l2\ncolor space. So that should always be possible in some form or it's a\nbug that needs to be fixed.\n\nIf the driver doesn't support the color space, that's separate of\ncourse, and should be handled when setting/getting.\n\nBut we should always be able to map a libcamera description to an\nequivalent kernel description. And if we can't - we can and should\nreport an error.\n\n\n> you should still know that there are some problems that may need\n> attention.\n> \n> Thanks!\n> David\n> \n> >\n> > Thanks\n> > j\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> > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > > + if (itPrimaries != primariesToV4l2.end())\n> > > + v4l2Format.colorspace = itPrimaries->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > > + if (itTransfer != transferFunctionToV4l2.end())\n> > > + v4l2Format.xfer_func = itTransfer->second;\n> > > + else\n> > > + ret = -1;\n> > > +\n> > > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > > + if (itRange != rangeToV4l2.end())\n> > > + v4l2Format.quantization = itRange->second;\n> > > + else\n> > > + ret = -1;\n\nHrm ... I think I've already commented on those, but just in case, I\nthink these should be -EINVAL.\n\nWe could also have an explicit \\retval too.\n\n> > > +\n> > > + return ret;\n> > > +}\n> > > +\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > > +\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.20.1\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 49B86BDB13\n\tfor ;\n\tThu, 25 Nov 2021 11:40:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF2A06036F;\n\tThu, 25 Nov 2021 12:40:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFAB460231\n\tfor ;\n\tThu, 25 Nov 2021 12:40:06 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3AA5B90E;\n\tThu, 25 Nov 2021 12:40:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"H27QKHlP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637840406;\n\tbh=SjUu52iAhjOta/FwCJX3PkdfIq3asiNQlfsN9OPCDbw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=H27QKHlPNV5OZLHoOzN/DFr99MNC54Ma+fkFY7BbWHc1TMM8P4HDOh4iugheo7Sb0\n\tXSv/2VmLRw05wmwNPX/YiwJxqF2dKn50uZRL5IXyD0MVg+jcK2dqrnA2IxG96CAIwx\n\tsE/bisCnYYj47kEHMmGhnCNxUk8QwfPiSZMFUTeM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tJacopo Mondi ","Date":"Thu, 25 Nov 2021 11:40:03 +0000","Message-ID":"<163784040367.3059017.15937905094444936814@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Hans Verkuil , Tomasz Figa , \n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21236,"web_url":"https://patchwork.libcamera.org/comment/21236/","msgid":"<20211125115405.xngh44ff7fnzlgiy@uno.localdomain>","date":"2021-11-25T11:54:05","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David, Kieran,\n\nOn Thu, Nov 25, 2021 at 11:40:03AM +0000, Kieran Bingham wrote:\n> Quoting David Plowman (2021-11-25 11:06:08)\n> > Hi Jacopo\n> >\n> > Thanks for reviewing this!\n> >\n> > On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote:\n> > > > These methods are added to the base V4L2Device class so that they can\n> > > > be shared both by the video device class and subdevices.\n> > > >\n> > > > With the ColorSpace class, the color space and related other fields\n> > > > are stored together, corresponding to a number of fields in the\n> > > > various different V4L2 format structures. Template methods are\n> > > > therefore a convenient implementation, and we must explicitly\n> > > > instantiate the templates that will be needed.\n> > > >\n> > > > Note that unset color spaces are converted to requests for the\n> > > > device's \"default\" color space.\n> > >\n> > > You seem to use \"unset\" in place of default in this version.\n> >\n> > Yes, as a result of our discussion I went back to using std::optional\n> > so there is a genuine notion of it being \"unset\". Of course, there's\n> > then the question of what that means and it's true that when you\n> > request a ColorSpace that is \"unset\", our V4L2 layer will ask the\n> > drivers for their \"default\" colour space. So it's the same. Does that\n> > make sense?\n> >\n> > >\n> > > Also, this is a nice explanation about the implementation but\n> > > what something like the following as first paragraph ?\n> > >\n> > > Add functions to the V4L2Device class to convert to and from\n> > > libcamera ColorSpace.\n> >\n> > Yes, good idea.\n> >\n> > >\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/internal/v4l2_device.h | 7 +\n> > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > > > 2 files changed, 197 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > index f21bc370..89ff6120 100644\n> > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > @@ -17,6 +17,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > namespace libcamera {\n> > > > @@ -44,6 +45,12 @@ public:\n> > > >\n> > > > void updateControlInfo();\n> > > >\n> > > > + template\n> > > > + static std::optional toColorSpace(const T &v4l2Format);\n> > > > +\n> > > > + template\n> > > > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > > > +\n> > >\n> > > Can't we have this protected and the template specializations as\n> > > public in the appropriate classes ? Not sure if it's possible, but I\n> > > guess it's desirable ?\n> >\n> > I'll see if I can make those protected.\n> >\n> > >\n> > > > protected:\n> > > > V4L2Device(const std::string &deviceNode);\n> > > > ~V4L2Device();\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index 9c783c9c..4d6985aa 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -16,6 +16,8 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > >\n> > > Is this just for the template specialization ?\n> >\n> > Not sure that I remember any more! But I'll double-check if I really need it.\n> >\n> > >\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > > > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > > > }\n> > > >\n> > > > +static const std::map v4l2ToColorSpace = {\n> > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > > > +};\n> > > > +\n> > > > +static const std::map v4l2ToYcbcrEncoding = {\n> > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > > > +};\n> > > > +\n> > > > +static const std::map v4l2ToTransferFunction = {\n> > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > > > +};\n> > > > +\n> > > > +static const std::map v4l2ToRange = {\n> > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > > > +};\n> > > > +\n> > > > +static const std::vector> colorSpaceToV4l2 = {\n> > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > > +};\n> > > > +\n> > > > +static const std::map primariesToV4l2 = {\n> > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > > +};\n> > > > +\n> > > > +static const std::map ycbcrEncodingToV4l2 = {\n> > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > > +};\n> > > > +\n> > > > +static const std::map transferFunctionToV4l2 = {\n> > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > > +};\n> > > > +\n> > > > +static const std::map rangeToV4l2 = {\n> > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > > > +};\n> > > > +\n> > > > +/**\n> > > > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > > > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > > > + *\n> > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > > > + *\n> > > > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > > > + * color space.\n> > > > + *\n> > > > + * \\return The ColorSpace corresponding to the input V4L2 format\n> > >\n> > > You can return nullptr, this should be documented\n> >\n> > It can return a nullopt. If it couldn't return a nullopt I wouldn't\n> > have returned a std::optional at all, so I'm thinking this is OK?\n>\n> I think something like this should be added (after the existing return)\n>\n> \t\\retval std::nullopt One or more V4L2 color space fields were not recognised\n>\n> It duplicates what you've written above, but it directly documents the\n> return values in the Doxygen output.\n>\n> > >\n> > > > + */\n> > > > +template\n> > > > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > > > +{\n> > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > > > + if (itColor == v4l2ToColorSpace.end())\n> > > > + return std::nullopt;\n> > > > +\n> > > > + /* This sets all the color space fields to the correct \"default\" values. */\n> > > > + ColorSpace colorSpace = itColor->second;\n> > > > +\n> > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > > > + return std::nullopt;\n> > > > +\n> > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > > > + }\n> > > > +\n> > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > > > + if (itTransfer == v4l2ToTransferFunction.end())\n> > > > + return std::nullopt;\n> > > > +\n> > > > + colorSpace.transferFunction = itTransfer->second;\n> > > > + }\n> > > > +\n> > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > > > + if (itRange == v4l2ToRange.end())\n> > > > + return std::nullopt;\n> > >\n> > > Should all of these cases be associated with an error message, asking\n> > > to update the ColorSpace class to add the new\n> > > encoding/xfer_func/quantization methods not yet supported ?\n> >\n> > Currently I'm signalling errors where these values get used. I'm\n> > always a bit nervous about outputting error messages at the lowest\n> > level unless you're really sure, on the grounds that they can be hard\n> > to turn off! But I don't feel very strongly...\n> >\n> > > > +\n> > > > + colorSpace.range = itRange->second;\n> > > > + }\n> > > > +\n> > > > + return colorSpace;\n> > > > +}\n> > > > +\n> > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > > > +\n> > > > +/**\n> > > > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > > > + * \\param[in] colorSpace The ColorSpace to be converted\n> > > > + * \\param[out] v4l2Format A V4L2 format containing color space information\n> > >\n> > > I'm a bit uncertain about the two functions having different\n> > > prototypes...\n> >\n> > Yes, the catch with filling in V4L2 fields is that there's no way to\n> > say \"I couldn't do it\", so there's an extra return code to signal\n> > that.\n> >\n> > >\n> > > > + *\n> > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > > > + *\n> > > > + * An error is returned if any of the V4L2 fields do not support the\n> > > > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > > > + * \"default\" values, but all other fields are still filled in where\n> > > > + * possible.\n> > > > + *\n> > > > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > > > + * everywhere, so a driver would then choose its preferred color space.\n> > >\n> > > We're translating to libcamera::ColorSpace to V4L2 fields.\n> > >\n> > > The returned color space is adjusted if nothing in V4L2 matches a\n> > > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you\n> > > think this could ever happen ?\n> >\n> > Not for the time being, though possibly further in the future. I think\n> > it's probably fair enough if you can't get the colour space that you\n> > want?\n> >\n> > >\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +template\n> > > > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > > > +{\n> > > > + int ret = 0;\n> > > > +\n> > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > > > +\n> > > > + if (!colorSpace)\n> > > > + return ret;\n> > >\n> > >\n> > > return 0 and declare ret later ?\n> > >\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 ret;\n> > > > + }\n> > >\n> > > I again wonder when this could happen (no matching V4L2 colorspace).\n> > > I understand if we were matching on the device supported colorospace,\n> > > but we're comparing against the ones defined by v4l2 in general.\n> >\n> > As above, at the moment this can't happen, though maybe one day...\n> > Returning an error code seems reasonable to me in this case so that a\n> > problem gets flagged up. It's unlikely that you're going to want to\n> > \"fix\" V4L2, or a driver, to give you the colour space you want, but\n>\n> I disagree ;-)\n>\n> If the Kernel has a fault, or is missing a feature - it should be fixed\n> or added. That's the point of it being open source. We're not reliant on\n> waiting for some external company to provide it to us. The\n> user/developer who hits the issue is exactly the one who should be\n> pushing the kernel side too.\n>\n> I know lots of people get scared of the kernel, but even just reporting\n> it to the linux-media list (or libcamera) would be enough to make it\n> noticed.\n>\n> In this instance, if somehow we have added a colour space that is not\n> supported by V4L2 ... then either libcamera needs to convert it\n> (correctly?) or V4L2 needs to support it if it gets used?\n>\n> At this part, we're only mapping a libcamera colour space to a v4l2\n> color space. So that should always be possible in some form or it's a\n> bug that needs to be fixed.\n>\n> If the driver doesn't support the color space, that's separate of\n> course, and should be handled when setting/getting.\n>\n> But we should always be able to map a libcamera description to an\n> equivalent kernel description. And if we can't - we can and should\n> report an error.\n>\n\nI concur. I'm not getting why we're so sensitive about color spaces\nmismatches. If anything needs to be supported in kernel or libcamera\nto have a device working properly, we should be loud and warn users\nthey need to update the faulty side. We do the same for CameraSensor\nrequired controls, where so far we've been complaining about missing\nfeatures, but we should sooner or later refuse to operate to have the\nsensor drivers updated to the minimum required features set. I know it\nmight sound harsh, but I don't see other ways to actually reach a\nstandardization.\n\n>\n> > you should still know that there are some problems that may need\n> > attention.\n> >\n> > Thanks!\n> > David\n> >\n> > >\n> > > Thanks\n> > > j\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> > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > > > + if (itPrimaries != primariesToV4l2.end())\n> > > > + v4l2Format.colorspace = itPrimaries->second;\n> > > > + else\n> > > > + ret = -1;\n> > > > +\n> > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > > > + else\n> > > > + ret = -1;\n> > > > +\n> > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > > > + if (itTransfer != transferFunctionToV4l2.end())\n> > > > + v4l2Format.xfer_func = itTransfer->second;\n> > > > + else\n> > > > + ret = -1;\n> > > > +\n> > > > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > > > + if (itRange != rangeToV4l2.end())\n> > > > + v4l2Format.quantization = itRange->second;\n> > > > + else\n> > > > + ret = -1;\n>\n> Hrm ... I think I've already commented on those, but just in case, I\n> think these should be -EINVAL.\n>\n> We could also have an explicit \\retval too.\n>\n> > > > +\n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > > > +\n> > > > } /* namespace libcamera */\n> > > > --\n> > > > 2.20.1\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 16C23BDB13\n\tfor ;\n\tThu, 25 Nov 2021 11:53:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8EC4960376;\n\tThu, 25 Nov 2021 12:53:15 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 288D660231\n\tfor ;\n\tThu, 25 Nov 2021 12:53:15 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 53B9710000E;\n\tThu, 25 Nov 2021 11:53:13 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 12:54:05 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211125115405.xngh44ff7fnzlgiy@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>\n\t\n\t<163784040367.3059017.15937905094444936814@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163784040367.3059017.15937905094444936814@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21244,"web_url":"https://patchwork.libcamera.org/comment/21244/","msgid":"","date":"2021-11-25T12:14:05","subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nOn Thu, 25 Nov 2021 at 11:53, Jacopo Mondi wrote:\n>\n> Hi David, Kieran,\n>\n> On Thu, Nov 25, 2021 at 11:40:03AM +0000, Kieran Bingham wrote:\n> > Quoting David Plowman (2021-11-25 11:06:08)\n> > > Hi Jacopo\n> > >\n> > > Thanks for reviewing this!\n> > >\n> > > On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi wrote:\n> > > >\n> > > > Hi David\n> > > >\n> > > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote:\n> > > > > These methods are added to the base V4L2Device class so that they can\n> > > > > be shared both by the video device class and subdevices.\n> > > > >\n> > > > > With the ColorSpace class, the color space and related other fields\n> > > > > are stored together, corresponding to a number of fields in the\n> > > > > various different V4L2 format structures. Template methods are\n> > > > > therefore a convenient implementation, and we must explicitly\n> > > > > instantiate the templates that will be needed.\n> > > > >\n> > > > > Note that unset color spaces are converted to requests for the\n> > > > > device's \"default\" color space.\n> > > >\n> > > > You seem to use \"unset\" in place of default in this version.\n> > >\n> > > Yes, as a result of our discussion I went back to using std::optional\n> > > so there is a genuine notion of it being \"unset\". Of course, there's\n> > > then the question of what that means and it's true that when you\n> > > request a ColorSpace that is \"unset\", our V4L2 layer will ask the\n> > > drivers for their \"default\" colour space. So it's the same. Does that\n> > > make sense?\n> > >\n> > > >\n> > > > Also, this is a nice explanation about the implementation but\n> > > > what something like the following as first paragraph ?\n> > > >\n> > > > Add functions to the V4L2Device class to convert to and from\n> > > > libcamera ColorSpace.\n> > >\n> > > Yes, good idea.\n> > >\n> > > >\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/internal/v4l2_device.h | 7 +\n> > > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++\n> > > > > 2 files changed, 197 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > > index f21bc370..89ff6120 100644\n> > > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > > @@ -17,6 +17,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > namespace libcamera {\n> > > > > @@ -44,6 +45,12 @@ public:\n> > > > >\n> > > > > void updateControlInfo();\n> > > > >\n> > > > > + template\n> > > > > + static std::optional toColorSpace(const T &v4l2Format);\n> > > > > +\n> > > > > + template\n> > > > > + static int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > > > > +\n> > > >\n> > > > Can't we have this protected and the template specializations as\n> > > > public in the appropriate classes ? Not sure if it's possible, but I\n> > > > guess it's desirable ?\n> > >\n> > > I'll see if I can make those protected.\n> > >\n> > > >\n> > > > > protected:\n> > > > > V4L2Device(const std::string &deviceNode);\n> > > > > ~V4L2Device();\n> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > > index 9c783c9c..4d6985aa 100644\n> > > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > > @@ -16,6 +16,8 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > +\n> > > >\n> > > > Is this just for the template specialization ?\n> > >\n> > > Not sure that I remember any more! But I'll double-check if I really need it.\n> > >\n> > > >\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable()\n> > > > > frameStart.emit(event.u.frame_sync.frame_sequence);\n> > > > > }\n> > > > >\n> > > > > +static const std::map v4l2ToColorSpace = {\n> > > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n> > > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> > > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> > > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },\n> > > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },\n> > > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map v4l2ToYcbcrEncoding = {\n> > > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 },\n> > > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 },\n> > > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map v4l2ToTransferFunction = {\n> > > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear },\n> > > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb },\n> > > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map v4l2ToRange = {\n> > > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full },\n> > > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::vector> colorSpaceToV4l2 = {\n> > > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },\n> > > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map primariesToV4l2 = {\n> > > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map ycbcrEncodingToV4l2 = {\n> > > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map transferFunctionToV4l2 = {\n> > > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > > > +};\n> > > > > +\n> > > > > +static const std::map rangeToV4l2 = {\n> > > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > > > > +};\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Convert the color space fields in a V4L2 format to a ColorSpace\n> > > > > + * \\param[in] v4l2Format A V4L2 format containing color space information\n> > > > > + *\n> > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > > > + * V4L2 format structure are converted to a corresponding ColorSpace.\n> > > > > + *\n> > > > > + * If any V4L2 fields are not recognised then we return an \"unset\"\n> > > > > + * color space.\n> > > > > + *\n> > > > > + * \\return The ColorSpace corresponding to the input V4L2 format\n> > > >\n> > > > You can return nullptr, this should be documented\n> > >\n> > > It can return a nullopt. If it couldn't return a nullopt I wouldn't\n> > > have returned a std::optional at all, so I'm thinking this is OK?\n> >\n> > I think something like this should be added (after the existing return)\n> >\n> > \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> >\n> > It duplicates what you've written above, but it directly documents the\n> > return values in the Doxygen output.\n\nYes, I like that. Shows how much I know about Doxygen (i.e. nothing!).\n\n> >\n> > > >\n> > > > > + */\n> > > > > +template\n> > > > > +std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > > > > +{\n> > > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > > > > + if (itColor == v4l2ToColorSpace.end())\n> > > > > + return std::nullopt;\n> > > > > +\n> > > > > + /* This sets all the color space fields to the correct \"default\" values. */\n> > > > > + ColorSpace colorSpace = itColor->second;\n> > > > > +\n> > > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) {\n> > > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc);\n> > > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end())\n> > > > > + return std::nullopt;\n> > > > > +\n> > > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > > > > + }\n> > > > > +\n> > > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) {\n> > > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func);\n> > > > > + if (itTransfer == v4l2ToTransferFunction.end())\n> > > > > + return std::nullopt;\n> > > > > +\n> > > > > + colorSpace.transferFunction = itTransfer->second;\n> > > > > + }\n> > > > > +\n> > > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization);\n> > > > > + if (itRange == v4l2ToRange.end())\n> > > > > + return std::nullopt;\n> > > >\n> > > > Should all of these cases be associated with an error message, asking\n> > > > to update the ColorSpace class to add the new\n> > > > encoding/xfer_func/quantization methods not yet supported ?\n> > >\n> > > Currently I'm signalling errors where these values get used. I'm\n> > > always a bit nervous about outputting error messages at the lowest\n> > > level unless you're really sure, on the grounds that they can be hard\n> > > to turn off! But I don't feel very strongly...\n> > >\n> > > > > +\n> > > > > + colorSpace.range = itRange->second;\n> > > > > + }\n> > > > > +\n> > > > > + return colorSpace;\n> > > > > +}\n> > > > > +\n> > > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > > > > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > > > > + * \\param[in] colorSpace The ColorSpace to be converted\n> > > > > + * \\param[out] v4l2Format A V4L2 format containing color space information\n> > > >\n> > > > I'm a bit uncertain about the two functions having different\n> > > > prototypes...\n> > >\n> > > Yes, the catch with filling in V4L2 fields is that there's no way to\n> > > say \"I couldn't do it\", so there's an extra return code to signal\n> > > that.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a\n> > > > > + * V4L2 format structure are filled in from a corresponding ColorSpace.\n> > > > > + *\n> > > > > + * An error is returned if any of the V4L2 fields do not support the\n> > > > > + * value given in the ColorSpace. Such fields are set to the V4L2\n> > > > > + * \"default\" values, but all other fields are still filled in where\n> > > > > + * possible.\n> > > > > + *\n> > > > > + * If the color space is completely unset, \"default\" V4L2 values are used\n> > > > > + * everywhere, so a driver would then choose its preferred color space.\n> > > >\n> > > > We're translating to libcamera::ColorSpace to V4L2 fields.\n> > > >\n> > > > The returned color space is adjusted if nothing in V4L2 matches a\n> > > > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you\n> > > > think this could ever happen ?\n> > >\n> > > Not for the time being, though possibly further in the future. I think\n> > > it's probably fair enough if you can't get the colour space that you\n> > > want?\n> > >\n> > > >\n> > > > > + *\n> > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > + */\n> > > > > +template\n> > > > > +int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v4l2Format)\n> > > > > +{\n> > > > > + int ret = 0;\n> > > > > +\n> > > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > > > > +\n> > > > > + if (!colorSpace)\n> > > > > + return ret;\n> > > >\n> > > >\n> > > > return 0 and declare ret later ?\n> > > >\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 ret;\n> > > > > + }\n> > > >\n> > > > I again wonder when this could happen (no matching V4L2 colorspace).\n> > > > I understand if we were matching on the device supported colorospace,\n> > > > but we're comparing against the ones defined by v4l2 in general.\n> > >\n> > > As above, at the moment this can't happen, though maybe one day...\n> > > Returning an error code seems reasonable to me in this case so that a\n> > > problem gets flagged up. It's unlikely that you're going to want to\n> > > \"fix\" V4L2, or a driver, to give you the colour space you want, but\n> >\n> > I disagree ;-)\n> >\n> > If the Kernel has a fault, or is missing a feature - it should be fixed\n> > or added. That's the point of it being open source. We're not reliant on\n> > waiting for some external company to provide it to us. The\n> > user/developer who hits the issue is exactly the one who should be\n> > pushing the kernel side too.\n> >\n> > I know lots of people get scared of the kernel, but even just reporting\n> > it to the linux-media list (or libcamera) would be enough to make it\n> > noticed.\n> >\n> > In this instance, if somehow we have added a colour space that is not\n> > supported by V4L2 ... then either libcamera needs to convert it\n> > (correctly?) or V4L2 needs to support it if it gets used?\n> >\n> > At this part, we're only mapping a libcamera colour space to a v4l2\n> > color space. So that should always be possible in some form or it's a\n> > bug that needs to be fixed.\n> >\n> > If the driver doesn't support the color space, that's separate of\n> > course, and should be handled when setting/getting.\n> >\n> > But we should always be able to map a libcamera description to an\n> > equivalent kernel description. And if we can't - we can and should\n> > report an error.\n> >\n>\n> I concur. I'm not getting why we're so sensitive about color spaces\n> mismatches. If anything needs to be supported in kernel or libcamera\n> to have a device working properly, we should be loud and warn users\n> they need to update the faulty side. We do the same for CameraSensor\n> required controls, where so far we've been complaining about missing\n> features, but we should sooner or later refuse to operate to have the\n> sensor drivers updated to the minimum required features set. I know it\n> might sound harsh, but I don't see other ways to actually reach a\n> standardization.\n\nI agree that fixing the thing that is broken is the right thing to do,\nthough I'm still a bit unconvinced what will happen in practice. Will\na random developer with some random device that they need to make work\non an old kernel go to this trouble? I don't think I'm as optimistic\nas you!\n\nBut I'm fine to flag errors when things are missing - indeed this is\nexactly what it already does (though maybe with \"Warning\" rather than\n\"Error\", but that's trivial to change). So I think this is all in\norder.\n\nDavid\n\n>\n> >\n> > > you should still know that there are some problems that may need\n> > > attention.\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > >\n> > > > Thanks\n> > > > j\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> > > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);\n> > > > > + if (itPrimaries != primariesToV4l2.end())\n> > > > > + v4l2Format.colorspace = itPrimaries->second;\n> > > > > + else\n> > > > > + ret = -1;\n> > > > > +\n> > > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding);\n> > > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end())\n> > > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second;\n> > > > > + else\n> > > > > + ret = -1;\n> > > > > +\n> > > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction);\n> > > > > + if (itTransfer != transferFunctionToV4l2.end())\n> > > > > + v4l2Format.xfer_func = itTransfer->second;\n> > > > > + else\n> > > > > + ret = -1;\n> > > > > +\n> > > > > + auto itRange = rangeToV4l2.find(colorSpace->range);\n> > > > > + if (itRange != rangeToV4l2.end())\n> > > > > + v4l2Format.quantization = itRange->second;\n> > > > > + else\n> > > > > + ret = -1;\n> >\n> > Hrm ... I think I've already commented on those, but just in case, I\n> > think these should be -EINVAL.\n> >\n> > We could also have an explicit \\retval too.\n> >\n> > > > > +\n> > > > > + return ret;\n> > > > > +}\n> > > > > +\n> > > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format &);\n> > > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_pix_format_mplane &);\n> > > > > +template int V4L2Device::fromColorSpace(const std::optional &, struct v4l2_mbus_framefmt &);\n> > > > > +\n> > > > > } /* namespace libcamera */\n> > > > > --\n> > > > > 2.20.1\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 38C47BF415\n\tfor ;\n\tThu, 25 Nov 2021 12:14:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9150360376;\n\tThu, 25 Nov 2021 13:14:17 +0100 (CET)","from mail-wr1-x431.google.com (mail-wr1-x431.google.com\n\t[IPv6:2a00:1450:4864:20::431])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D460C60231\n\tfor ;\n\tThu, 25 Nov 2021 13:14:16 +0100 (CET)","by mail-wr1-x431.google.com with SMTP id a9so11168289wrr.8\n\tfor ;\n\tThu, 25 Nov 2021 04:14:16 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kOPN9ZQt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=yZZkJkx81KtgAfsmTPGiM23+73JkTPNPShxhRXC8Kjw=;\n\tb=kOPN9ZQthHLpWgUSjqVHYHQDwLMX1axgDykbt371NwOVE/2Tgym76rQDVidc7sfWCb\n\t+SDO/ZK1vEjJEQre10hZB+cL7ebU3gNG0HzZygjcWKTn1dyOglwGCxoXoDQV3sZIMflp\n\tmMNyXUwBKKGs0MQRUAYnUSAWnnxkXyxUtkWJogAz9ckWEmTZsvvaKzeoRAxYDz0r6yVb\n\t8wtqbSCuYhW4z1A5cwG/i/h98yMKwsBLC8MrvtMnK//pH1CMFGERz4Di6w8zsdkIgm0K\n\tqEI5Zh1UBlw203UWzWYqlnxxpUCPKpvmcn4jQedg68mc1HaWv7ura0Uxn87SXwYwpuHr\n\toL+w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yZZkJkx81KtgAfsmTPGiM23+73JkTPNPShxhRXC8Kjw=;\n\tb=MyKCz0KYUa6nP50w+zTJ9kfwpNd4yx3CGmfyF+5sk3dAOS4epOUVz+ERbFCiGz1rKG\n\tXbvtR9iIAq4LvIj1tfTyfWehnLtBmm0PcHQSrIghIHwQcfuPdbPxp7UsXgXIUyPMdSQg\n\tvqPVCYJ1vGl0Pb1LgqK12VbxzQkslISfLB51rns84VM+9IsOgtvFH7FI06y8eCIBuNq9\n\tLwezhx76Ia9JJysK/LrCouyxosdccVAwUsLjvoNcqBTrtQcrQtnK/ZB/9DwY/pDokA/k\n\ty10NRmwqsfSdstwx7g+6tv6HqvxsF5OCTLbnKpxvass2tcwZ07iMPx4czcDELBnxwVKG\n\t+hLg==","X-Gm-Message-State":"AOAM5338X7DzucACPy7ht1yT4nUMZnzSu3v04D9cc/dZ1hNhZPB9ID8W\n\t+yQrOpaqc7MAfxXv9qyEkJjOLPC7hBt0v08Dcpk5xg==","X-Google-Smtp-Source":"ABdhPJwsHTXkfNIuw5ngRNH3qnE01OlaiCxyaQCRjDY/D2sPF0h1Eutyy3G5ZdXnqTOajutvC88X2t8HRQEYoiwM3yw=","X-Received":"by 2002:adf:f2ca:: with SMTP id d10mr5886876wrp.79.1637842456301;\n\tThu, 25 Nov 2021 04:14:16 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-4-david.plowman@raspberrypi.com>\n\t<20211124160612.zqn5zwunaodu2k5y@uno.localdomain>\n\t\n\t<163784040367.3059017.15937905094444936814@Monstersaurus>\n\t<20211125115405.xngh44ff7fnzlgiy@uno.localdomain>","In-Reply-To":"<20211125115405.xngh44ff7fnzlgiy@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 12:14:05 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 3/7] libcamera: Convert between\n\tColorSpace class and V4L2 formats","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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]