[{"id":24877,"web_url":"https://patchwork.libcamera.org/comment/24877/","msgid":"<20220901035734.GG27075@pyrite.rasen.tech>","date":"2022-09-01T03:57:34","subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: color_space: Move\n\tcolor space adjustment to ColorSpace class","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Tue, Aug 30, 2022 at 01:17:23PM +0530, Umang Jain via libcamera-devel wrote:\n> The CameraConfiguration::validateColorSpaces() function performs color\n> space validation on a camera configuration, by validating the color\n> space of each stream individually, and optionally ensuring that all\n> streams share the same color space. The individual validation is very\n> basic, limited to ensuring that raw formats use a raw color space.\n> \n> Color spaces are more constrained than that:\n> \n> - The Y'CbCr encoding and quantization range for RGB formats must be\n> YcbcrEncoding::None and Range::Full respectively.\n> - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None.\n> \n> Instead of open-coding these constraints in the validateColorSpaces()\n> function, create a new ColorSpace::adjust() function to centralize color\n> space validation and adjustment, and use it in validateColorSpaces().\n> \n> Signed-off-by: Umang Jain \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Umang Jain \n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/color_space.h | 4 ++\n> src/libcamera/camera.cpp | 43 ++++++--------\n> src/libcamera/color_space.cpp | 101 ++++++++++++++++++++++++++++++++\n> 3 files changed, 122 insertions(+), 26 deletions(-)\n> \n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> index f493f72d..6d6c2829 100644\n> --- a/include/libcamera/color_space.h\n> +++ b/include/libcamera/color_space.h\n> @@ -12,6 +12,8 @@\n> \n> namespace libcamera {\n> \n> +class PixelFormat;\n> +\n> class ColorSpace\n> {\n> public:\n> @@ -61,6 +63,8 @@ public:\n> \tstatic std::string toString(const std::optional &colorSpace);\n> \n> \tstatic std::optional fromString(const std::string &str);\n> +\n> +\tbool adjust(PixelFormat format);\n> };\n> \n> bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a5c3aabe..9fe29ca9 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -317,17 +317,6 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n> \n> -namespace {\n> -\n> -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> -} /* namespace */\n> -\n> /**\n> * \\enum CameraConfiguration::ColorSpaceFlag\n> * \\brief Specify the behaviour of validateColorSpaces\n> @@ -368,29 +357,31 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\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 * Set all raw streams to the Raw color space, and make a note of the\n> +\t * largest non-raw stream with a defined color space (if there is one).\n> \t */\n> -\tint index = -1;\n> +\tstd::optional colorSpace;\n> +\n> \tfor (auto [i, cfg] : utils::enumerate(config_)) {\n> -\t\tif (isRaw(cfg.pixelFormat)) {\n> -\t\t\tif (cfg.colorSpace != ColorSpace::Raw) {\n> -\t\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> -\t\t\t\tstatus = Adjusted;\n> -\t\t\t}\n> -\t\t} else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n> -\t\t\tindex = i;\n> -\t\t}\n> +\t\tif (!cfg.colorSpace)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (cfg.colorSpace->adjust(cfg.pixelFormat))\n> +\t\t\tstatus = Adjusted;\n> +\n> +\t\tif (cfg.colorSpace != ColorSpace::Raw &&\n> +\t\t (!colorSpace || cfg.size > config_[i].size))\n> +\t\t\tcolorSpace = cfg.colorSpace;\n> \t}\n> \n> -\tif (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> +\tif (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\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\tif (cfg.colorSpace != ColorSpace::Raw &&\n> +\t\t cfg.colorSpace != colorSpace) {\n> +\t\t\tcfg.colorSpace = colorSpace;\n> \t\t\tstatus = Adjusted;\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> index ec34620a..7356bf7d 100644\n> --- a/src/libcamera/color_space.cpp\n> +++ b/src/libcamera/color_space.cpp\n> @@ -16,6 +16,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/formats.h\"\n> +\n> /**\n> * \\file color_space.h\n> * \\brief Class and enums to represent color spaces\n> @@ -398,6 +400,105 @@ std::optional ColorSpace::fromString(const std::string &str)\n> \treturn colorSpace;\n> }\n> \n> +/**\n> + * \\brief Adjust the color space to match a pixel format\n> + * \\param[in] format The pixel format\n> + *\n> + * Not all combinations of pixel formats and color spaces make sense. For\n> + * instance, nobody uses a limited quantization range with raw Bayer formats,\n> + * and the YcbcrEncoding::None encoding isn't valid for YUV formats. This\n> + * function adjusts the ColorSpace to make it compatible with the given \\a\n> + * format, by applying the following rules:\n> + *\n> + * - The color space for RAW formats must be Raw.\n> + * - The Y'CbCr encoding and quantization range for RGB formats must be\n> + * YcbcrEncoding::None and Range::Full respectively.\n> + * - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None. The\n> + * best encoding is in that case guessed based on the primaries and transfer\n> + * function.\n> + *\n> + * \\return True if the color space has been adjusted, or false if it was\n> + * already compatible with the format and hasn't been changed\n> + */\n> +bool ColorSpace::adjust(PixelFormat format)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\tbool adjusted = false;\n> +\n> +\tswitch (info.colourEncoding) {\n> +\tcase PixelFormatInfo::ColourEncodingRAW:\n> +\t\t/* Raw formats must use the raw color space. */\n> +\t\tif (*this != ColorSpace::Raw) {\n> +\t\t\t*this = ColorSpace::Raw;\n> +\t\t\tadjusted = true;\n> +\t\t}\n> +\t\tbreak;\n> +\n> +\tcase PixelFormatInfo::ColourEncodingRGB:\n> +\t\t/*\n> +\t\t * RGB formats can't have a Y'CbCr encoding, and must use full\n> +\t\t * range quantization.\n> +\t\t */\n> +\t\tif (ycbcrEncoding != YcbcrEncoding::None) {\n> +\t\t\tycbcrEncoding = YcbcrEncoding::None;\n> +\t\t\tadjusted = true;\n> +\t\t}\n> +\n> +\t\tif (range != Range::Full) {\n> +\t\t\trange = Range::Full;\n> +\t\t\tadjusted = true;\n> +\t\t}\n> +\t\tbreak;\n> +\n> +\tcase PixelFormatInfo::ColourEncodingYUV:\n> +\t\tif (ycbcrEncoding != YcbcrEncoding::None)\n> +\t\t\tbreak;\n> +\n> +\t\t/*\n> +\t\t * YUV formats must have a Y'CbCr encoding. Infer the most\n> +\t\t * probable option from the transfer function and primaries.\n> +\t\t */\n> +\t\tswitch (transferFunction) {\n> +\t\tcase TransferFunction::Linear:\n> +\t\t\t/*\n> +\t\t\t * Linear YUV is not used in any standard color space,\n> +\t\t\t * pick the widely supported and used Rec601 as default.\n> +\t\t\t */\n> +\t\t\tycbcrEncoding = YcbcrEncoding::Rec601;\n> +\t\t\tbreak;\n> +\n> +\t\tcase TransferFunction::Rec709:\n> +\t\t\tswitch (primaries) {\n> +\t\t\t/* Raw should never happen. */\n> +\t\t\tcase Primaries::Raw:\n> +\t\t\tcase Primaries::Smpte170m:\n> +\t\t\t\tycbcrEncoding = YcbcrEncoding::Rec601;\n> +\t\t\t\tbreak;\n> +\t\t\tcase Primaries::Rec709:\n> +\t\t\t\tycbcrEncoding = YcbcrEncoding::Rec709;\n> +\t\t\t\tbreak;\n> +\t\t\tcase Primaries::Rec2020:\n> +\t\t\t\tycbcrEncoding = YcbcrEncoding::Rec2020;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\n> +\t\tcase TransferFunction::Srgb:\n> +\t\t\t/*\n> +\t\t\t * Only the sYCC color space uses the sRGB transfer\n> +\t\t\t * function, the corresponding encoding is Rec601.\n> +\t\t\t */\n> +\t\t\tycbcrEncoding = YcbcrEncoding::Rec601;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tadjusted = true;\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn adjusted;\n> +}\n> +\n> /**\n> * \\brief Compare color spaces for equality\n> * \\return True if the two color spaces are identical, false otherwise\n> -- \n> 2.37.2\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 E2F34C0DA4\n\tfor ;\n\tThu, 1 Sep 2022 03:57:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CB8561FC9;\n\tThu, 1 Sep 2022 05:57:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BFC961F9A\n\tfor ;\n\tThu, 1 Sep 2022 05:57:51 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F4059481;\n\tThu, 1 Sep 2022 05:57:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662004672;\n\tbh=4INeO/ZJ5KXzd7DE9TnEjv2l6kniToxChFKxvQZisbg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=U7cT2AXzueNW9QQA1R1NW7wwet1omLRRk6uPhSn7jg89fqkGzY4w5M7W3NdXb6l7D\n\tisRIFTWz0J6gw7WsgRO8CAhNeo/mLVVEZgZEWjva4udlyBAMiYF5xGYWwIU4vZ5CE6\n\tsT/m4lDp/t3SmAvOM6fcNHU5pXFEzx+sCSeMYbf3LqS1yLMf39kmJk7oQUDGmK9KZt\n\tzOqWg9oAXqlE6a1Pi5bp7Q6ly3Dnejiidd1sVJgsqQaR/xcoAIbTrQCSfkbbapYkFB\n\tiZEx6aYUr00dZYbdBP3Ka16beG5mrwbBDdkgc5BS4LiSCfuUciScfL1sbV088mjigK\n\tp+5ztVvFqYQtw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662004671;\n\tbh=4INeO/ZJ5KXzd7DE9TnEjv2l6kniToxChFKxvQZisbg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AqfBzX/1AbPB+vWLIFvHam1GUvAebNYy7Cnh/Do7wttAgH34Tf39JQl3Yg75qmIFW\n\tIArtxoSCR9U0+EvglblgFU+L8eBLF8wnidW1x9xjzSpNlO8kbok/BoubS4dN54Leyd\n\tClh957gP/iGAzAIHkLHmWxA+HgniEErpFJjQFyLQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AqfBzX/1\"; dkim-atps=neutral","Date":"Wed, 31 Aug 2022 23:57:34 -0400","To":"Umang Jain ","Message-ID":"<20220901035734.GG27075@pyrite.rasen.tech>","References":"<20220830074725.1059643-1-umang.jain@ideasonboard.com>\n\t<20220830074725.1059643-6-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220830074725.1059643-6-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: color_space: Move\n\tcolor space adjustment to ColorSpace 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","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]