[{"id":21603,"web_url":"https://patchwork.libcamera.org/comment/21603/","msgid":"<20211206111950.ema2kuboe6gao2cu@uno.localdomain>","date":"2021-12-06T11:19:50","subject":"Re: [libcamera-devel] [PATCH v9 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n\nOn Mon, Dec 06, 2021 at 10:50:30AM +0000, David Plowman wrote:\n> This method forces raw streams to have the \"raw\" color space, and also\n> optionally makes all output streams to share the same color space as\n> some platforms may require this.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 59 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 61 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index a7759ccb..fdcb66ab 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -69,6 +69,8 @@ public:\n> protected:\n> \tCameraConfiguration();\n>\n> +\tStatus validateColorSpaces(bool shareOutputColorSpaces);\n> +\n> \tstd::vector config_;\n> };\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..388af4f7 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -14,12 +14,14 @@\n> #include \n> #include \n>\n> +#include \n> #include \n> #include \n> #include \n>\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> /**\n> @@ -314,6 +316,63 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n>\n> +static bool isRaw(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.isValid() &&\n> +\t info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n> +\n> +/**\n> + * \\brief Check and update the color spaces requested for each stream\n> + * \\param[in] shareOutputColorSpaces Force all non-raw output streams to share\n> + * the same color space\n> + *\n> + * This function performs certain consistency checks on the color spaces of\n> + * the streams and may adjust them so that:\n> + *\n> + * - Any raw streams have the Raw color space\n> + * - If shareOutputColorSpaces is set, all output streams are forced to share\n> + * the same color space (this may be a constraint on some platforms).\n> + *\n> + * It is optional for a pipeline handler to use this method.\n> + *\n> + * \\return A CameraConfiguration::Status value that describes the validation\n> + * status (same as CameraConfiguration::validate).\n> + */\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces)\n> +{\n> +\tStatus status = Valid;\n> +\n> +\t/*\n> +\t * Set all raw streams to the Raw color space, and make a note of the largest\n> +\t * non-raw stream with a defined color space (if there is one).\n> +\t */\n> +\tint index = -1;\n> +\tfor (auto [i, cfg] : utils::enumerate(config_)) {\n> +\t\tif (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t\telse if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))\n\nNote for when applying:\n\n if () {\n\n } else if {\n\n }\n> +\t\t\tindex = i;\n> +\t}\n> +\n> +\tif (index < 0 || !shareOutputColorSpaces)\n> +\t\treturn status;\n> +\n> +\t/* Make all output color spaces the same, if requested. */\n> +\tfor (auto &cfg : config_) {\n> +\t\tif (!isRaw(cfg.pixelFormat) &&\n> +\t\t cfg.colorSpace != config_[index].colorSpace) {\n> +\t\t\tcfg.colorSpace = config_[index].colorSpace;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image\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 9AC15BDB13\n\tfor ;\n\tMon, 6 Dec 2021 11:19:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B18226086C;\n\tMon, 6 Dec 2021 12:19:01 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A760C60725\n\tfor ;\n\tMon, 6 Dec 2021 12:19:00 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 59636240008;\n\tMon, 6 Dec 2021 11:18:58 +0000 (UTC)"],"Date":"Mon, 6 Dec 2021 12:19:50 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211206111950.ema2kuboe6gao2cu@uno.localdomain>","References":"<20211206105032.13876-1-david.plowman@raspberrypi.com>\n\t<20211206105032.13876-8-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211206105032.13876-8-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v9 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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":21637,"web_url":"https://patchwork.libcamera.org/comment/21637/","msgid":"","date":"2021-12-07T13:52:28","subject":"Re: [libcamera-devel] [PATCH v9 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Mon, Dec 06, 2021 at 10:50:30AM +0000, David Plowman wrote:\n> This method forces raw streams to have the \"raw\" color space, and also\n\ns/method/function/ (same below)\n\n> optionally makes all output streams to share the same color space as\n> some platforms may require this.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 59 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 61 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index a7759ccb..fdcb66ab 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -69,6 +69,8 @@ public:\n> protected:\n> \tCameraConfiguration();\n> \n> +\tStatus validateColorSpaces(bool shareOutputColorSpaces);\n> +\n\nI'm not a big fan of boolean parameters for this kind of use case, as\nit's not clear what they mean when you look at the callers. Explicit\nflags are better:\n\n\tenum class ColorSpaceFlag {\n\t\tNone,\n\t\tStreamsShareColorSpace,\t-- or a better name\n\t};\n\n\tusing ColorSpaceFlags = Flags;\n\n\tStatus validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None);\n\nIt may be overkill to use the Flags class for a single flag, the enum\nalone would be enough.\n\n> \tstd::vector config_;\n> };\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..388af4f7 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -14,12 +14,14 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> /**\n> @@ -314,6 +316,63 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n> \n> +static bool isRaw(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.isValid() &&\n> +\t info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n\nnamespace {\n\nbool isRaw(const PixelFormat &pixFmt)\n{\n\t...\n}\n\n} /* namespace */\n\nwould be the C++ way.\n\n> +\n> +/**\n> + * \\brief Check and update the color spaces requested for each stream\n> + * \\param[in] shareOutputColorSpaces Force all non-raw output streams to share\n> + * the same color space\n> + *\n> + * This function performs certain consistency checks on the color spaces of\n> + * the streams and may adjust them so that:\n> + *\n> + * - Any raw streams have the Raw color space\n> + * - If shareOutputColorSpaces is set, all output streams are forced to share\n> + * the same color space (this may be a constraint on some platforms).\n> + *\n> + * It is optional for a pipeline handler to use this method.\n> + *\n> + * \\return A CameraConfiguration::Status value that describes the validation\n> + * status (same as CameraConfiguration::validate).\n> + */\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces)\n> +{\n> +\tStatus status = Valid;\n> +\n> +\t/*\n> +\t * Set all raw streams to the Raw color space, and make a note of the largest\n> +\t * non-raw stream with a defined color space (if there is one).\n> +\t */\n> +\tint index = -1;\n> +\tfor (auto [i, cfg] : utils::enumerate(config_)) {\n> +\t\tif (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t\telse if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))\n\nAs Jacopo said :-)\n\nAny particular reason to go for the largest stream ?\n\n> +\t\t\tindex = i;\n> +\t}\n> +\n> +\tif (index < 0 || !shareOutputColorSpaces)\n> +\t\treturn status;\n> +\n> +\t/* Make all output color spaces the same, if requested. */\n> +\tfor (auto &cfg : config_) {\n> +\t\tif (!isRaw(cfg.pixelFormat) &&\n> +\t\t cfg.colorSpace != config_[index].colorSpace) {\n> +\t\t\tcfg.colorSpace = config_[index].colorSpace;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image","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 D22D7BF415\n\tfor ;\n\tTue, 7 Dec 2021 13:52:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21C716086A;\n\tTue, 7 Dec 2021 14:52:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1220660592\n\tfor ;\n\tTue, 7 Dec 2021 14:52:58 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57F32556;\n\tTue, 7 Dec 2021 14:52:57 +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=\"tXnJ5b2S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638885177;\n\tbh=agxAu9qyJxFtVi0EHs7+xvgJqiEU2uIbgy81XgCcoIg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tXnJ5b2SstIshjWR0sOjwYbYWF8OemxOD96gfk8OBzlw7P0VrT/qT9NjBUzLO7fI0\n\tUteMa4IgieMhcdis49qfjBOO2Zrke/768Xky04NyFmITwBAJaC1mwGh8E0X3+d1+IY\n\txYfgYnDkn4YiCgFVs0EznDJ5WjcqUEEtznQl8U/8=","Date":"Tue, 7 Dec 2021 15:52:28 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20211206105032.13876-1-david.plowman@raspberrypi.com>\n\t<20211206105032.13876-8-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211206105032.13876-8-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v9 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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\" "}}]