[{"id":26303,"web_url":"https://patchwork.libcamera.org/comment/26303/","msgid":"<167423342874.42371.4548256991574240321@Monstersaurus>","date":"2023-01-20T16:50:28","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman via libcamera-devel (2023-01-20 16:22:58)\n> We implement a custom validateColorSpaces method that forces all\n> (non-raw) streams to same colour space, whilst distinguishing RGB\n> streams from YUV ones, as the former must have the YCbCr encoding and\n> range over-written.\n> \n> When we apply the colour space, we always send the full YUV version as\n> that gets converted correctly to what our hardware drivers expect. It\n> is also careful to check what comes back as the YCbCr information gets\n> overwritten again on the way back.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> 1 file changed, 114 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17..f768dced 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration\n> public:\n> RPiCameraConfiguration(const RPiCameraData *data);\n> \n> + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);\n> Status validate() override;\n> \n> /* Cache the combinedTransform_ that will be applied to the sensor */\n> @@ -317,6 +318,13 @@ public:\n> \n> private:\n> const RPiCameraData *data_;\n> +\n> + /*\n> + * Store the colour spaces that all our streams will have. RGB format streams\n> + * will be the same but need the YCbCr fields cleared.\n> + */\n> + std::optional yuvColorSpace_;\n> + std::optional rgbColorSpace_;\n> };\n> \n> class PipelineHandlerRPi : public PipelineHandler\n> @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> {\n> }\n> \n> +static const std::vector validColorSpaces = {\n> + ColorSpace::Sycc,\n> + ColorSpace::Smpte170m,\n> + ColorSpace::Rec709\n> +};\n> +\n> +static std::optional findValidColorSpace(const ColorSpace &colourSpace)\n> +{\n> + for (auto cs : validColorSpaces) {\n> + if (colourSpace.primaries == cs.primaries &&\n> + colourSpace.transferFunction == cs.transferFunction)\n> + return cs;\n> + }\n> +\n> + return std::nullopt;\n> +}\n> +\n> +static bool isRgb(const PixelFormat &pixFmt)\n> +{\n> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> +}\n> +\n> +static bool isYuv(const PixelFormat &pixFmt)\n> +{\n> + /* The code below would return true for raw mono streams, so weed those out first. */\n> + if (isRaw(pixFmt))\n> + return false;\n> +\n> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> +}\n> +\n> +/*\n> + * Raspberry Pi drivers expect the following colour spaces:\n> + * - V4L2_COLORSPACE_RAW for raw streams.\n> + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for\n> + * non-raw streams. Other fields such as transfer function, YCbCr encoding and\n> + * quantisation are not used.\n> + *\n> + * The libcamera colour spaces that we wish to use corresponding to these are therefore:\n> + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> + */\n> +\n> +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)\n> +{\n> + Status status = Valid;\n> + yuvColorSpace_.reset();\n> +\n> + for (auto cfg : config_) {\n> + /* First fix up raw streams to have the \"raw\" colour space. */\n> + if (isRaw(cfg.pixelFormat)) {\n> + /* If there was no value here, that doesn't count as \"adjusted\". */\n> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {\n> + status = Adjusted;\n> + cfg.colorSpace = ColorSpace::Raw;\n> + }\n> + continue;\n> + }\n> +\n> + /* Next we need to find our shared colour space. The first valid one will do. */\n> + if (cfg.colorSpace && !yuvColorSpace_)\n> + yuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());\n> + }\n> +\n> + /* If no colour space was given anywhere, choose sYCC. */\n> + if (!yuvColorSpace_)\n> + yuvColorSpace_ = ColorSpace::Sycc;\n> +\n> + /* Note the version of this that any RGB streams will have to use. */\n> + rgbColorSpace_ = yuvColorSpace_;\n> + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> + rgbColorSpace_->range = ColorSpace::Range::Full;\n> +\n> + /* Go through the streams again and force everyone to the same colour space. */\n> + for (auto cfg : config_) {\n> + if (cfg.colorSpace == ColorSpace::Raw)\n> + continue;\n> +\n> + if (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {\n> + /* Again, no value means \"not adjusted\". */\n> + if (cfg.colorSpace)\n> + status = Adjusted;\n> + cfg.colorSpace = yuvColorSpace_;\n> + }\n\nNit blank line? Or is this hugging of two scopes intentional?\n\n> + if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {\n> + /* Be nice, and let the YUV version count as non-adjusted too. */\n\nI do wonder if we should update the Adjusted to a set of Flags that say\n'what' has been adjusted instead ... Then an application can identify if\nit cares about the change or not.\n\nBut - I don't think that matters to this patch.\n\nThe fact we're making small adjustments but not reporting to the\napplication may worry me a little, but I expect if this passes your\nmetric for worrying about colorspace, I shouldn't worry, as you're bar\nmust be higher than mine!\n\nSo I'd say:\n\nReviewed-by: Kieran Bingham \n\n\n> + if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)\n> + status = Adjusted;\n> + cfg.colorSpace = rgbColorSpace_;\n> + }\n> + }\n> +\n> + return status;\n> +}\n> +\n> CameraConfiguration::Status RPiCameraConfiguration::validate()\n> {\n> Status status = Valid;\n> @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> V4L2DeviceFormat format;\n> format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> format.size = cfg.size;\n> - format.colorSpace = cfg.colorSpace;\n> + /* We want to send the associated YCbCr info through to the driver. */\n> + format.colorSpace = yuvColorSpace_;\n> \n> LOG(RPI, Debug)\n> << \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> if (ret)\n> return Invalid;\n> \n> + /*\n> + * But for RGB streams, the YCbCr info gets overwritten on the way back\n> + * so we must check against what the stream cfg says, not what we actually\n> + * requested (which carefully included the YCbCr info)!\n> + */\n> if (cfg.colorSpace != format.colorSpace) {\n> status = Adjusted;\n> LOG(RPI, Debug)\n> -- \n> 2.30.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 9C345C3240\n\tfor ;\n\tFri, 20 Jan 2023 16:50:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C08FD625E4;\n\tFri, 20 Jan 2023 17:50:33 +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 44F2B61EFD\n\tfor ;\n\tFri, 20 Jan 2023 17:50:32 +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 C130D514;\n\tFri, 20 Jan 2023 17:50:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674233433;\n\tbh=xc11FALii9xwY1jgtVqd8osuoWdcd/pM+Y0agJ00kuI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=X4mjs+vcSjsmlgAYlxB4gX0EX6UYRSGUj5+BRlpQKjW8LcA4ZaNOffcIpmXf35yRW\n\t3eEbIzMZ+iuZa+SoSLKPdcdHmV2idThgPUlWpulOSPsm7t/u0jt3VDTMFLPB2dvw/I\n\tuUjx1H46f9Dp24xsppRNvfaZxC9C31fwCKxSYAnwlwHA7H8DZDG7p2zubi6xWTrcMJ\n\tS5AFbcakEcQs76vHIuvO7sWSMfAhKqiZ42Htx0qtqqyhEtB3i9e+8Vplt6Ljeogc+z\n\twrPHxkJ2RXgmQ04QjhhF8uF47+n+CfrKlM9JxdtBhKWJysOUEKt5ZPhUX545ZLoHmt\n\tdQ5sqi/0naSEg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674233431;\n\tbh=xc11FALii9xwY1jgtVqd8osuoWdcd/pM+Y0agJ00kuI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=H3Tho4qk40HRfZYE5S0LrJ606R+h1YY3CQnotUqtoprW0f0VkoLMigeVuXkLpAElb\n\tT1L0rdKjUOsEHu8EZIRH1/splQ6HOESmuFRmZ7tOj7R0C3GN+fenskXmi6QjqP3igf\n\tyEjlIZ5huPfrRQT1Ymk11ZrIRUQ807tUoVsa4ed0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"H3Tho4qk\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230120162258.7039-2-david.plowman@raspberrypi.com>","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 20 Jan 2023 16:50:28 +0000","Message-ID":"<167423342874.42371.4548256991574240321@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26345,"web_url":"https://patchwork.libcamera.org/comment/26345/","msgid":"<45271050-d93d-c8de-b8d7-bb289cf406a4@ideasonboard.com>","date":"2023-01-25T06:48:19","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOn 1/20/23 9:52 PM, David Plowman via libcamera-devel wrote:\n> We implement a custom validateColorSpaces method that forces all\n> (non-raw) streams to same colour space, whilst distinguishing RGB\n> streams from YUV ones, as the former must have the YCbCr encoding and\n> range over-written.\n>\n> When we apply the colour space, we always send the full YUV version as\n> that gets converted correctly to what our hardware drivers expect. It\n> is also careful to check what comes back as the YCbCr information gets\n> overwritten again on the way back.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> 1 file changed, 114 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17..f768dced 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration\n> public:\n> \tRPiCameraConfiguration(const RPiCameraData *data);\n> \n> +\tCameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);\n> \tStatus validate() override;\n> \n> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n> @@ -317,6 +318,13 @@ public:\n> \n> private:\n> \tconst RPiCameraData *data_;\n> +\n> +\t/*\n> +\t * Store the colour spaces that all our streams will have. RGB format streams\n> +\t * will be the same but need the YCbCr fields cleared.\n\nWould re-pharse it like:\n\n         /*\n          * Store the colour spaces that all our streams will have. RGB \nformat streams\n          * will have the same colorspace as YUV streams, with YCbCr \nfield cleared and\n          * range set to full.\n          */\n\n> +\t */\n> +\tstd::optional yuvColorSpace_;\n> +\tstd::optional rgbColorSpace_;\n> };\n> \n> class PipelineHandlerRPi : public PipelineHandler\n> @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> {\n> }\n> \n> +static const std::vector validColorSpaces = {\n> +\tColorSpace::Sycc,\n> +\tColorSpace::Smpte170m,\n> +\tColorSpace::Rec709\n> +};\n> +\n> +static std::optional findValidColorSpace(const ColorSpace &colourSpace)\n> +{\n> +\tfor (auto cs : validColorSpaces) {\n> +\t\tif (colourSpace.primaries == cs.primaries &&\n> +\t\t colourSpace.transferFunction == cs.transferFunction)\n> +\t\t\treturn cs;\n> +\t}\n> +\n> +\treturn std::nullopt;\n> +}\n> +\n> +static bool isRgb(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> +}\n> +\n> +static bool isYuv(const PixelFormat &pixFmt)\n> +{\n> +\t/* The code below would return true for raw mono streams, so weed those out first. */\n> +\tif (isRaw(pixFmt))\n> +\t\treturn false;\n> +\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> +}\n> +\n> +/*\n> + * Raspberry Pi drivers expect the following colour spaces:\n> + * - V4L2_COLORSPACE_RAW for raw streams.\n> + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for\n> + * non-raw streams. Other fields such as transfer function, YCbCr encoding and\n> + * quantisation are not used.\n> + *\n> + * The libcamera colour spaces that we wish to use corresponding to these are therefore:\n> + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> + */\n\nThank you for the information. This makes the expectations clearer.\n\nWith that,\n\nReviewed-by: Umang Jain \n\n\nI can fixup the comment while applying the patch (if you are OK with the \ncomment's explanation).\n\n> +\n> +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)\n> +{\n> +\tStatus status = Valid;\n> +\tyuvColorSpace_.reset();\n> +\n> +\tfor (auto cfg : config_) {\n> +\t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> +\t\tif (isRaw(cfg.pixelFormat)) {\n> +\t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> +\t\t\tif (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> +\t\t\t}\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* Next we need to find our shared colour space. The first valid one will do. */\n> +\t\tif (cfg.colorSpace && !yuvColorSpace_)\n> +\t\t\tyuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());\n> +\t}\n> +\n> +\t/* If no colour space was given anywhere, choose sYCC. */\n> +\tif (!yuvColorSpace_)\n> +\t\tyuvColorSpace_ = ColorSpace::Sycc;\n> +\n> +\t/* Note the version of this that any RGB streams will have to use. */\n> +\trgbColorSpace_ = yuvColorSpace_;\n> +\trgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> +\trgbColorSpace_->range = ColorSpace::Range::Full;\n> +\n> +\t/* Go through the streams again and force everyone to the same colour space. */\n> +\tfor (auto cfg : config_) {\n> +\t\tif (cfg.colorSpace == ColorSpace::Raw)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {\n> +\t\t\t/* Again, no value means \"not adjusted\". */\n> +\t\t\tif (cfg.colorSpace)\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\tcfg.colorSpace = yuvColorSpace_;\n> +\t\t}\n> +\t\tif (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {\n> +\t\t\t/* Be nice, and let the YUV version count as non-adjusted too. */\n> +\t\t\tif (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\tcfg.colorSpace = rgbColorSpace_;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> CameraConfiguration::Status RPiCameraConfiguration::validate()\n> {\n> \tStatus status = Valid;\n> @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\tV4L2DeviceFormat format;\n> \t\tformat.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> \t\tformat.size = cfg.size;\n> -\t\tformat.colorSpace = cfg.colorSpace;\n> +\t\t/* We want to send the associated YCbCr info through to the driver. */\n> +\t\tformat.colorSpace = yuvColorSpace_;\n> \n> \t\tLOG(RPI, Debug)\n> \t\t\t<< \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\tif (ret)\n> \t\t\treturn Invalid;\n> \n> +\t\t/*\n> +\t\t * But for RGB streams, the YCbCr info gets overwritten on the way back\n> +\t\t * so we must check against what the stream cfg says, not what we actually\n> +\t\t * requested (which carefully included the YCbCr info)!\n> +\t\t */\n> \t\tif (cfg.colorSpace != format.colorSpace) {\n> \t\t\tstatus = Adjusted;\n> \t\t\tLOG(RPI, Debug)","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 7412DBEFBE\n\tfor ;\n\tWed, 25 Jan 2023 06:50:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90C36625D8;\n\tWed, 25 Jan 2023 07:50:04 +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 C23FC600FE\n\tfor ;\n\tWed, 25 Jan 2023 07:50:02 +0100 (CET)","from [IPV6:2409:4080:9407:cd8:f9f0:1364:cec8:4ae2] (unknown\n\t[IPv6:2409:4080:9407:cd8:f9f0:1364:cec8:4ae2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91A516E0;\n\tWed, 25 Jan 2023 07:49:24 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674629404;\n\tbh=uV3tWgweSOWOsMw0WzOBxLgQ9g80obRc826C9CjXKBA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=p4YXpKug9psHBuhyGNC86XSUcb6azjez0yomzCwbnPtP4AJHS223x6lJAiSpwY5Xr\n\to5TNBL7BInCouruiU/a4IF/66jCOdU0nyMzZysPsDondeb/Gw1exBO0TjXwg72A4kk\n\tyJN8S1LCNOam6Riy4ppM8l5nOMIoLAqOeGcyrHujJBzAZjBgYt912h6An1WbQ6eTxh\n\tMnI059CrUPWRbzy9ZteHemZSsWGSxSZ5n4AOvLm5zhDdoO21PTqtm95lXxhhmIfgvh\n\top/tyzU0zfrAdJ5PeOxByxIQBP4nOwU2/7t5MB2UqOZSstfVpD5f3np6lyK1VRdJTZ\n\thFJGM+y6ea0aQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674629402;\n\tbh=uV3tWgweSOWOsMw0WzOBxLgQ9g80obRc826C9CjXKBA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=gUvs1eJuhyeCV2dTsqWYSBmiU+fExrUIzzQ0rj94bmsrvLELE5qCvmLMLSmt8Cxcm\n\t83hYXeIbfOevllZ7DWqKLdBxU7AWQkAWrQbgQ9ii5SJZUF/kTT0wHNOZAxqR8o2NQN\n\tVwdeqjfoXWTN3QhyXO0DrmRiAPzDE8NBtIA629SI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gUvs1eJu\"; dkim-atps=neutral","Message-ID":"<45271050-d93d-c8de-b8d7-bb289cf406a4@ideasonboard.com>","Date":"Wed, 25 Jan 2023 12:18:19 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.6.0","Content-Language":"en-US","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20230120162258.7039-2-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26367,"web_url":"https://patchwork.libcamera.org/comment/26367/","msgid":"<167507664392.42371.13710079184268773117@Monstersaurus>","date":"2023-01-30T11:04:03","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nThere's a small question below that is holding this back from being merged.\n\nI'm planning a release tomorrow, so I'd like to get this colourspace fix\nin.\n\nQuoting Umang Jain via libcamera-devel (2023-01-25 06:48:19)\n> Hi David,\n> \n> On 1/20/23 9:52 PM, David Plowman via libcamera-devel wrote:\n> > We implement a custom validateColorSpaces method that forces all\n> > (non-raw) streams to same colour space, whilst distinguishing RGB\n> > streams from YUV ones, as the former must have the YCbCr encoding and\n> > range over-written.\n> >\n> > When we apply the colour space, we always send the full YUV version as\n> > that gets converted correctly to what our hardware drivers expect. It\n> > is also careful to check what comes back as the YCbCr information gets\n> > overwritten again on the way back.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> > 1 file changed, 114 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8569df17..f768dced 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration\n> > public:\n> > RPiCameraConfiguration(const RPiCameraData *data);\n> > \n> > + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);\n> > Status validate() override;\n> > \n> > /* Cache the combinedTransform_ that will be applied to the sensor */\n> > @@ -317,6 +318,13 @@ public:\n> > \n> > private:\n> > const RPiCameraData *data_;\n> > +\n> > + /*\n> > + * Store the colour spaces that all our streams will have. RGB format streams\n> > + * will be the same but need the YCbCr fields cleared.\n> \n> Would re-pharse it like:\n> \n>         /*\n>          * Store the colour spaces that all our streams will have. RGB \n> format streams\n>          * will have the same colorspace as YUV streams, with YCbCr \n> field cleared and\n>          * range set to full.\n>          */\n\nAre you happy with the expansion on this comment?\n\n\n--\nKieran\n\n\n\n> \n> > + */\n> > + std::optional yuvColorSpace_;\n> > + std::optional rgbColorSpace_;\n> > };\n> > \n> > class PipelineHandlerRPi : public PipelineHandler\n> > @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> > {\n> > }\n> > \n> > +static const std::vector validColorSpaces = {\n> > + ColorSpace::Sycc,\n> > + ColorSpace::Smpte170m,\n> > + ColorSpace::Rec709\n> > +};\n> > +\n> > +static std::optional findValidColorSpace(const ColorSpace &colourSpace)\n> > +{\n> > + for (auto cs : validColorSpaces) {\n> > + if (colourSpace.primaries == cs.primaries &&\n> > + colourSpace.transferFunction == cs.transferFunction)\n> > + return cs;\n> > + }\n> > +\n> > + return std::nullopt;\n> > +}\n> > +\n> > +static bool isRgb(const PixelFormat &pixFmt)\n> > +{\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> > +}\n> > +\n> > +static bool isYuv(const PixelFormat &pixFmt)\n> > +{\n> > + /* The code below would return true for raw mono streams, so weed those out first. */\n> > + if (isRaw(pixFmt))\n> > + return false;\n> > +\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> > +}\n> > +\n> > +/*\n> > + * Raspberry Pi drivers expect the following colour spaces:\n> > + * - V4L2_COLORSPACE_RAW for raw streams.\n> > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for\n> > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and\n> > + * quantisation are not used.\n> > + *\n> > + * The libcamera colour spaces that we wish to use corresponding to these are therefore:\n> > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> > + */\n> \n> Thank you for the information. This makes the expectations clearer.\n> \n> With that,\n> \n> Reviewed-by: Umang Jain \n> \n> \n> I can fixup the comment while applying the patch (if you are OK with the \n> comment's explanation).\n> \n> > +\n> > +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)\n> > +{\n> > + Status status = Valid;\n> > + yuvColorSpace_.reset();\n> > +\n> > + for (auto cfg : config_) {\n> > + /* First fix up raw streams to have the \"raw\" colour space. */\n> > + if (isRaw(cfg.pixelFormat)) {\n> > + /* If there was no value here, that doesn't count as \"adjusted\". */\n> > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {\n> > + status = Adjusted;\n> > + cfg.colorSpace = ColorSpace::Raw;\n> > + }\n> > + continue;\n> > + }\n> > +\n> > + /* Next we need to find our shared colour space. The first valid one will do. */\n> > + if (cfg.colorSpace && !yuvColorSpace_)\n> > + yuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());\n> > + }\n> > +\n> > + /* If no colour space was given anywhere, choose sYCC. */\n> > + if (!yuvColorSpace_)\n> > + yuvColorSpace_ = ColorSpace::Sycc;\n> > +\n> > + /* Note the version of this that any RGB streams will have to use. */\n> > + rgbColorSpace_ = yuvColorSpace_;\n> > + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> > + rgbColorSpace_->range = ColorSpace::Range::Full;\n> > +\n> > + /* Go through the streams again and force everyone to the same colour space. */\n> > + for (auto cfg : config_) {\n> > + if (cfg.colorSpace == ColorSpace::Raw)\n> > + continue;\n> > +\n> > + if (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {\n> > + /* Again, no value means \"not adjusted\". */\n> > + if (cfg.colorSpace)\n> > + status = Adjusted;\n> > + cfg.colorSpace = yuvColorSpace_;\n> > + }\n> > + if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {\n> > + /* Be nice, and let the YUV version count as non-adjusted too. */\n> > + if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)\n> > + status = Adjusted;\n> > + cfg.colorSpace = rgbColorSpace_;\n> > + }\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > {\n> > Status status = Valid;\n> > @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > V4L2DeviceFormat format;\n> > format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> > format.size = cfg.size;\n> > - format.colorSpace = cfg.colorSpace;\n> > + /* We want to send the associated YCbCr info through to the driver. */\n> > + format.colorSpace = yuvColorSpace_;\n> > \n> > LOG(RPI, Debug)\n> > << \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> > @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > if (ret)\n> > return Invalid;\n> > \n> > + /*\n> > + * But for RGB streams, the YCbCr info gets overwritten on the way back\n> > + * so we must check against what the stream cfg says, not what we actually\n> > + * requested (which carefully included the YCbCr info)!\n> > + */\n> > if (cfg.colorSpace != format.colorSpace) {\n> > status = Adjusted;\n> > LOG(RPI, Debug)\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 EA0C5BEFBE\n\tfor ;\n\tMon, 30 Jan 2023 11:04:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F133625E4;\n\tMon, 30 Jan 2023 12:04:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53F1B60482\n\tfor ;\n\tMon, 30 Jan 2023 12:04:07 +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 CC63C8B8;\n\tMon, 30 Jan 2023 12:04:06 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675076648;\n\tbh=ifeUiHTA/wdSMln+6aPvtoM2uMWulEM6sQxD+e1wwWo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=o9Hw0GkQMz3AbKEyVZx8Ae99e1QfDuJJE+oHMrXmOvxPMStSNeb7HaVPGqUAWkk9b\n\tHYaeXjf0tz3u5PFeIyUhMgFJMhINsu4/2mNYsS/h5fglIJKxFhH2EgmpTOTmTIatwJ\n\tOKd1TVWWPb/2x1jcBhLz4MuOVqsbDX66VNjE4g5I7n67wYycvHM9oY6uB1sPmOe7Fr\n\tblBAFrJ9P/RGPEXHi5lu8eKYeqXe340TEtXHN6ZN1QLYPrLM2l3J3FpSLc03kDJZ83\n\tVCG0by65OM0dqLQ6jcEOk9qLfaT5pj6R4TDhZPvLsyt8vl6A6gHUWxasJftqGqteFV\n\tUGKcl4n7vrziw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675076646;\n\tbh=ifeUiHTA/wdSMln+6aPvtoM2uMWulEM6sQxD+e1wwWo=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Oj6srZgIplt6fY5i0k3NnkLqSwMryt0ozrmj1oIAVMRGFxs2oGmAKXFDNjCYpyyW7\n\tieLdMeF4ffG3pCXp7KaOcP4k/S7ofYiJrRZ7ia0W7YgFdIQlrYfNIXb45bdK6JsCnO\n\tSaN5A+2HZR2k14taLnY5I9c44QtZNpmZWOSWrgAI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Oj6srZgI\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<45271050-d93d-c8de-b8d7-bb289cf406a4@ideasonboard.com>","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>\n\t<45271050-d93d-c8de-b8d7-bb289cf406a4@ideasonboard.com>","To":"David Plowman ,\n\tUmang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 30 Jan 2023 11:04:03 +0000","Message-ID":"<167507664392.42371.13710079184268773117@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26384,"web_url":"https://patchwork.libcamera.org/comment/26384/","msgid":"","date":"2023-01-31T06:38:32","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran, Umag\n\nI didn't spot the question, sorry about that. But yes, more complete\ncomments always welcome!\n\nThanks\nDavid\n\nOn Mon, 30 Jan 2023 at 11:04, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> There's a small question below that is holding this back from being merged.\n>\n> I'm planning a release tomorrow, so I'd like to get this colourspace fix\n> in.\n>\n> Quoting Umang Jain via libcamera-devel (2023-01-25 06:48:19)\n> > Hi David,\n> >\n> > On 1/20/23 9:52 PM, David Plowman via libcamera-devel wrote:\n> > > We implement a custom validateColorSpaces method that forces all\n> > > (non-raw) streams to same colour space, whilst distinguishing RGB\n> > > streams from YUV ones, as the former must have the YCbCr encoding and\n> > > range over-written.\n> > >\n> > > When we apply the colour space, we always send the full YUV version as\n> > > that gets converted correctly to what our hardware drivers expect. It\n> > > is also careful to check what comes back as the YCbCr information gets\n> > > overwritten again on the way back.\n> > >\n> > > Signed-off-by: David Plowman \n> > > Reviewed-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> > > 1 file changed, 114 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 8569df17..f768dced 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration\n> > > public:\n> > > RPiCameraConfiguration(const RPiCameraData *data);\n> > >\n> > > + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);\n> > > Status validate() override;\n> > >\n> > > /* Cache the combinedTransform_ that will be applied to the sensor */\n> > > @@ -317,6 +318,13 @@ public:\n> > >\n> > > private:\n> > > const RPiCameraData *data_;\n> > > +\n> > > + /*\n> > > + * Store the colour spaces that all our streams will have. RGB format streams\n> > > + * will be the same but need the YCbCr fields cleared.\n> >\n> > Would re-pharse it like:\n> >\n> > /*\n> > * Store the colour spaces that all our streams will have. RGB\n> > format streams\n> > * will have the same colorspace as YUV streams, with YCbCr\n> > field cleared and\n> > * range set to full.\n> > */\n>\n> Are you happy with the expansion on this comment?\n>\n>\n> --\n> Kieran\n>\n>\n>\n> >\n> > > + */\n> > > + std::optional yuvColorSpace_;\n> > > + std::optional rgbColorSpace_;\n> > > };\n> > >\n> > > class PipelineHandlerRPi : public PipelineHandler\n> > > @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> > > {\n> > > }\n> > >\n> > > +static const std::vector validColorSpaces = {\n> > > + ColorSpace::Sycc,\n> > > + ColorSpace::Smpte170m,\n> > > + ColorSpace::Rec709\n> > > +};\n> > > +\n> > > +static std::optional findValidColorSpace(const ColorSpace &colourSpace)\n> > > +{\n> > > + for (auto cs : validColorSpaces) {\n> > > + if (colourSpace.primaries == cs.primaries &&\n> > > + colourSpace.transferFunction == cs.transferFunction)\n> > > + return cs;\n> > > + }\n> > > +\n> > > + return std::nullopt;\n> > > +}\n> > > +\n> > > +static bool isRgb(const PixelFormat &pixFmt)\n> > > +{\n> > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> > > +}\n> > > +\n> > > +static bool isYuv(const PixelFormat &pixFmt)\n> > > +{\n> > > + /* The code below would return true for raw mono streams, so weed those out first. */\n> > > + if (isRaw(pixFmt))\n> > > + return false;\n> > > +\n> > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Raspberry Pi drivers expect the following colour spaces:\n> > > + * - V4L2_COLORSPACE_RAW for raw streams.\n> > > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for\n> > > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and\n> > > + * quantisation are not used.\n> > > + *\n> > > + * The libcamera colour spaces that we wish to use corresponding to these are therefore:\n> > > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> > > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> > > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> > > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> > > + */\n> >\n> > Thank you for the information. This makes the expectations clearer.\n> >\n> > With that,\n> >\n> > Reviewed-by: Umang Jain \n> >\n> >\n> > I can fixup the comment while applying the patch (if you are OK with the\n> > comment's explanation).\n> >\n> > > +\n> > > +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)\n> > > +{\n> > > + Status status = Valid;\n> > > + yuvColorSpace_.reset();\n> > > +\n> > > + for (auto cfg : config_) {\n> > > + /* First fix up raw streams to have the \"raw\" colour space. */\n> > > + if (isRaw(cfg.pixelFormat)) {\n> > > + /* If there was no value here, that doesn't count as \"adjusted\". */\n> > > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {\n> > > + status = Adjusted;\n> > > + cfg.colorSpace = ColorSpace::Raw;\n> > > + }\n> > > + continue;\n> > > + }\n> > > +\n> > > + /* Next we need to find our shared colour space. The first valid one will do. */\n> > > + if (cfg.colorSpace && !yuvColorSpace_)\n> > > + yuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());\n> > > + }\n> > > +\n> > > + /* If no colour space was given anywhere, choose sYCC. */\n> > > + if (!yuvColorSpace_)\n> > > + yuvColorSpace_ = ColorSpace::Sycc;\n> > > +\n> > > + /* Note the version of this that any RGB streams will have to use. */\n> > > + rgbColorSpace_ = yuvColorSpace_;\n> > > + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> > > + rgbColorSpace_->range = ColorSpace::Range::Full;\n> > > +\n> > > + /* Go through the streams again and force everyone to the same colour space. */\n> > > + for (auto cfg : config_) {\n> > > + if (cfg.colorSpace == ColorSpace::Raw)\n> > > + continue;\n> > > +\n> > > + if (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {\n> > > + /* Again, no value means \"not adjusted\". */\n> > > + if (cfg.colorSpace)\n> > > + status = Adjusted;\n> > > + cfg.colorSpace = yuvColorSpace_;\n> > > + }\n> > > + if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {\n> > > + /* Be nice, and let the YUV version count as non-adjusted too. */\n> > > + if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)\n> > > + status = Adjusted;\n> > > + cfg.colorSpace = rgbColorSpace_;\n> > > + }\n> > > + }\n> > > +\n> > > + return status;\n> > > +}\n> > > +\n> > > CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > {\n> > > Status status = Valid;\n> > > @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > V4L2DeviceFormat format;\n> > > format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> > > format.size = cfg.size;\n> > > - format.colorSpace = cfg.colorSpace;\n> > > + /* We want to send the associated YCbCr info through to the driver. */\n> > > + format.colorSpace = yuvColorSpace_;\n> > >\n> > > LOG(RPI, Debug)\n> > > << \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> > > @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > if (ret)\n> > > return Invalid;\n> > >\n> > > + /*\n> > > + * But for RGB streams, the YCbCr info gets overwritten on the way back\n> > > + * so we must check against what the stream cfg says, not what we actually\n> > > + * requested (which carefully included the YCbCr info)!\n> > > + */\n> > > if (cfg.colorSpace != format.colorSpace) {\n> > > status = Adjusted;\n> > > LOG(RPI, Debug)\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 53D40BEFBE\n\tfor ;\n\tTue, 31 Jan 2023 06:38:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84D50625E4;\n\tTue, 31 Jan 2023 07:38:47 +0100 (CET)","from mail-oi1-x231.google.com (mail-oi1-x231.google.com\n\t[IPv6:2607:f8b0:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE47861EF5\n\tfor ;\n\tTue, 31 Jan 2023 07:38:44 +0100 (CET)","by mail-oi1-x231.google.com with SMTP id s124so12068742oif.1\n\tfor ;\n\tMon, 30 Jan 2023 22:38:44 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675147127;\n\tbh=Wc9dzQ88seOWmWyZqiJ2GxvzGqSKU6YhYJKrWh4tbGI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SwoD7wOzmKxwVEEjchqeL9ZRxYYB/ZX526yKE/VeWiJwrZOXEbhMaliXrd1B6i0Oe\n\tYFPmrwvghR6ufa7B2gOT0woikK5vlzVW3BFTZVcITR4Z+TGx1GlzD7NIowm2qzZadH\n\tKNHOQ6Dsqt1AACMGHzAoljlMnMD9btQF+PiXJVIVRgYkOJQC1AsQo79kfJ94i7kFiV\n\t/O+E6uuvgzWk2Zh40fMD4MdeH3UVk4z2+t4W9bjE8YKjgewF8/21p4efS3GH8RWPqe\n\t4LyMKDI8BLVXOHjlJALABiysIBmOjCuwypF0c0YRHJpilChmgdimwdsltsm9l5J+ee\n\tgPyaA6LrWPEKg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=9dHrzd9sL92a6IstAM2bBzRd4dc2wWsQKFcnmuxlKsI=;\n\tb=jNzXHd0ToHJsP8L3A21McbEexq/cj9wx8HJDxwzMEKMvqi9VEvFuWMQo0X5DPCmWxq\n\tmgBUKh2XvVaX9K1fANilwOYN/4jqfW75XQP1+wpRhHZkYdhZ+aBCkYWom9L11JF/JstR\n\th1k2AxOVPNR3++xNZoNmqTL5dZm0YbeAe8Kjj6lo5EXmjbuyyA2jgKPGZFBkRGuEI/bv\n\t1SWgDCLBs1S+WGGwy9e8gCvFQnT591EWEp9QaFpoVXTA0NCaMakptmBC4y1QvNZ5hbQu\n\tsYNxnbrGCs7eXrx5gWQ6iSleREkgQJUa99vPxWevl8GxPYhCO9cCoNtB7nSy0NMmC3gv\n\ttwCw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"jNzXHd0T\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=9dHrzd9sL92a6IstAM2bBzRd4dc2wWsQKFcnmuxlKsI=;\n\tb=0yzufXetIqXYrN/3ztHzaLTaOjXAOCuBuD/khgCfXngdTbWcXvvaf8nVDIIhHu+95p\n\tHDPdeNRoJcyhrsf4oKrP+9sN3IizKrjgyxq2VffxvlnucdR7pmZygezYawubWPKpa78b\n\t04v1sPxHi6pY/Au5hNzPrWlobfR0hrjtzMYh6Yq0377swzCdULEonO5HrfP9Uql4FV54\n\tGEwUULAdEaYdyMGYcFOFOsGblM/ysy5UPf+bConPb5inNw4a0E3t1TtyukS2ClCr6AgQ\n\tGftSbRODa0jTDnBgHTw0nMxirfrp58p2FrwU5tB5IS0lgWZG7IL9hYrabrskhrJXqs7f\n\tp+sA==","X-Gm-Message-State":"AO0yUKUfduQaMTg4I661Br7ocf9vow1jprgacQwFsToIjCd6ceD+uLmN\n\t53so/YLsN3U8EjbdGH0hEUHj9ReTWp6CklW2vW8VlVLUqSCsnR3j","X-Google-Smtp-Source":"AK7set9QvAxIaQ42wu7t/PwrZDh6Jt5dV7Z/39QGQOcASfkwwiGGrSa00jy7GevDKkRLnpVeBRFPtAZe9kbEudfrYzE=","X-Received":"by 2002:a05:6808:195:b0:378:6ad6:b86c with SMTP id\n\tw21-20020a056808019500b003786ad6b86cmr265764oic.20.1675147123187;\n\tMon, 30 Jan 2023 22:38:43 -0800 (PST)","MIME-Version":"1.0","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>\n\t<45271050-d93d-c8de-b8d7-bb289cf406a4@ideasonboard.com>\n\t<167507664392.42371.13710079184268773117@Monstersaurus>","In-Reply-To":"<167507664392.42371.13710079184268773117@Monstersaurus>","Date":"Tue, 31 Jan 2023 06:38:32 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26412,"web_url":"https://patchwork.libcamera.org/comment/26412/","msgid":"","date":"2023-02-06T17:56:23","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","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 Fri, Jan 20, 2023 at 04:22:58PM +0000, David Plowman via libcamera-devel wrote:\n> We implement a custom validateColorSpaces method that forces all\n> (non-raw) streams to same colour space, whilst distinguishing RGB\n> streams from YUV ones, as the former must have the YCbCr encoding and\n> range over-written.\n> \n> When we apply the colour space, we always send the full YUV version as\n> that gets converted correctly to what our hardware drivers expect. It\n> is also careful to check what comes back as the YCbCr information gets\n> overwritten again on the way back.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> 1 file changed, 114 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17..f768dced 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration\n> public:\n> \tRPiCameraConfiguration(const RPiCameraData *data);\n> \n> +\tCameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);\n> \tStatus validate() override;\n> \n> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n> @@ -317,6 +318,13 @@ public:\n> \n> private:\n> \tconst RPiCameraData *data_;\n> +\n> +\t/*\n> +\t * Store the colour spaces that all our streams will have. RGB format streams\n> +\t * will be the same but need the YCbCr fields cleared.\n> +\t */\n> +\tstd::optional yuvColorSpace_;\n> +\tstd::optional rgbColorSpace_;\n> };\n> \n> class PipelineHandlerRPi : public PipelineHandler\n> @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> {\n> }\n> \n> +static const std::vector validColorSpaces = {\n> +\tColorSpace::Sycc,\n> +\tColorSpace::Smpte170m,\n> +\tColorSpace::Rec709\n> +};\n> +\n> +static std::optional findValidColorSpace(const ColorSpace &colourSpace)\n> +{\n> +\tfor (auto cs : validColorSpaces) {\n> +\t\tif (colourSpace.primaries == cs.primaries &&\n> +\t\t colourSpace.transferFunction == cs.transferFunction)\n> +\t\t\treturn cs;\n> +\t}\n> +\n> +\treturn std::nullopt;\n> +}\n> +\n> +static bool isRgb(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> +}\n> +\n> +static bool isYuv(const PixelFormat &pixFmt)\n> +{\n> +\t/* The code below would return true for raw mono streams, so weed those out first. */\n> +\tif (isRaw(pixFmt))\n> +\t\treturn false;\n> +\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> +}\n> +\n> +/*\n> + * Raspberry Pi drivers expect the following colour spaces:\n> + * - V4L2_COLORSPACE_RAW for raw streams.\n> + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for\n> + * non-raw streams. Other fields such as transfer function, YCbCr encoding and\n> + * quantisation are not used.\n> + *\n> + * The libcamera colour spaces that we wish to use corresponding to these are therefore:\n> + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> + */\n> +\n> +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)\n> +{\n> +\tStatus status = Valid;\n> +\tyuvColorSpace_.reset();\n> +\n> +\tfor (auto cfg : config_) {\n> +\t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> +\t\tif (isRaw(cfg.pixelFormat)) {\n> +\t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> +\t\t\tif (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tcfg.colorSpace = ColorSpace::Raw;\n\nAgreed for the Adjusted flag, but shouldn't you set cfg.colorSpace to\nraw even if !cfg.colorSpace ?\n\n> +\t\t\t}\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* Next we need to find our shared colour space. The first valid one will do. */\n> +\t\tif (cfg.colorSpace && !yuvColorSpace_)\n> +\t\t\tyuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());\n> +\t}\n> +\n> +\t/* If no colour space was given anywhere, choose sYCC. */\n> +\tif (!yuvColorSpace_)\n> +\t\tyuvColorSpace_ = ColorSpace::Sycc;\n> +\n> +\t/* Note the version of this that any RGB streams will have to use. */\n> +\trgbColorSpace_ = yuvColorSpace_;\n> +\trgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> +\trgbColorSpace_->range = ColorSpace::Range::Full;\n> +\n> +\t/* Go through the streams again and force everyone to the same colour space. */\n> +\tfor (auto cfg : config_) {\n> +\t\tif (cfg.colorSpace == ColorSpace::Raw)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {\n> +\t\t\t/* Again, no value means \"not adjusted\". */\n> +\t\t\tif (cfg.colorSpace)\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\tcfg.colorSpace = yuvColorSpace_;\n> +\t\t}\n> +\t\tif (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {\n> +\t\t\t/* Be nice, and let the YUV version count as non-adjusted too. */\n> +\t\t\tif (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\tcfg.colorSpace = rgbColorSpace_;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> CameraConfiguration::Status RPiCameraConfiguration::validate()\n> {\n> \tStatus status = Valid;\n> @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\tV4L2DeviceFormat format;\n> \t\tformat.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> \t\tformat.size = cfg.size;\n> -\t\tformat.colorSpace = cfg.colorSpace;\n> +\t\t/* We want to send the associated YCbCr info through to the driver. */\n> +\t\tformat.colorSpace = yuvColorSpace_;\n> \n> \t\tLOG(RPI, Debug)\n> \t\t\t<< \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\tif (ret)\n> \t\t\treturn Invalid;\n> \n> +\t\t/*\n> +\t\t * But for RGB streams, the YCbCr info gets overwritten on the way back\n> +\t\t * so we must check against what the stream cfg says, not what we actually\n> +\t\t * requested (which carefully included the YCbCr info)!\n> +\t\t */\n> \t\tif (cfg.colorSpace != format.colorSpace) {\n> \t\t\tstatus = Adjusted;\n> \t\t\tLOG(RPI, Debug)","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 80344BDB1C\n\tfor ;\n\tMon, 6 Feb 2023 17:56:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2E17625F1;\n\tMon, 6 Feb 2023 18:56:25 +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 3222C603B8\n\tfor ;\n\tMon, 6 Feb 2023 18:56:25 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.136.43.56])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFE3E4DA;\n\tMon, 6 Feb 2023 18:56:24 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675706185;\n\tbh=j5FXsNVwAcl/KLCTEUv0nmZAleXFPwvmCZ7U35wFSrI=;\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=vVwjnbqyt9sRu4JuPcvpy6lk2arS/L+X8HVkalyfpe0VC8SS6HMLMSUBr+w1ntRhp\n\tsKG2PxUV+3LtXy7+CVuzgnfchuGWzGBHIy6cGQjfvw3SEs32rM73PzByxNnPIRtw6A\n\tywN0UQEceGTKrmBZeQOEVi8XNrV4mqwvz6gz1RBmlnDMXZhnKblXMrm74ZTNw8bBJE\n\tCiLIGfCAtzEyPJUS3R4G09q3IattzBOh2BBzZwHtZxeS6tJegHhAgUV5zVIfJYlF5K\n\thUsxM2w8N7sidA6ZDILTuw4fy1dz/qMuoaSqD/hOPDLDDvBwki4z8E+LVfsKey3sAJ\n\tKIvcLOg1ROCWg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675706184;\n\tbh=j5FXsNVwAcl/KLCTEUv0nmZAleXFPwvmCZ7U35wFSrI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K1NkXR+ICO+TfWoDZNgC7jAFYejcVIQLKwf5vsWGGLilIXl4m3tPPR7wcUEtTg3zV\n\t1CT5hi6Ha2Ip6uwbvM0brjAfAwgBxyaU7pOF5LEkFBBKlbVUxaGRn2CLhea5KlEsK/\n\tDaii22k4lWAV/9sc0hjQ+wqATAcUSEr8zCWCCqsE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K1NkXR+I\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 19:56:23 +0200","To":"David Plowman ","Message-ID":"","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230120162258.7039-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26448,"web_url":"https://patchwork.libcamera.org/comment/26448/","msgid":"","date":"2023-02-13T09:10:43","subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nWhoops, bit of a slip in the little \"optimisation\" in the v2 version.\nThanks for spotting it, I'll send a small patch.\n\nDavid\n\nOn Mon, 6 Feb 2023 at 17:56, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 20, 2023 at 04:22:58PM +0000, David Plowman via\n> libcamera-devel wrote:\n> > We implement a custom validateColorSpaces method that forces all\n> > (non-raw) streams to same colour space, whilst distinguishing RGB\n> > streams from YUV ones, as the former must have the YCbCr encoding and\n> > range over-written.\n> >\n> > When we apply the colour space, we always send the full YUV version as\n> > that gets converted correctly to what our hardware drivers expect. It\n> > is also careful to check what comes back as the YCbCr information gets\n> > overwritten again on the way back.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-\n> > 1 file changed, 114 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8569df17..f768dced 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public\n> CameraConfiguration\n> > public:\n> > RPiCameraConfiguration(const RPiCameraData *data);\n> >\n> > + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags\n> flags);\n> > Status validate() override;\n> >\n> > /* Cache the combinedTransform_ that will be applied to the sensor\n> */\n> > @@ -317,6 +318,13 @@ public:\n> >\n> > private:\n> > const RPiCameraData *data_;\n> > +\n> > + /*\n> > + * Store the colour spaces that all our streams will have. RGB\n> format streams\n> > + * will be the same but need the YCbCr fields cleared.\n> > + */\n> > + std::optional yuvColorSpace_;\n> > + std::optional rgbColorSpace_;\n> > };\n> >\n> > class PipelineHandlerRPi : public PipelineHandler\n> > @@ -357,6 +365,105 @@\n> RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> > {\n> > }\n> >\n> > +static const std::vector validColorSpaces = {\n> > + ColorSpace::Sycc,\n> > + ColorSpace::Smpte170m,\n> > + ColorSpace::Rec709\n> > +};\n> > +\n> > +static std::optional findValidColorSpace(const ColorSpace\n> &colourSpace)\n> > +{\n> > + for (auto cs : validColorSpaces) {\n> > + if (colourSpace.primaries == cs.primaries &&\n> > + colourSpace.transferFunction == cs.transferFunction)\n> > + return cs;\n> > + }\n> > +\n> > + return std::nullopt;\n> > +}\n> > +\n> > +static bool isRgb(const PixelFormat &pixFmt)\n> > +{\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;\n> > +}\n> > +\n> > +static bool isYuv(const PixelFormat &pixFmt)\n> > +{\n> > + /* The code below would return true for raw mono streams, so weed\n> those out first. */\n> > + if (isRaw(pixFmt))\n> > + return false;\n> > +\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;\n> > +}\n> > +\n> > +/*\n> > + * Raspberry Pi drivers expect the following colour spaces:\n> > + * - V4L2_COLORSPACE_RAW for raw streams.\n> > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M,\n> V4L2_COLORSPACE_REC709 for\n> > + * non-raw streams. Other fields such as transfer function, YCbCr\n> encoding and\n> > + * quantisation are not used.\n> > + *\n> > + * The libcamera colour spaces that we wish to use corresponding to\n> these are therefore:\n> > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW\n> > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG\n> > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M\n> > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709\n> > + */\n> > +\n> > +CameraConfiguration::Status\n> RPiCameraConfiguration::validateColorSpaces([[maybe_unused]]\n> ColorSpaceFlags flags)\n> > +{\n> > + Status status = Valid;\n> > + yuvColorSpace_.reset();\n> > +\n> > + for (auto cfg : config_) {\n> > + /* First fix up raw streams to have the \"raw\" colour\n> space. */\n> > + if (isRaw(cfg.pixelFormat)) {\n> > + /* If there was no value here, that doesn't count\n> as \"adjusted\". */\n> > + if (cfg.colorSpace && cfg.colorSpace !=\n> ColorSpace::Raw) {\n> > + status = Adjusted;\n> > + cfg.colorSpace = ColorSpace::Raw;\n>\n> Agreed for the Adjusted flag, but shouldn't you set cfg.colorSpace to\n> raw even if !cfg.colorSpace ?\n>\n> > + }\n> > + continue;\n> > + }\n> > +\n> > + /* Next we need to find our shared colour space. The first\n> valid one will do. */\n> > + if (cfg.colorSpace && !yuvColorSpace_)\n> > + yuvColorSpace_ =\n> findValidColorSpace(cfg.colorSpace.value());\n> > + }\n> > +\n> > + /* If no colour space was given anywhere, choose sYCC. */\n> > + if (!yuvColorSpace_)\n> > + yuvColorSpace_ = ColorSpace::Sycc;\n> > +\n> > + /* Note the version of this that any RGB streams will have to use.\n> */\n> > + rgbColorSpace_ = yuvColorSpace_;\n> > + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> > + rgbColorSpace_->range = ColorSpace::Range::Full;\n> > +\n> > + /* Go through the streams again and force everyone to the same\n> colour space. */\n> > + for (auto cfg : config_) {\n> > + if (cfg.colorSpace == ColorSpace::Raw)\n> > + continue;\n> > +\n> > + if (isYuv(cfg.pixelFormat) && cfg.colorSpace !=\n> yuvColorSpace_) {\n> > + /* Again, no value means \"not adjusted\". */\n> > + if (cfg.colorSpace)\n> > + status = Adjusted;\n> > + cfg.colorSpace = yuvColorSpace_;\n> > + }\n> > + if (isRgb(cfg.pixelFormat) && cfg.colorSpace !=\n> rgbColorSpace_) {\n> > + /* Be nice, and let the YUV version count as\n> non-adjusted too. */\n> > + if (cfg.colorSpace && cfg.colorSpace !=\n> yuvColorSpace_)\n> > + status = Adjusted;\n> > + cfg.colorSpace = rgbColorSpace_;\n> > + }\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > {\n> > Status status = Valid;\n> > @@ -533,7 +640,8 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > V4L2DeviceFormat format;\n> > format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> > format.size = cfg.size;\n> > - format.colorSpace = cfg.colorSpace;\n> > + /* We want to send the associated YCbCr info through to\n> the driver. */\n> > + format.colorSpace = yuvColorSpace_;\n> >\n> > LOG(RPI, Debug)\n> > << \"Try color space \" <<\n> ColorSpace::toString(cfg.colorSpace);\n> > @@ -542,6 +650,11 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > if (ret)\n> > return Invalid;\n> >\n> > + /*\n> > + * But for RGB streams, the YCbCr info gets overwritten on\n> the way back\n> > + * so we must check against what the stream cfg says, not\n> what we actually\n> > + * requested (which carefully included the YCbCr info)!\n> > + */\n> > if (cfg.colorSpace != format.colorSpace) {\n> > status = Adjusted;\n> > LOG(RPI, Debug)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7910CBDB1C\n\tfor ;\n\tMon, 13 Feb 2023 09:10:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D686625F4;\n\tMon, 13 Feb 2023 10:10:57 +0100 (CET)","from mail-oa1-x2a.google.com (mail-oa1-x2a.google.com\n\t[IPv6:2001:4860:4864:20::2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1145161EED\n\tfor ;\n\tMon, 13 Feb 2023 10:10:55 +0100 (CET)","by mail-oa1-x2a.google.com with SMTP id\n\t586e51a60fabf-16aa71c1600so14423530fac.11\n\tfor ;\n\tMon, 13 Feb 2023 01:10:54 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1676279457;\n\tbh=NOFsJoZbqvTxSssMxdiuS2ZjDB3LgIBsvhn8v0M6eYw=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PbouHXUXcW6arhrYPHLh81XYCbw88OjZE0gaRH/i/sU79DZuR8ozoTab/BJsZGpw5\n\tOnnh74hkeKzrWs/B8aUkjXLOdSP9RHOdi/SJlMSVwbOhrs+gX7X3kFPhouWs2NeNTe\n\tT3eHT+IQ+RWIZe+sCv+PjTupb4GJlpB24I+SZf65SiPFMRMvKC8zau+JhZ1l2ZjhWn\n\tSjXkBehLRtzQojmlgkweLXoCrD7R33wZ/UZrS0Bp/WqqPntEAM8uCXBAljYMQC0TXh\n\tL5ViLcqwNxV1ghyuXDA6qb0gbyOBNnjjC+szlBm5jn0Ks9qyjBvSDZz9wd/fZs01Df\n\tU7GICvVAbBDyg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=IlcCMVCGgcFwtNXR4LtiumMIV0H1CRG1OykD5tfHIgo=;\n\tb=bzeffsLRzSyYvHkNsLaTPFL42lRlw+apPz4Wp5nhz2PK8/kujbCq1kOT4pk0ersJrL\n\tavoTDGv5br7H9APueRW9s0iFcMW2DMWyQk0DE8OsG1UXXLeP1yrbThxnI/TXTliWiCBu\n\tpWXf7lXDc9s/s2cPK7Sbe5PDLGQVQK1v2zRSQL2asZfM46p46xbMyvDzTcqRQWOiJGQ8\n\tTLNJp6DtN/S+BcEpPmAuIAWQMGZUPIvagd1yPMmmCVGPhAvnEt64gP/Xsre4L3uExYVl\n\t8pLuMRPUuUNU7P2ZTO5+0deGLkHYsUkLZUKZSSnEeW1W/I5eMrvDAi2MeuGOJdy5vIwA\n\tO7PQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"bzeffsLR\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=IlcCMVCGgcFwtNXR4LtiumMIV0H1CRG1OykD5tfHIgo=;\n\tb=0OykHMA6ljExmiboPAEhKud6MtZvELrWS5KtU/oLCw4LRglalFi1JgXAt3Z1Q39HGD\n\tiwxUJfRATU71ieU6wRHwQB4m3zXdMQ4QX6wgJb/mOt5GbYVjEnkjFBbx/m3pzL8BUejM\n\tzzRORxXGijWCrJYzlcqVDNy4be3hgIcQKNqejZMiuGXlDee1gPzpitf5RPTnzLXVBlcw\n\tF/awA6d78BIWfbbCX3JvynVSSZLSXzAY1GcCKgAyil9O9WqgMQuNJVKQm0mo56gb3Y4D\n\tG9ybTa5+XlMz5riVmN5MHViwu5E5Ra9bq4z2j3xY7fLFJnPrj6nuSpCSEzhO7msgRniw\n\trl9w==","X-Gm-Message-State":"AO0yUKX86FKc8mV9X8yyfNeATh7BtHHJ1Sz9/0ZqIzfy0qQtm5gsfg37\n\t5YV4Cx6eTYTIMdoEOrdncHL6+c1YNfUhQhOE+3MAgzhAelrVEA==","X-Google-Smtp-Source":"AK7set/EpXOcoyONkNqO1sKhNeGtWKDFJ7UYbP+fL9BsegWQXU8gPQSnRgVfk//c/HUhVYF4mDdeorMPgGYaCgOhfWI=","X-Received":"by 2002:a05:6870:310:b0:16a:3253:8854 with SMTP id\n\tm16-20020a056870031000b0016a32538854mr2883425oaf.20.1676279453771;\n\tMon, 13 Feb 2023 01:10:53 -0800 (PST)","MIME-Version":"1.0","References":"<20230120162258.7039-1-david.plowman@raspberrypi.com>\n\t<20230120162258.7039-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 13 Feb 2023 09:10:43 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000063095905f49139e8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix\n\thandling of colour spaces","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]