[{"id":33799,"web_url":"https://patchwork.libcamera.org/comment/33799/","msgid":"<174344307107.3394313.13915108310238565915@ping.linuxembedded.co.uk>","date":"2025-03-31T17:44:31","subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-03-19 16:11:17)\n> In the grey world AWB case, if no colour gains are contained in the\n> tuning file, the color gains get reset to 1 when the colour temperature\n> is set manually. This is unexpected and undesirable. Allow the\n> gainsFromColourTemp() function to return a std::nullopt to handle that\n> case.\n\nI tried to quibble on this but failed, so I think that means it seems\nreasonable...\n\n\nReviewed-by: Kieran Bingham \n\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v2:\n> - Added this patch\n> ---\n> src/ipa/libipa/awb.cpp | 2 +-\n> src/ipa/libipa/awb.h | 2 +-\n> src/ipa/libipa/awb_bayes.cpp | 4 ++--\n> src/ipa/libipa/awb_bayes.h | 2 +-\n> src/ipa/libipa/awb_grey.cpp | 6 +++---\n> src/ipa/libipa/awb_grey.h | 2 +-\n> src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----\n> 7 files changed, 20 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp\n> index 925fac232709..214bac8b56a6 100644\n> --- a/src/ipa/libipa/awb.cpp\n> +++ b/src/ipa/libipa/awb.cpp\n> @@ -114,7 +114,7 @@ namespace ipa {\n> * does not take any statistics into account. It is used to compute the colour\n> * gains when the user manually specifies a colour temperature.\n> *\n> - * \\return The colour gains\n> + * \\return The colour gains or std::nullopt if the conversion is not possible\n> */\n> \n> /**\n> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> index 4bab7a451ce5..789a1d534f9a 100644\n> --- a/src/ipa/libipa/awb.h\n> +++ b/src/ipa/libipa/awb.h\n> @@ -39,7 +39,7 @@ public:\n> \n> virtual int init(const YamlObject &tuningData) = 0;\n> virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> - virtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> + virtual std::optional> gainsFromColourTemperature(double colourTemperature) = 0;\n> \n> const ControlInfoMap::Map &controls() const\n> {\n> diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> index d1d0eaf0e68f..d2bcbd83d7f8 100644\n> --- a/src/ipa/libipa/awb_bayes.cpp\n> +++ b/src/ipa/libipa/awb_bayes.cpp\n> @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)\n> }\n> }\n> \n> -RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> +std::optional> AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> {\n> /*\n> * \\todo In the RaspberryPi code, the ct curve was interpolated in\n> @@ -278,7 +278,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> * intuitive, as the gains are in linear space. But I can't prove it.\n> */\n> const auto &gains = colourGainCurve_.getInterpolated(colourTemperature);\n> - return { { gains[0], 1.0, gains[1] } };\n> + return RGB{ { gains[0], 1.0, gains[1] } };\n> }\n> \n> AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> index bb933038d6d2..47ef3cce4d58 100644\n> --- a/src/ipa/libipa/awb_bayes.h\n> +++ b/src/ipa/libipa/awb_bayes.h\n> @@ -27,7 +27,7 @@ public:\n> \n> int init(const YamlObject &tuningData) override;\n> AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> - RGB gainsFromColourTemperature(double temperatureK) override;\n> + std::optional> gainsFromColourTemperature(double temperatureK) override;\n> void handleControls(const ControlList &controls) override;\n> \n> private:\n> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> index d3d2132b8869..d252edb2b6c6 100644\n> --- a/src/ipa/libipa/awb_grey.cpp\n> +++ b/src/ipa/libipa/awb_grey.cpp\n> @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned\n> * \\return The colour gains if a colour temperature curve is available,\n> * [1, 1, 1] otherwise.\n> */\n> -RGB AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> +std::optional> AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> {\n> if (!colourGainCurve_) {\n> LOG(Awb, Error) << \"No gains defined\";\n> - return RGB({ 1.0, 1.0, 1.0 });\n> + return std::nullopt;\n> }\n> \n> auto gains = colourGainCurve_->getInterpolated(colourTemperature);\n> - return { { gains[0], 1.0, gains[1] } };\n> + return RGB{ { gains[0], 1.0, gains[1] } };\n> }\n> \n> } /* namespace ipa */\n> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> index 7ec7bfa5da9a..f82a368d11cc 100644\n> --- a/src/ipa/libipa/awb_grey.h\n> +++ b/src/ipa/libipa/awb_grey.h\n> @@ -25,7 +25,7 @@ public:\n> \n> int init(const YamlObject &tuningData) override;\n> AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> - RGB gainsFromColourTemperature(double colourTemperature) override;\n> + std::optional> gainsFromColourTemperature(double colourTemperature) override;\n> \n> private:\n> std::optional>> colourGainCurve_;\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 5e067e50cd52..58b8370d2c61 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,\n> const IPACameraSensorInfo &configInfo)\n> {\n> context.activeState.awb.manual.gains = RGB{ 1.0 };\n> - context.activeState.awb.automatic.gains =\n> - awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> + auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> + if (gains)\n> + context.activeState.awb.automatic.gains = *gains;\n> + else\n> + context.activeState.awb.automatic.gains = RGB{ 1.0 };\n> +\n> context.activeState.awb.autoEnabled = true;\n> context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;\n> context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;\n> @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,\n> update = true;\n> } else if (colourTemperature) {\n> const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> - awb.manual.gains.r() = gains.r();\n> - awb.manual.gains.b() = gains.b();\n> + if (gains) {\n> + awb.manual.gains.r() = gains->r();\n> + awb.manual.gains.b() = gains->b();\n> + update = true;\n> + }\n> awb.manual.temperatureK = *colourTemperature;\n> - update = true;\n> }\n> \n> if (update)\n> -- \n> 2.43.0\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 D0CB6C323E\n\tfor ;\n\tMon, 31 Mar 2025 17:44:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D5A668981;\n\tMon, 31 Mar 2025 19:44:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47B2E68967\n\tfor ;\n\tMon, 31 Mar 2025 19:44:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F0DD703;\n\tMon, 31 Mar 2025 19:42:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PQSoNbum\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743442962;\n\tbh=Ye3loN9kDuYoperPnZGTD2ToZU9z8kNIwCVkNkQdJYw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PQSoNbumH0ZE12KTU0f97XBH72InXX6fzkz4Yc7b2kc32zFuzgtdYEDyK0z19IWA3\n\tkQZ6gTZkRHV+0YIIxHgXTCeLM4Y+X17vl9GZuxr3xrEk7tArFYT1Nyh5aBZVV2/K4U\n\tOoogxI7jqixbzgyzgr8FYX0R3VFYe8wNacWpItMk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250319161152.63625-13-stefan.klug@ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-13-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 31 Mar 2025 18:44:31 +0100","Message-ID":"<174344307107.3394313.13915108310238565915@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33831,"web_url":"https://patchwork.libcamera.org/comment/33831/","msgid":"<20250401012901.GU14432@pendragon.ideasonboard.com>","date":"2025-04-01T01:29:01","subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Wed, Mar 19, 2025 at 05:11:17PM +0100, Stefan Klug wrote:\n> In the grey world AWB case, if no colour gains are contained in the\n> tuning file, the color gains get reset to 1 when the colour temperature\n> is set manually. This is unexpected and undesirable. Allow the\n> gainsFromColourTemp() function to return a std::nullopt to handle that\n> case.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v2:\n> - Added this patch\n> ---\n> src/ipa/libipa/awb.cpp | 2 +-\n> src/ipa/libipa/awb.h | 2 +-\n> src/ipa/libipa/awb_bayes.cpp | 4 ++--\n> src/ipa/libipa/awb_bayes.h | 2 +-\n> src/ipa/libipa/awb_grey.cpp | 6 +++---\n> src/ipa/libipa/awb_grey.h | 2 +-\n> src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----\n> 7 files changed, 20 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp\n> index 925fac232709..214bac8b56a6 100644\n> --- a/src/ipa/libipa/awb.cpp\n> +++ b/src/ipa/libipa/awb.cpp\n> @@ -114,7 +114,7 @@ namespace ipa {\n> * does not take any statistics into account. It is used to compute the colour\n> * gains when the user manually specifies a colour temperature.\n> *\n> - * \\return The colour gains\n> + * \\return The colour gains or std::nullopt if the conversion is not possible\n> */\n> \n> /**\n> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> index 4bab7a451ce5..789a1d534f9a 100644\n> --- a/src/ipa/libipa/awb.h\n> +++ b/src/ipa/libipa/awb.h\n> @@ -39,7 +39,7 @@ public:\n> \n> \tvirtual int init(const YamlObject &tuningData) = 0;\n> \tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> -\tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> +\tvirtual std::optional> gainsFromColourTemperature(double colourTemperature) = 0;\n\nYou need to include \n\n> \n> \tconst ControlInfoMap::Map &controls() const\n> \t{\n> diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> index d1d0eaf0e68f..d2bcbd83d7f8 100644\n> --- a/src/ipa/libipa/awb_bayes.cpp\n> +++ b/src/ipa/libipa/awb_bayes.cpp\n> @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)\n> \t}\n> }\n> \n> -RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> +std::optional> AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> {\n> \t/*\n> \t * \\todo In the RaspberryPi code, the ct curve was interpolated in\n> @@ -278,7 +278,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> \t * intuitive, as the gains are in linear space. But I can't prove it.\n> \t */\n> \tconst auto &gains = colourGainCurve_.getInterpolated(colourTemperature);\n> -\treturn { { gains[0], 1.0, gains[1] } };\n> +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> }\n> \n> AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> index bb933038d6d2..47ef3cce4d58 100644\n> --- a/src/ipa/libipa/awb_bayes.h\n> +++ b/src/ipa/libipa/awb_bayes.h\n> @@ -27,7 +27,7 @@ public:\n> \n> \tint init(const YamlObject &tuningData) override;\n> \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> -\tRGB gainsFromColourTemperature(double temperatureK) override;\n> +\tstd::optional> gainsFromColourTemperature(double temperatureK) override;\n> \tvoid handleControls(const ControlList &controls) override;\n> \n> private:\n> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> index d3d2132b8869..d252edb2b6c6 100644\n> --- a/src/ipa/libipa/awb_grey.cpp\n> +++ b/src/ipa/libipa/awb_grey.cpp\n> @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned\n> * \\return The colour gains if a colour temperature curve is available,\n> * [1, 1, 1] otherwise.\n> */\n> -RGB AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> +std::optional> AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> {\n> \tif (!colourGainCurve_) {\n> \t\tLOG(Awb, Error) << \"No gains defined\";\n> -\t\treturn RGB({ 1.0, 1.0, 1.0 });\n> +\t\treturn std::nullopt;\n> \t}\n> \n> \tauto gains = colourGainCurve_->getInterpolated(colourTemperature);\n> -\treturn { { gains[0], 1.0, gains[1] } };\n> +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> }\n> \n> } /* namespace ipa */\n> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> index 7ec7bfa5da9a..f82a368d11cc 100644\n> --- a/src/ipa/libipa/awb_grey.h\n> +++ b/src/ipa/libipa/awb_grey.h\n> @@ -25,7 +25,7 @@ public:\n> \n> \tint init(const YamlObject &tuningData) override;\n> \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> -\tRGB gainsFromColourTemperature(double colourTemperature) override;\n> +\tstd::optional> gainsFromColourTemperature(double colourTemperature) override;\n> \n> private:\n> \tstd::optional>> colourGainCurve_;\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 5e067e50cd52..58b8370d2c61 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,\n> \t\t const IPACameraSensorInfo &configInfo)\n> {\n> \tcontext.activeState.awb.manual.gains = RGB{ 1.0 };\n> -\tcontext.activeState.awb.automatic.gains =\n> -\t\tawbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> +\tauto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> +\tif (gains)\n> +\t\tcontext.activeState.awb.automatic.gains = *gains;\n> +\telse\n> +\t\tcontext.activeState.awb.automatic.gains = RGB{ 1.0 };\n> +\n> \tcontext.activeState.awb.autoEnabled = true;\n> \tcontext.activeState.awb.manual.temperatureK = kDefaultColourTemperature;\n> \tcontext.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;\n> @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,\n> \t\tupdate = true;\n> \t} else if (colourTemperature) {\n> \t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> -\t\tawb.manual.gains.r() = gains.r();\n> -\t\tawb.manual.gains.b() = gains.b();\n> +\t\tif (gains) {\n> +\t\t\tawb.manual.gains.r() = gains->r();\n> +\t\t\tawb.manual.gains.b() = gains->b();\n> +\t\t\tupdate = true;\n> +\t\t}\n> \t\tawb.manual.temperatureK = *colourTemperature;\n\nShouldn't we skip the colour temperature update too ?\n\n> -\t\tupdate = true;\n> \t}\n> \n> \tif (update)","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 1FE00C3213\n\tfor ;\n\tTue, 1 Apr 2025 01:29:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3041F6897A;\n\tTue, 1 Apr 2025 03:29:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D83662C66\n\tfor ;\n\tTue, 1 Apr 2025 03:29:27 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-147-224-nat.elisa-mobile.fi\n\t[85.76.147.224])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4711F6F9;\n\tTue, 1 Apr 2025 03:27:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C9/OoA5W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743470855;\n\tbh=1Fr6ekniFnlcGEnI2ZCI4pFM47fOEObZzbqzHSZaZZQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C9/OoA5W/Vzva0GovZAmkfUKHFPzr1LjE1cILFL7riViyVRvpEWysfCAU1QA5dbnM\n\tvHL6f/rZhAdhMdSWwXFko4hzUQrON0Abvn0obdkuEsJlxstCCR6CGpY+gsI849Ew7+\n\teelcpHILTiTgYRKHOqU6YL+QmT0y8KxEvvm7g2X0=","Date":"Tue, 1 Apr 2025 04:29:01 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","Message-ID":"<20250401012901.GU14432@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-13-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250319161152.63625-13-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33891,"web_url":"https://patchwork.libcamera.org/comment/33891/","msgid":"","date":"2025-04-02T15:01:22","subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Tue, Apr 01, 2025 at 04:29:01AM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 19, 2025 at 05:11:17PM +0100, Stefan Klug wrote:\n> > In the grey world AWB case, if no colour gains are contained in the\n> > tuning file, the color gains get reset to 1 when the colour temperature\n> > is set manually. This is unexpected and undesirable. Allow the\n> > gainsFromColourTemp() function to return a std::nullopt to handle that\n> > case.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Added this patch\n> > ---\n> > src/ipa/libipa/awb.cpp | 2 +-\n> > src/ipa/libipa/awb.h | 2 +-\n> > src/ipa/libipa/awb_bayes.cpp | 4 ++--\n> > src/ipa/libipa/awb_bayes.h | 2 +-\n> > src/ipa/libipa/awb_grey.cpp | 6 +++---\n> > src/ipa/libipa/awb_grey.h | 2 +-\n> > src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----\n> > 7 files changed, 20 insertions(+), 14 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp\n> > index 925fac232709..214bac8b56a6 100644\n> > --- a/src/ipa/libipa/awb.cpp\n> > +++ b/src/ipa/libipa/awb.cpp\n> > @@ -114,7 +114,7 @@ namespace ipa {\n> > * does not take any statistics into account. It is used to compute the colour\n> > * gains when the user manually specifies a colour temperature.\n> > *\n> > - * \\return The colour gains\n> > + * \\return The colour gains or std::nullopt if the conversion is not possible\n> > */\n> > \n> > /**\n> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > index 4bab7a451ce5..789a1d534f9a 100644\n> > --- a/src/ipa/libipa/awb.h\n> > +++ b/src/ipa/libipa/awb.h\n> > @@ -39,7 +39,7 @@ public:\n> > \n> > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > \tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > -\tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > +\tvirtual std::optional> gainsFromColourTemperature(double colourTemperature) = 0;\n> \n> You need to include \n\nRight, and remove it from rkisp1/awb.h.\n\n> \n> > \n> > \tconst ControlInfoMap::Map &controls() const\n> > \t{\n> > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > index d1d0eaf0e68f..d2bcbd83d7f8 100644\n> > --- a/src/ipa/libipa/awb_bayes.cpp\n> > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)\n> > \t}\n> > }\n> > \n> > -RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > +std::optional> AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > {\n> > \t/*\n> > \t * \\todo In the RaspberryPi code, the ct curve was interpolated in\n> > @@ -278,7 +278,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > \t * intuitive, as the gains are in linear space. But I can't prove it.\n> > \t */\n> > \tconst auto &gains = colourGainCurve_.getInterpolated(colourTemperature);\n> > -\treturn { { gains[0], 1.0, gains[1] } };\n> > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > }\n> > \n> > AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > index bb933038d6d2..47ef3cce4d58 100644\n> > --- a/src/ipa/libipa/awb_bayes.h\n> > +++ b/src/ipa/libipa/awb_bayes.h\n> > @@ -27,7 +27,7 @@ public:\n> > \n> > \tint init(const YamlObject &tuningData) override;\n> > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > -\tRGB gainsFromColourTemperature(double temperatureK) override;\n> > +\tstd::optional> gainsFromColourTemperature(double temperatureK) override;\n> > \tvoid handleControls(const ControlList &controls) override;\n> > \n> > private:\n> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > index d3d2132b8869..d252edb2b6c6 100644\n> > --- a/src/ipa/libipa/awb_grey.cpp\n> > +++ b/src/ipa/libipa/awb_grey.cpp\n> > @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned\n> > * \\return The colour gains if a colour temperature curve is available,\n> > * [1, 1, 1] otherwise.\n> > */\n> > -RGB AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > +std::optional> AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > {\n> > \tif (!colourGainCurve_) {\n> > \t\tLOG(Awb, Error) << \"No gains defined\";\n> > -\t\treturn RGB({ 1.0, 1.0, 1.0 });\n> > +\t\treturn std::nullopt;\n> > \t}\n> > \n> > \tauto gains = colourGainCurve_->getInterpolated(colourTemperature);\n> > -\treturn { { gains[0], 1.0, gains[1] } };\n> > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > }\n> > \n> > } /* namespace ipa */\n> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > index 7ec7bfa5da9a..f82a368d11cc 100644\n> > --- a/src/ipa/libipa/awb_grey.h\n> > +++ b/src/ipa/libipa/awb_grey.h\n> > @@ -25,7 +25,7 @@ public:\n> > \n> > \tint init(const YamlObject &tuningData) override;\n> > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > -\tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > +\tstd::optional> gainsFromColourTemperature(double colourTemperature) override;\n> > \n> > private:\n> > \tstd::optional>> colourGainCurve_;\n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 5e067e50cd52..58b8370d2c61 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,\n> > \t\t const IPACameraSensorInfo &configInfo)\n> > {\n> > \tcontext.activeState.awb.manual.gains = RGB{ 1.0 };\n> > -\tcontext.activeState.awb.automatic.gains =\n> > -\t\tawbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > +\tauto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > +\tif (gains)\n> > +\t\tcontext.activeState.awb.automatic.gains = *gains;\n> > +\telse\n> > +\t\tcontext.activeState.awb.automatic.gains = RGB{ 1.0 };\n> > +\n> > \tcontext.activeState.awb.autoEnabled = true;\n> > \tcontext.activeState.awb.manual.temperatureK = kDefaultColourTemperature;\n> > \tcontext.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;\n> > @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,\n> > \t\tupdate = true;\n> > \t} else if (colourTemperature) {\n> > \t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> > -\t\tawb.manual.gains.r() = gains.r();\n> > -\t\tawb.manual.gains.b() = gains.b();\n> > +\t\tif (gains) {\n> > +\t\t\tawb.manual.gains.r() = gains->r();\n> > +\t\t\tawb.manual.gains.b() = gains->b();\n> > +\t\t\tupdate = true;\n> > +\t\t}\n> > \t\tawb.manual.temperatureK = *colourTemperature;\n> \n> Shouldn't we skip the colour temperature update too ?\n\nI don't think so, as the temperature is still reported in metadata. And\nit arrived correctly in libcamera, only awb can't create gains out of\nit. Oh and ccm might still use it. So it makes sense to report it in\nmetadata.\n\nBest regards,\nStefan\n\n> \n> > -\t\tupdate = true;\n> > \t}\n> > \n> > \tif (update)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 1415AC323E\n\tfor ;\n\tWed, 2 Apr 2025 15:01:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 000E16898E;\n\tWed, 2 Apr 2025 17:01:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20C1368979\n\tfor ;\n\tWed, 2 Apr 2025 17:01:25 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3e5a:57a6:9731:f378])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8F276A2;\n\tWed, 2 Apr 2025 16:59:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o0xN2Q3l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743605972;\n\tbh=w1hBAcbDZRDOC4IOvYzNd/vsCh+OVDY3uULQ8nWfxks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o0xN2Q3l+DLbP+3Wl8t3d4fYPvLWX72imTPO3HL9YSZlJH5zSThren7eD7lRHGkY8\n\tLg1fRCNcN6V5WQs56TjAjxmF4GJs0zqXSKuqctoAh3FHc1c28VIKt7aDoNvZqGu9T5\n\tXFPqfUrjzSnAYQC1kb1GwnEhSUkpwWjR4AyHtRv8=","Date":"Wed, 2 Apr 2025 17:01:22 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","Message-ID":"","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-13-stefan.klug@ideasonboard.com>\n\t<20250401012901.GU14432@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250401012901.GU14432@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33892,"web_url":"https://patchwork.libcamera.org/comment/33892/","msgid":"<20250402150621.GE13762@pendragon.ideasonboard.com>","date":"2025-04-02T15:06:21","subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Apr 02, 2025 at 05:01:22PM +0200, Stefan Klug wrote:\n> On Tue, Apr 01, 2025 at 04:29:01AM +0300, Laurent Pinchart wrote:\n> > On Wed, Mar 19, 2025 at 05:11:17PM +0100, Stefan Klug wrote:\n> > > In the grey world AWB case, if no colour gains are contained in the\n> > > tuning file, the color gains get reset to 1 when the colour temperature\n> > > is set manually. This is unexpected and undesirable. Allow the\n> > > gainsFromColourTemp() function to return a std::nullopt to handle that\n> > > case.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - Added this patch\n> > > ---\n> > > src/ipa/libipa/awb.cpp | 2 +-\n> > > src/ipa/libipa/awb.h | 2 +-\n> > > src/ipa/libipa/awb_bayes.cpp | 4 ++--\n> > > src/ipa/libipa/awb_bayes.h | 2 +-\n> > > src/ipa/libipa/awb_grey.cpp | 6 +++---\n> > > src/ipa/libipa/awb_grey.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----\n> > > 7 files changed, 20 insertions(+), 14 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp\n> > > index 925fac232709..214bac8b56a6 100644\n> > > --- a/src/ipa/libipa/awb.cpp\n> > > +++ b/src/ipa/libipa/awb.cpp\n> > > @@ -114,7 +114,7 @@ namespace ipa {\n> > > * does not take any statistics into account. It is used to compute the colour\n> > > * gains when the user manually specifies a colour temperature.\n> > > *\n> > > - * \\return The colour gains\n> > > + * \\return The colour gains or std::nullopt if the conversion is not possible\n> > > */\n> > > \n> > > /**\n> > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > > index 4bab7a451ce5..789a1d534f9a 100644\n> > > --- a/src/ipa/libipa/awb.h\n> > > +++ b/src/ipa/libipa/awb.h\n> > > @@ -39,7 +39,7 @@ public:\n> > > \n> > > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > > \tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > > -\tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > > +\tvirtual std::optional> gainsFromColourTemperature(double colourTemperature) = 0;\n> > \n> > You need to include \n> \n> Right, and remove it from rkisp1/awb.h.\n> \n> > > \n> > > \tconst ControlInfoMap::Map &controls() const\n> > > \t{\n> > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > > index d1d0eaf0e68f..d2bcbd83d7f8 100644\n> > > --- a/src/ipa/libipa/awb_bayes.cpp\n> > > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > > @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)\n> > > \t}\n> > > }\n> > > \n> > > -RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > +std::optional> AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > {\n> > > \t/*\n> > > \t * \\todo In the RaspberryPi code, the ct curve was interpolated in\n> > > @@ -278,7 +278,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > \t * intuitive, as the gains are in linear space. But I can't prove it.\n> > > \t */\n> > > \tconst auto &gains = colourGainCurve_.getInterpolated(colourTemperature);\n> > > -\treturn { { gains[0], 1.0, gains[1] } };\n> > > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > > }\n> > > \n> > > AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > > index bb933038d6d2..47ef3cce4d58 100644\n> > > --- a/src/ipa/libipa/awb_bayes.h\n> > > +++ b/src/ipa/libipa/awb_bayes.h\n> > > @@ -27,7 +27,7 @@ public:\n> > > \n> > > \tint init(const YamlObject &tuningData) override;\n> > > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > -\tRGB gainsFromColourTemperature(double temperatureK) override;\n> > > +\tstd::optional> gainsFromColourTemperature(double temperatureK) override;\n> > > \tvoid handleControls(const ControlList &controls) override;\n> > > \n> > > private:\n> > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > > index d3d2132b8869..d252edb2b6c6 100644\n> > > --- a/src/ipa/libipa/awb_grey.cpp\n> > > +++ b/src/ipa/libipa/awb_grey.cpp\n> > > @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned\n> > > * \\return The colour gains if a colour temperature curve is available,\n> > > * [1, 1, 1] otherwise.\n> > > */\n> > > -RGB AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > > +std::optional> AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > > {\n> > > \tif (!colourGainCurve_) {\n> > > \t\tLOG(Awb, Error) << \"No gains defined\";\n> > > -\t\treturn RGB({ 1.0, 1.0, 1.0 });\n> > > +\t\treturn std::nullopt;\n> > > \t}\n> > > \n> > > \tauto gains = colourGainCurve_->getInterpolated(colourTemperature);\n> > > -\treturn { { gains[0], 1.0, gains[1] } };\n> > > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > > }\n> > > \n> > > } /* namespace ipa */\n> > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > > index 7ec7bfa5da9a..f82a368d11cc 100644\n> > > --- a/src/ipa/libipa/awb_grey.h\n> > > +++ b/src/ipa/libipa/awb_grey.h\n> > > @@ -25,7 +25,7 @@ public:\n> > > \n> > > \tint init(const YamlObject &tuningData) override;\n> > > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > -\tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > > +\tstd::optional> gainsFromColourTemperature(double colourTemperature) override;\n> > > \n> > > private:\n> > > \tstd::optional>> colourGainCurve_;\n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 5e067e50cd52..58b8370d2c61 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,\n> > > \t\t const IPACameraSensorInfo &configInfo)\n> > > {\n> > > \tcontext.activeState.awb.manual.gains = RGB{ 1.0 };\n> > > -\tcontext.activeState.awb.automatic.gains =\n> > > -\t\tawbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > > +\tauto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > > +\tif (gains)\n> > > +\t\tcontext.activeState.awb.automatic.gains = *gains;\n> > > +\telse\n> > > +\t\tcontext.activeState.awb.automatic.gains = RGB{ 1.0 };\n> > > +\n> > > \tcontext.activeState.awb.autoEnabled = true;\n> > > \tcontext.activeState.awb.manual.temperatureK = kDefaultColourTemperature;\n> > > \tcontext.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;\n> > > @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,\n> > > \t\tupdate = true;\n> > > \t} else if (colourTemperature) {\n> > > \t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> > > -\t\tawb.manual.gains.r() = gains.r();\n> > > -\t\tawb.manual.gains.b() = gains.b();\n> > > +\t\tif (gains) {\n> > > +\t\t\tawb.manual.gains.r() = gains->r();\n> > > +\t\t\tawb.manual.gains.b() = gains->b();\n> > > +\t\t\tupdate = true;\n> > > +\t\t}\n> > > \t\tawb.manual.temperatureK = *colourTemperature;\n> > \n> > Shouldn't we skip the colour temperature update too ?\n> \n> I don't think so, as the temperature is still reported in metadata. And\n> it arrived correctly in libcamera, only awb can't create gains out of\n> it. Oh and ccm might still use it. So it makes sense to report it in\n> metadata.\n\nThe policy is to adjust the value of controls requested by the user to\nwhat makes sense for the camera. This can include simply clamping to a\nvalid range, or ignoring the control completely (for instance for enum\ncontrols, as there's no way for libcamera to meaningfully guess how to\nadjust an invalid enum value). If the colour temperature is such that\ncolour gains can't be computed, this patch ignores the effect of the\ncolour temperature on the gains. Reporting the requested colour\ntemperature in metadata as if it was fine doesn't seem consistent with\nthe behaviour of the AWB algorithm.\n\n> > > -\t\tupdate = true;\n> > > \t}\n> > > \n> > > \tif (update)","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 16B13C3213\n\tfor ;\n\tWed, 2 Apr 2025 15:06:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31B8568996;\n\tWed, 2 Apr 2025 17:06:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5462568979\n\tfor ;\n\tWed, 2 Apr 2025 17:06:46 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E44D415;\n\tWed, 2 Apr 2025 17:04:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HIoYkfV1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743606293;\n\tbh=1hEx7Bz9ImnDzYhh5553W7JC9taIsTpi1duuSleu7Iw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HIoYkfV1+kYfoHZOwQoC30ndYw8hcvySjeiFEPQ8kfOUMG7evHj2Xp74X+hq7OjrM\n\trN/cC0deMD2lV85pnw21wEwOHw6ikpRIRhVrOYFADaHu31qgLXGiW78Z1D1eJ1z+Qo\n\tNyKOKq29OMkUeBNavw3BhONfVPm/qaKuO+c3wxEg=","Date":"Wed, 2 Apr 2025 18:06:21 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","Message-ID":"<20250402150621.GE13762@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-13-stefan.klug@ideasonboard.com>\n\t<20250401012901.GU14432@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33905,"web_url":"https://patchwork.libcamera.org/comment/33905/","msgid":"","date":"2025-04-03T09:50:59","subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Apr 02, 2025 at 06:06:21PM +0300, Laurent Pinchart wrote:\n> On Wed, Apr 02, 2025 at 05:01:22PM +0200, Stefan Klug wrote:\n> > On Tue, Apr 01, 2025 at 04:29:01AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Mar 19, 2025 at 05:11:17PM +0100, Stefan Klug wrote:\n> > > > In the grey world AWB case, if no colour gains are contained in the\n> > > > tuning file, the color gains get reset to 1 when the colour temperature\n> > > > is set manually. This is unexpected and undesirable. Allow the\n> > > > gainsFromColourTemp() function to return a std::nullopt to handle that\n> > > > case.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > \n> > > > ---\n> > > > \n> > > > Changes in v2:\n> > > > - Added this patch\n> > > > ---\n> > > > src/ipa/libipa/awb.cpp | 2 +-\n> > > > src/ipa/libipa/awb.h | 2 +-\n> > > > src/ipa/libipa/awb_bayes.cpp | 4 ++--\n> > > > src/ipa/libipa/awb_bayes.h | 2 +-\n> > > > src/ipa/libipa/awb_grey.cpp | 6 +++---\n> > > > src/ipa/libipa/awb_grey.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----\n> > > > 7 files changed, 20 insertions(+), 14 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp\n> > > > index 925fac232709..214bac8b56a6 100644\n> > > > --- a/src/ipa/libipa/awb.cpp\n> > > > +++ b/src/ipa/libipa/awb.cpp\n> > > > @@ -114,7 +114,7 @@ namespace ipa {\n> > > > * does not take any statistics into account. It is used to compute the colour\n> > > > * gains when the user manually specifies a colour temperature.\n> > > > *\n> > > > - * \\return The colour gains\n> > > > + * \\return The colour gains or std::nullopt if the conversion is not possible\n> > > > */\n> > > > \n> > > > /**\n> > > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > > > index 4bab7a451ce5..789a1d534f9a 100644\n> > > > --- a/src/ipa/libipa/awb.h\n> > > > +++ b/src/ipa/libipa/awb.h\n> > > > @@ -39,7 +39,7 @@ public:\n> > > > \n> > > > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > > > \tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > > > -\tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > > > +\tvirtual std::optional> gainsFromColourTemperature(double colourTemperature) = 0;\n> > > \n> > > You need to include \n> > \n> > Right, and remove it from rkisp1/awb.h.\n> > \n> > > > \n> > > > \tconst ControlInfoMap::Map &controls() const\n> > > > \t{\n> > > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > > > index d1d0eaf0e68f..d2bcbd83d7f8 100644\n> > > > --- a/src/ipa/libipa/awb_bayes.cpp\n> > > > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > > > @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)\n> > > > \t}\n> > > > }\n> > > > \n> > > > -RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > > +std::optional> AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > > {\n> > > > \t/*\n> > > > \t * \\todo In the RaspberryPi code, the ct curve was interpolated in\n> > > > @@ -278,7 +278,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > > \t * intuitive, as the gains are in linear space. But I can't prove it.\n> > > > \t */\n> > > > \tconst auto &gains = colourGainCurve_.getInterpolated(colourTemperature);\n> > > > -\treturn { { gains[0], 1.0, gains[1] } };\n> > > > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > > > }\n> > > > \n> > > > AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > > > index bb933038d6d2..47ef3cce4d58 100644\n> > > > --- a/src/ipa/libipa/awb_bayes.h\n> > > > +++ b/src/ipa/libipa/awb_bayes.h\n> > > > @@ -27,7 +27,7 @@ public:\n> > > > \n> > > > \tint init(const YamlObject &tuningData) override;\n> > > > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > > -\tRGB gainsFromColourTemperature(double temperatureK) override;\n> > > > +\tstd::optional> gainsFromColourTemperature(double temperatureK) override;\n> > > > \tvoid handleControls(const ControlList &controls) override;\n> > > > \n> > > > private:\n> > > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > > > index d3d2132b8869..d252edb2b6c6 100644\n> > > > --- a/src/ipa/libipa/awb_grey.cpp\n> > > > +++ b/src/ipa/libipa/awb_grey.cpp\n> > > > @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned\n> > > > * \\return The colour gains if a colour temperature curve is available,\n> > > > * [1, 1, 1] otherwise.\n> > > > */\n> > > > -RGB AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > > > +std::optional> AwbGrey::gainsFromColourTemperature(double colourTemperature)\n> > > > {\n> > > > \tif (!colourGainCurve_) {\n> > > > \t\tLOG(Awb, Error) << \"No gains defined\";\n> > > > -\t\treturn RGB({ 1.0, 1.0, 1.0 });\n> > > > +\t\treturn std::nullopt;\n> > > > \t}\n> > > > \n> > > > \tauto gains = colourGainCurve_->getInterpolated(colourTemperature);\n> > > > -\treturn { { gains[0], 1.0, gains[1] } };\n> > > > +\treturn RGB{ { gains[0], 1.0, gains[1] } };\n> > > > }\n> > > > \n> > > > } /* namespace ipa */\n> > > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > > > index 7ec7bfa5da9a..f82a368d11cc 100644\n> > > > --- a/src/ipa/libipa/awb_grey.h\n> > > > +++ b/src/ipa/libipa/awb_grey.h\n> > > > @@ -25,7 +25,7 @@ public:\n> > > > \n> > > > \tint init(const YamlObject &tuningData) override;\n> > > > \tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > > -\tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > > > +\tstd::optional> gainsFromColourTemperature(double colourTemperature) override;\n> > > > \n> > > > private:\n> > > > \tstd::optional>> colourGainCurve_;\n> > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > index 5e067e50cd52..58b8370d2c61 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,\n> > > > \t\t const IPACameraSensorInfo &configInfo)\n> > > > {\n> > > > \tcontext.activeState.awb.manual.gains = RGB{ 1.0 };\n> > > > -\tcontext.activeState.awb.automatic.gains =\n> > > > -\t\tawbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > > > +\tauto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);\n> > > > +\tif (gains)\n> > > > +\t\tcontext.activeState.awb.automatic.gains = *gains;\n> > > > +\telse\n> > > > +\t\tcontext.activeState.awb.automatic.gains = RGB{ 1.0 };\n> > > > +\n> > > > \tcontext.activeState.awb.autoEnabled = true;\n> > > > \tcontext.activeState.awb.manual.temperatureK = kDefaultColourTemperature;\n> > > > \tcontext.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;\n> > > > @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,\n> > > > \t\tupdate = true;\n> > > > \t} else if (colourTemperature) {\n> > > > \t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> > > > -\t\tawb.manual.gains.r() = gains.r();\n> > > > -\t\tawb.manual.gains.b() = gains.b();\n> > > > +\t\tif (gains) {\n> > > > +\t\t\tawb.manual.gains.r() = gains->r();\n> > > > +\t\t\tawb.manual.gains.b() = gains->b();\n> > > > +\t\t\tupdate = true;\n> > > > +\t\t}\n> > > > \t\tawb.manual.temperatureK = *colourTemperature;\n> > > \n> > > Shouldn't we skip the colour temperature update too ?\n> > \n> > I don't think so, as the temperature is still reported in metadata. And\n> > it arrived correctly in libcamera, only awb can't create gains out of\n> > it. Oh and ccm might still use it. So it makes sense to report it in\n> > metadata.\n> \n> The policy is to adjust the value of controls requested by the user to\n> what makes sense for the camera. This can include simply clamping to a\n> valid range, or ignoring the control completely (for instance for enum\n> controls, as there's no way for libcamera to meaningfully guess how to\n> adjust an invalid enum value). If the colour temperature is such that\n> colour gains can't be computed, this patch ignores the effect of the\n> colour temperature on the gains. Reporting the requested colour\n> temperature in metadata as if it was fine doesn't seem consistent with\n> the behaviour of the AWB algorithm.\n\nYes, but the CCM algorithm still handles colour temperature control and\nadjusts the CCM accordingly (but doesn't set the metadata). The LSC\nalgorithm directly accesses frameContext.awb.temperatureK to apply the\ncorrect table (and doesn't set the metadata). So we don't set it for the\nAWB algorithm only. Only if there is no CCM and no LSC algorithm and no\ncolour gains in AWB, the colour temperature could be omitted from\nmetadata as it didn't do anything. But I don't think the required logic\nfor that is worth it. What do you think?\n\nBest regards,\nStefan\n\n> \n> > > > -\t\tupdate = true;\n> > > > \t}\n> > > > \n> > > > \tif (update)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 6454CC327D\n\tfor ;\n\tThu, 3 Apr 2025 09:51:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 934E868994;\n\tThu, 3 Apr 2025 11:51:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D8F268967\n\tfor ;\n\tThu, 3 Apr 2025 11:51:02 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6d9d:9854:3fc1:4bb2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE8808FA;\n\tThu, 3 Apr 2025 11:49:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PldtyFut\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743673748;\n\tbh=GHfUrxb+xFZO4xXFwiK/lKLBimo2OkxM4hMtM9UMApE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PldtyFutzOkH7mt8ZgkgxSrRSccXkmeazM+5BspQsW7Y/YJpHSUR+ba5qeshOVLBv\n\tAJDCKzRJEd76UleWsvRz9xzKVH271vN978VYW9UG7h9g6QM3c2L1KNqnwxMbhyQLlQ\n\tT7AGrAn4aeiYVMqRmLP5C6AQjbOvCzevWL2P9Fsc=","Date":"Thu, 3 Apr 2025 11:50:59 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp\n\toptional","Message-ID":"","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-13-stefan.klug@ideasonboard.com>\n\t<20250401012901.GU14432@pendragon.ideasonboard.com>\n\t\n\t<20250402150621.GE13762@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250402150621.GE13762@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]