[{"id":26245,"web_url":"https://patchwork.libcamera.org/comment/26245/","msgid":"<62f42ad5-9cae-daa1-3d2e-95aa7a91d597@ideasonboard.com>","date":"2023-01-17T08:47:03","subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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\nThanks for the patch.\n\nOn 1/12/23 5:40 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\nIf I may, can you point to sources what the hardware / firmware expects \nwith respect to colorspaces /exactly/ ?\n\nThe larger issue I see this is as a fragile piece, that has been \nregressed. Moving forward, I believe there is a high chance that the \nregression might happen again (provided the fragility) hence a solid \ndocument on the Pi's hardware / firmware should be included in the code \nbase I believe.  If it has been summarised in any other colorspace \nthreads, I would be happy to take a look at that as well!\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 | 99 ++++++++++++++++++-\n> 1 file changed, 98 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17..135024e7 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,89 @@ 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\nBoth of these helpers can be utils I believe - but can be done on top as \nwell...\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) && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> +\t\t\tif (cfg.colorSpace)\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n\nCan be slightly optimised by having a    :\n               continue;\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 && cfg.colorSpace != ColorSpace::Raw && !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\nI believe there is some degree of logic duplication coming primarily \nfrom CameraConfiguration::validateColorSpaces() which calls \nColorSpace::adjust()\n\nI am not sure (yet) if we want to merge the two - i.e. a overall \nCameraConfiguration::validateColorspace() with a pipeline-handler \nspecific PHValidateColorSpace() working / adjusting on top...  and have \na more clear layers of functions operating on user-input-ed colorspace\nor\nHave a single function only for adjusting colorspace (either use \nCameraConfiguration::validateColorspace() OR a pipeline-handler specific \none like you've done here).\n\nBut .... nonetheless  I believe:\n\n- Assigning ColorSpace::Raw  for the raw streams above\n- For clearing Y'CbCrEncoding and range on RGB stream\n\nOne can use the ColorSpace::adjust() helper and avoid the code \nduplication here... no?\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\nI am not sure what's happening here.\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 +624,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 +634,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 6E2B2C3240\n\tfor ;\n\tTue, 17 Jan 2023 08:47:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6902625D8;\n\tTue, 17 Jan 2023 09:47:09 +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 37FE861EFE\n\tfor ;\n\tTue, 17 Jan 2023 09:47:08 +0100 (CET)","from [192.168.1.103] (unknown [103.86.18.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32E4710C;\n\tTue, 17 Jan 2023 09:47:07 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673945229;\n\tbh=IoQ1PP63poQFL5IL39pTnb4IPqo8mFdDZjNw38dr1Rc=;\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=eBAAiKmUf9W4TIaLoZJAy5o5WoZUOLNdOhDpH1Ge2vK8WL8ErG4yK9URL9apeSG8r\n\tK7ehaS3kXARGvx8jJJjauBFbaGWPoH6vWVTHchGsIPV7pNV/ejdp5REhgqeM6XowQ1\n\tmPTWSFduUFRCWHVewXm3i9U1FnaMmomIYlfnPsbRqQf4vvhkthJGsLyBeTfjAdZXhp\n\tuPWMjCzBVHzaw0t0gvv+Pa8WhW4xgFurQktx13W9J/emhT3iH/8h4QKlAmlp+otaEj\n\tImDdLNpZCbwKpA5PwKlAwiX8CUokOOt4PXt/0l6DBaKNl+mbh0SBw4mBoKo2ui1Mm2\n\tuMg5/foQVxgVw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673945227;\n\tbh=IoQ1PP63poQFL5IL39pTnb4IPqo8mFdDZjNw38dr1Rc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=T1D1maX9VauVR1dXHzELd7vWeJxe8T936LwXGxn0PMCnyb5/Td0OJbKxCnBjIB53Q\n\tUj+ldwIRsu+zCDRy0dVW7MhZX4jYfOyackvLoNLJzPDTQP5tRn6aFhvSquWOecHGq9\n\t5QcFpNQeI0CDUR/+97NRf+wsaouqEsbXU+TMw8m8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T1D1maX9\"; dkim-atps=neutral","Message-ID":"<62f42ad5-9cae-daa1-3d2e-95aa7a91d597@ideasonboard.com>","Date":"Tue, 17 Jan 2023 14:17:03 +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":"<20230112121044.28003-1-david.plowman@raspberrypi.com>\n\t<20230112121044.28003-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20230112121044.28003-3-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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":26258,"web_url":"https://patchwork.libcamera.org/comment/26258/","msgid":"","date":"2023-01-18T16:06:48","subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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 Umang\n\nThanks for the comments!\n\nOn Tue, 17 Jan 2023 at 08:47, Umang Jain wrote:\n>\n> Hi David,\n>\n> Thanks for the patch.\n>\n> On 1/12/23 5:40 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>\n> If I may, can you point to sources what the hardware / firmware expects\n> with respect to colorspaces /exactly/ ?\n\nThe Pi is very simple. When a raw stream is being used, the V4L2\ndrivers expect the colour space to be V4L2_COLORSPACE_RAW.\n\nFor non-raw streams, the colour space should be one of\nV4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M or\nV4L2_COLORSPACE_REC709. That's it. All the other colour space modifier\nfields are ignored.\n\nCorrespondingly, the only colour spaces I want to use in libcamera are\nColorSpace::Raw (raw streams only), otherwise ColorSpace::Sycc,\nColorSpace::Smpte170m or ColorSpace::Rec709. It should be very easy!\n\n>\n> The larger issue I see this is as a fragile piece, that has been\n> regressed. Moving forward, I believe there is a high chance that the\n> regression might happen again (provided the fragility) hence a solid\n> document on the Pi's hardware / firmware should be included in the code\n> base I believe. If it has been summarised in any other colorspace\n> threads, I would be happy to take a look at that as well!\n\nWhere would be the best place to put this, do you think? I'm very\nhappy to add something - as I explained above, the situation on the Pi\nis very straightforward.\n\nWe have very comprehensive tests for all our stream/colour space\ncombinations so we should notice straight away when things are broken.\nHopefully Kieran will be able to start running these once this patch\nis merged and colour spaces on the Pi are working again.\n\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 | 99 ++++++++++++++++++-\n> > 1 file changed, 98 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8569df17..135024e7 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,89 @@ 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> Both of these helpers can be utils I believe - but can be done on top as\n> well...\n\nPossibly... though some other PHs don't agree with my definition of\nisYuv (for raw mono streams, as noted in the comments), and\nconsequently, our definition of isRaw too. So I think I may be better\noff with local versions here.\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) && cfg.colorSpace != ColorSpace::Raw) {\n> > + /* If there was no value here, that doesn't count as \"adjusted\". */\n> > + if (cfg.colorSpace)\n> > + status = Adjusted;\n> > + cfg.colorSpace = ColorSpace::Raw;\n>\n> Can be slightly optimised by having a :\n> continue;\n\nSure, I can do that!\n\n> > + }\n> > +\n> > + /* Next we need to find our shared colour space. The first valid one will do. */\n> > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw && !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> I believe there is some degree of logic duplication coming primarily\n> from CameraConfiguration::validateColorSpaces() which calls\n> ColorSpace::adjust()\n>\n> I am not sure (yet) if we want to merge the two - i.e. a overall\n> CameraConfiguration::validateColorspace() with a pipeline-handler\n> specific PHValidateColorSpace() working / adjusting on top... and have\n> a more clear layers of functions operating on user-input-ed colorspace\n> or\n> Have a single function only for adjusting colorspace (either use\n> CameraConfiguration::validateColorspace() OR a pipeline-handler specific\n> one like you've done here).\n>\n> But .... nonetheless I believe:\n>\n> - Assigning ColorSpace::Raw for the raw streams above\n> - For clearing Y'CbCrEncoding and range on RGB stream\n>\n> One can use the ColorSpace::adjust() helper and avoid the code\n> duplication here... no?\n\nI'm not terribly keen on the automatic colour space changes that are\napplied in ColorSpace::adjust() so I'd rather be in control of them\nmyself. Really, I don't want any general-purpose functions that change\nthe colour space, either when I send stuff to the drivers or look at\nwhat comes back, without me knowing what has happened.\n\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>\n> I am not sure what's happening here.\n\nThe current libcamera behaviour is that you can't ask for (for\nexample) ColorSpace::Rec709 when the output format is RGB. Instead,\nthe colour space must be changed to Rec709/Srgb/None/Full.\n\nIn the code above I leave Rec709 alone for YUV streams, but \"correct\"\nit to Rec709/Srgb/None/Full for RGB streams. But I've decided not to\nflag the config as \"adjusted\" if the colour space was Rec709 initially\n(if it was something else then sure, that's \"adjusted\"). The reason is\nthat nothing has really changed - the output pixels will be no\ndifferent - so forcing the user to figure out what got changed and\nwhether it matters, seems quite unfriendly. But we do have a choice\nwhether to be nice to our users or not!\n\nIt's worth noting that the base class validateColorSpaces method\ndoesn't handle \"shared\" colour spaces correctly any more, it even\nseems like an awkward concept now. Personally, I'm tending to think we\nshould remove it, at which point the base class method doesn't provide\na huge amount of value to the Pi.\n\nYou know how it goes - I thought I was doing the world a favour by\nadding a base class method but now, well, I wish I hadn't. :(\n\nThanks!\nDavid\n\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 +624,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 +634,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 C09B1C3240\n\tfor ;\n\tWed, 18 Jan 2023 16:07:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3172E625D8;\n\tWed, 18 Jan 2023 17:07:03 +0100 (CET)","from mail-oi1-x233.google.com (mail-oi1-x233.google.com\n\t[IPv6:2607:f8b0:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AFAC9625CC\n\tfor ;\n\tWed, 18 Jan 2023 17:07:00 +0100 (CET)","by mail-oi1-x233.google.com with SMTP id r132so18426126oif.10\n\tfor ;\n\tWed, 18 Jan 2023 08:07:00 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674058023;\n\tbh=faRGeZDRW2c+kFs1uL8rhTHE5nLdwiYrX7AngdQvIK8=;\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=MFhmohxohy0/GjCiChWJsFvF6Smap3LahoM0uzxBEVsjAzwe8+jtUsSjyOMQQ2yau\n\tMSR75+Hrv8neBmhq1hMQzfrK8+VkyqkmVcvTrnNPDCq9ynGvDNaYFSg735RM5H/fUm\n\tiIFhYgG2HZBlFxIpb868Q+ErIzgHl5b2FCZNir5YAa8k7ErkKxGGXZdWGK5xfPfbDQ\n\tzuHafR0bB+anNT/8q40+uBHJFGpjey2wjREaJCF2RvxVgAX8B3ZLCQ+IO3wuiFTxPm\n\tDpthE7DUbCDUnlPOTTOnDWboIZ5ljt1rmRsvY/OZz6AhgaXnM0CqTzyS6H36ll3vHF\n\tMljHMYImd+1qA==","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=4tIBjHwsdaj3S0L10f7MaJUigRve0BXfsiJFgK/Q0+0=;\n\tb=GQJ/jfjiZK1ETIXxXXhcS5FUcAOXJjybbBR6/1jso0pV5aMQKEHFUy05euaGf4McM5\n\t41dy5Z4BekUJ+zNmpKiP/YeE5vuUr8kAtFRG1MET3u/CdkDR7hxGmOQXqxRw+KFzJzM2\n\tbRCINKkvHWm++B9e/Frd3TJUktprbBo1K5oFu9dsqzoH4ce/H5mfXk4oYauEyOTym0R6\n\tjepeYDdGZBs3bg++tJHhQGkarET4D/PdLD4/cde2Ub6QqwsZshXRM4qw1LXnhkSUkRXC\n\t7CGBQKPK2YejYhdGbPXb9Vop1Hs5OUKHKJdYV/Ie2J2hE5kUuQ6bqrUtFu13GX9FKiMR\n\tlZPA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"GQJ/jfji\"; 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=4tIBjHwsdaj3S0L10f7MaJUigRve0BXfsiJFgK/Q0+0=;\n\tb=OckkplHAWqNImUtBXMoIMCyb1Q3YnLCr+op3Ikg/TAyihRW7AdJEz1nMSSIzlD01Qm\n\tsIuyq/EGjYYwfLYwdhysnsPMoUMuw4xOmsetlI05PCqOA9dK4W7Q+MjmCv+j7feU1gi/\n\tekMwvZGMbV9Zyva52W3PW9t1TLqUPwdCOoK2B//ITuykhLPeQZMEEc1wqL6s6ycSaY+8\n\t+tPB1b2jf5/FqBWh1BJqqNwiC9sCDUpHwwmu6HDchpy3KG7XCgMFJdJrhGnXBshWGvYv\n\tD9+pa2DMD4r2OIornJDLVvkVQrzCn2wim7iDSzZL/xICR6Cagp6MDn+AO0OEjx040NQA\n\tvERA==","X-Gm-Message-State":"AFqh2kr8sqcvjWjlxlXD4lCA7IAbI/NxtpT0uDoIEHBJkJAuS/tR3vFj\n\tfxMGQu9OWKpsV2porb81SnT0gP5yMswAFO1JWb2gsvyyXMUnwwM9","X-Google-Smtp-Source":"AMrXdXvlZak9gFM3Yb7KRCfPLN67f8Zj6TcwFkTpSU9XaSCN5xtHegVQN1sdJ9iTQUSg7dyado/ezDNLBZUym3v3ZPA=","X-Received":"by 2002:a05:6808:4d0:b0:360:fbf1:4489 with SMTP id\n\ta16-20020a05680804d000b00360fbf14489mr434158oie.20.1674058019128;\n\tWed, 18 Jan 2023 08:06:59 -0800 (PST)","MIME-Version":"1.0","References":"<20230112121044.28003-1-david.plowman@raspberrypi.com>\n\t<20230112121044.28003-3-david.plowman@raspberrypi.com>\n\t<62f42ad5-9cae-daa1-3d2e-95aa7a91d597@ideasonboard.com>","In-Reply-To":"<62f42ad5-9cae-daa1-3d2e-95aa7a91d597@ideasonboard.com>","Date":"Wed, 18 Jan 2023 16:06:48 +0000","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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\" "}}]