[{"id":24589,"web_url":"https://patchwork.libcamera.org/comment/24589/","msgid":"","date":"2022-08-16T02:32:25","subject":"Re: [libcamera-devel] [RFC PATCH 2/4] libcamera: colorspace: Adjust\n\tcolorspace of YUV streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Aug 03, 2022 at 12:27:17AM +0530, Umang Jain via libcamera-devel wrote:\n> If a YUV stream is requested with no Y'Cbcr encoding, we need to adjust\n> it to provide a Y'Cbcr encoding. Depending on the transfer function and\n> primaries specified, a Y'Cbcr encoding is picked up.\n> \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/color_space.h | 6 ++++\n> src/libcamera/camera.cpp | 11 ++++++\n> src/libcamera/color_space.cpp | 61 +++++++++++++++++++++++++++++++++\n> 3 files changed, 78 insertions(+)\n> \n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> index 0d39fbc0..113e2715 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> +struct StreamConfiguration;\n> +\n> class ColorSpace\n> {\n> public:\n> @@ -59,6 +61,10 @@ public:\n> \n> \tstd::string toString() const;\n> \tstatic std::string toString(const std::optional &colorSpace);\n> +\tColorSpace adjust(const StreamConfiguration &cfg);\n\nThis function is meant to be used internally inside libcamera, I would\nprefer to avoid exposing it in the public API.\n\n> +\n> +private:\n> +\tColorSpace adjustYuv(const StreamConfiguration &cfg);\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 3910915c..05135d16 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -386,6 +386,17 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> \tif (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> \t\treturn status;\n> \n> +\t/*\n> +\t * \\todo Question StreamsShareColorSpace <> an adjusted colorspace\n> +\t * interation.\n> +\t */\n> +\tif (config_[index].colorSpace) {\n> +\t\tColorSpace oldColorspace = config_[index].colorSpace.value();\n> +\t\tColorSpace newColorspace = oldColorspace.adjust(config_[index]);\n> +\t\tif (oldColorspace != newColorspace)\n> +\t\t\tconfig_[index].colorSpace = newColorspace;\n\nDon't you need to set status to Adjusted here ?\n\n> +\t}\n\nThe color space needs to be adjusted for every stream, even in the\nnon-shared color space case.\n\n> +\n> \t/* Make all output color spaces the same, if requested. */\n\nI think we'll need to extend this in the future, with additional flags\nto tell if the full color space is shared, or only part of it. The color\nprimaries and transfer functions are typically configured in the ISP and\nshared between streams, but the quantization range (and perhaps even the\nYCbCr encoding) can possibly differ between ISP outputs. Let's address\nthat later if the need arises.\n\n> \tfor (auto &cfg : config_) {\n> \t\tif (!isRaw(cfg.pixelFormat) &&\n\nI would split the change to this file to a separate patch as it will\ngrow bigger.\n\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> index 73148228..ff3f76d9 100644\n> --- a/src/libcamera/color_space.cpp\n> +++ b/src/libcamera/color_space.cpp\n> @@ -13,6 +13,10 @@\n> #include \n> #include \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> @@ -193,6 +197,63 @@ std::string ColorSpace::toString() const\n> \treturn ss.str();\n> }\n> \n> +/**\n> + * \\brief Adjust colospace when stream configuration contains YUV stream\n> + * \\param[in] config Stream configuration\n> + *\n> + * This function adjust the stream's colorspace if it consists a YUV stream\n> + * and has no Y'Cbcr encoding specified. The function shall update the\n> + * Y'Cbcr encoding based on the transfer function and primaries fields.\n> + *\n> + * \\return The adjusted colorspace\n> + */\n> +ColorSpace\n> +ColorSpace::adjustYuv(const StreamConfiguration &cfg)\n> +{\n> +\tColorSpace cs = *this;\n> +\tbool isYUV = (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> +\t\t PixelFormatInfo::ColourEncodingYUV);\n> +\n> +\tif (isYUV && cs.ycbcrEncoding == YcbcrEncoding::None) {\n> +\t\tif (cs.transferFunction == TransferFunction::Rec709) {\n> +\t\t\tswitch (cs.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\tcs.ycbcrEncoding = YcbcrEncoding::Rec601;\n> +\t\t\t\tbreak;\n> +\t\t\tcase Primaries::Rec709:\n> +\t\t\t\tcs.ycbcrEncoding = YcbcrEncoding::Rec709;\n> +\t\t\t\tbreak;\n> +\t\t\tcase Primaries::Rec2020:\n> +\t\t\t\tcs.ycbcrEncoding = YcbcrEncoding::Rec2020;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} else if (cs.transferFunction == TransferFunction::Srgb) {\n> +\t\t\tcs.ycbcrEncoding = YcbcrEncoding::Rec601;\n> +\t\t}\n> +\n> +\t\t/* \\todo: Determine if range needs to be adjusted in some cases? */\n\nAny opinion ?\n\n> +\t}\n> +\n> +\treturn cs;\n> +}\n> +\n> +/**\n> + * \\brief Adjust the colorspace depending on the stream configuration\n> + * \\param[in] config Stream configuration\n> + *\n> + * This function adjust the stream's colorspace depending on various factors\n> + * as reflected by the \\a config.\n> + *\n> + * \\return The adjusted colorspace according to the stream configuration\n\nGiven the function name, and the fact the function isn't const, I would\nhave guessed that it adjusts the color space in-place. This is\nerror-prone, someone could call the function and ignore the return\nvalue. One option is to make this a const function and mark the return\nvalue as [[nodiscard]], but I think it would be better, given the\nfunction name, to adjust the color space in-place.\n\n> + */\n> +ColorSpace\n> +ColorSpace::adjust(const StreamConfiguration &config)\n> +{\n> +\treturn adjustYuv(config);\n> +}\n\nWhy is this split in two functions ?\n\n> +\n> /**\n> * \\brief Assemble and return a readable string representation of an\n> * optional ColorSpace","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 3193AC3272\n\tfor ;\n\tTue, 16 Aug 2022 02:32:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 572DE61FC0;\n\tTue, 16 Aug 2022 04:32:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDEF2603E3\n\tfor ;\n\tTue, 16 Aug 2022 04:32:38 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5692B496;\n\tTue, 16 Aug 2022 04:32:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660617160;\n\tbh=ICd5pMxrTRJ7yQb/qEzVAU5EI//yCxF9gmC4B+3oa8Y=;\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=vaK+eDgj4LBfVnOEufsW/YprxWutZ9Ev3BHhO3qSb99Z9DB05H7RJhXAGfhu8yiwI\n\tlVLNOzWYhK7GMSzFep2ohAcgGuOfu2mH/LvDAnb8p88NJAcAiB1cuuARfTkqO6wYCu\n\tsLIZv1SXJ+w2nV63XiKzSH3LBUztEb6snV08GivSGhhpAfPVndiUVMCelCdNHwNUvF\n\tp0KOLw9FJvdRwH9sNFmP/9OCq7qCTGs8RBqAh4T/760x6vr6LULPaOK1AyCZuMwqja\n\tFWsn/BTDX582fb+2aQDFr5Z70Vdf5WeAbe9KeJV+N74I8YSH+OOeK53friJhXZFi05\n\tPrMjtW2HwE6Jw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660617158;\n\tbh=ICd5pMxrTRJ7yQb/qEzVAU5EI//yCxF9gmC4B+3oa8Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V9YlWoB891HdleR8p9g7cOiBhO6K5JsU20+NC5QGb5bZdQj5kveiQhmGNEEYs2NsX\n\tEznPk7UuufDWsS+vhvu4NozFXWKKn6QG05CT6GzQurn02cOujFXAg0ZKc70WaWnwV0\n\tCaKfjJSDr5jWUamdqoSo969nPVRra2IGVuTUqDJI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"V9YlWoB8\"; dkim-atps=neutral","Date":"Tue, 16 Aug 2022 05:32:25 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220802185719.380855-1-umang.jain@ideasonboard.com>\n\t<20220802185719.380855-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220802185719.380855-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/4] libcamera: colorspace: Adjust\n\tcolorspace of YUV streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]