[{"id":32893,"web_url":"https://patchwork.libcamera.org/comment/32893/","msgid":"<20241220095457.GB6745@pendragon.ideasonboard.com>","date":"2024-12-20T09:54:57","subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","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 Thu, Dec 19, 2024 at 06:57:20PM +0100, Stefan Klug wrote:\n> There are many use-cases (tuning-validation, working in static\n> environments) where a manual ColourTemperature control is helpful.\n> Implement that by interpolating and applying the white balance gains\n> from the tuning file according to the requested colour temperature. If\n> colour gains are provided on the same request, they take precedence.\n> Store the colour temperature used for a given frame in the frame context\n> and report that in metadata.\n> \n> Note that in the automatic case, the colour gains are still based on the\n> gray world model and the CT curve from the tuning file get ignored.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Paul Elder \n> Reviewed-by: Kieran Bingham \n> \n> ---\n> \n> Changes in v6:\n> - Updated commit message\n> - Moved retrieval of controls to the place where needed\n> - Output the colour temperature applied to a frame in metadata\n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 51 +++++++++++++++++++++++--------\n> src/ipa/rkisp1/ipa_context.h | 1 +\n> 2 files changed, 40 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index e23f67a96edf..cffaa06a22c1 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Awb)\n> \n> +constexpr int32_t kMinColourTemperature = 2500;\n> +constexpr int32_t kMaxColourTemperature = 10000;\n> +constexpr int32_t kDefaultColourTemperature = 5000;\n> +\n> /* Minimum mean value below which AWB can't operate. */\n> constexpr double kMeanMinThreshold = 2.0;\n> \n> @@ -44,8 +48,13 @@ Awb::Awb()\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::init\n> */\n> -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> {\n> +\tauto &cmap = context.ctrlMap;\n> +\tcmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> +\t\t\t\t\t\t\t kMaxColourTemperature,\n> +\t\t\t\t\t\t\t kDefaultColourTemperature);\n> +\n> \tInterpolator> gainCurve;\n> \tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> \tif (ret < 0)\n> @@ -68,6 +77,7 @@ int Awb::configure(IPAContext &context,\n> \tcontext.activeState.awb.gains.manual = RGB{ 1.0 };\n> \tcontext.activeState.awb.gains.automatic = RGB{ 1.0 };\n> \tcontext.activeState.awb.autoEnabled = true;\n> +\tcontext.activeState.awb.temperatureK = kDefaultColourTemperature;\n> \n> \t/*\n> \t * Define the measurement window for AWB as a centered rectangle\n> @@ -101,19 +111,37 @@ void Awb::queueRequest(IPAContext &context,\n> \t\t\t<< (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> \t}\n> \n> +\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> +\n> +\tif (awb.autoEnabled)\n> +\t\treturn;\n> +\n> \tconst auto &colourGains = controls.get(controls::ColourGains);\n> -\tif (colourGains && !awb.autoEnabled) {\n> +\tconst auto &colourTemperature = controls.get(controls::ColourTemperature);\n> +\tbool update = false;\n> +\tif (colourGains) {\n> \t\tawb.gains.manual.r() = (*colourGains)[0];\n> \t\tawb.gains.manual.b() = (*colourGains)[1];\n> +\t\t/*\n> +\t\t * \\todo: Colour temperature reported in metadata is now\n> +\t\t * incorrect, as we can't deduce the temperature from the gains.\n> +\t\t * This will be fixed with the bayes AWB algorithm.\n> +\t\t */\n\nWill we require all cameras to implement bayesian AWB ? If not, the the\ncontrols documentation need to allow that. I suppose that\nColourTemperature will then not be reported as a settable control by the\ncamera, so application will know.\n\nI'm fine clarifying the documentation on top of this series. Can you\nmake sure it gets done ?\n\nReviewed-by: Laurent Pinchart \n\n> +\t\tupdate = true;\n> +\t} else if (colourTemperature && colourGainCurve_) {\n> +\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> +\t\tawb.gains.manual.r() = gains[0];\n> +\t\tawb.gains.manual.b() = gains[1];\n> +\t\tawb.temperatureK = *colourTemperature;\n> +\t\tupdate = true;\n> +\t}\n> \n> +\tif (update)\n> \t\tLOG(RkISP1Awb, Debug)\n> \t\t\t<< \"Set colour gains to \" << awb.gains.manual;\n> -\t}\n> \n> -\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> -\n> -\tif (!awb.autoEnabled)\n> -\t\tframeContext.awb.gains = awb.gains.manual;\n> +\tframeContext.awb.gains = awb.gains.manual;\n> +\tframeContext.awb.temperatureK = awb.temperatureK;\n> }\n> \n> /**\n> @@ -126,8 +154,10 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> \t * This is the latest time we can read the active state. This is the\n> \t * most up-to-date automatic values we can read.\n> \t */\n> -\tif (frameContext.awb.autoEnabled)\n> +\tif (frameContext.awb.autoEnabled) {\n> \t\tframeContext.awb.gains = context.activeState.awb.gains.automatic;\n> +\t\tframeContext.awb.temperatureK = context.activeState.awb.temperatureK;\n> +\t}\n> \n> \tauto gainConfig = params->block();\n> \tgainConfig.setEnabled(true);\n> @@ -206,7 +236,7 @@ void Awb::process(IPAContext &context,\n> \t\t\tstatic_cast(frameContext.awb.gains.r()),\n> \t\t\tstatic_cast(frameContext.awb.gains.b())\n> \t\t});\n> -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> +\tmetadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);\n> \n> \tif (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {\n> \t\tLOG(RkISP1Awb, Error) << \"AWB data is missing in statistics\";\n> @@ -281,9 +311,6 @@ void Awb::process(IPAContext &context,\n> \n> \tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> \n> -\t/* Metadata shall contain the up to date measurement */\n> -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> -\n> \t/*\n> \t * Estimate the red and blue gains to apply in a grey world. The green\n> \t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index deb8c196f1b8..4b50015beee8 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -135,6 +135,7 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tRGB gains;\n> \t\tbool autoEnabled;\n> +\t\tunsigned int temperatureK;\n> \t} awb;\n> \n> \tstruct {","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 A608EC32A3\n\tfor ;\n\tFri, 20 Dec 2024 09:55:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77C4C6848A;\n\tFri, 20 Dec 2024 10:55:06 +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 DF9C362C8A\n\tfor ;\n\tFri, 20 Dec 2024 10:55:03 +0100 (CET)","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 4BC59ABE;\n\tFri, 20 Dec 2024 10:54:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S92znYab\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734688464;\n\tbh=I2lUVBODDF+Rqb+cc2eB4QFNhx2kuGTbgYsUmXwb4rE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S92znYabvwMFFD6nOTLt8rp2pd9WnQbZ2qRW9+cuEOvYnsI5TfhVxVj2aMTXo+Wyf\n\tugQW9fM1JI4No6Q5NzgcmrboA+cv4/UFqnf7JTbCPbyW/O2Xzjk/IP0mRRwQBzHYDB\n\t6IcKZhLEkswRE00+fdJ5pCbuwCbUHUqH09Uj+pNA=","Date":"Fri, 20 Dec 2024 11:54:57 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"<20241220095457.GB6745@pendragon.ideasonboard.com>","References":"<20241219175729.293782-1-stefan.klug@ideasonboard.com>\n\t<20241219175729.293782-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241219175729.293782-4-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":32895,"web_url":"https://patchwork.libcamera.org/comment/32895/","msgid":"","date":"2024-12-20T10:07:14","subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","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 Fri, Dec 20, 2024 at 11:54:57AM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Thu, Dec 19, 2024 at 06:57:20PM +0100, Stefan Klug wrote:\n> > There are many use-cases (tuning-validation, working in static\n> > environments) where a manual ColourTemperature control is helpful.\n> > Implement that by interpolating and applying the white balance gains\n> > from the tuning file according to the requested colour temperature. If\n> > colour gains are provided on the same request, they take precedence.\n> > Store the colour temperature used for a given frame in the frame context\n> > and report that in metadata.\n> > \n> > Note that in the automatic case, the colour gains are still based on the\n> > gray world model and the CT curve from the tuning file get ignored.\n> > \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Paul Elder \n> > Reviewed-by: Kieran Bingham \n> > \n> > ---\n> > \n> > Changes in v6:\n> > - Updated commit message\n> > - Moved retrieval of controls to the place where needed\n> > - Output the colour temperature applied to a frame in metadata\n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 51 +++++++++++++++++++++++--------\n> > src/ipa/rkisp1/ipa_context.h | 1 +\n> > 2 files changed, 40 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index e23f67a96edf..cffaa06a22c1 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > \n> > +constexpr int32_t kMinColourTemperature = 2500;\n> > +constexpr int32_t kMaxColourTemperature = 10000;\n> > +constexpr int32_t kDefaultColourTemperature = 5000;\n> > +\n> > /* Minimum mean value below which AWB can't operate. */\n> > constexpr double kMeanMinThreshold = 2.0;\n> > \n> > @@ -44,8 +48,13 @@ Awb::Awb()\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::init\n> > */\n> > -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > +int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > {\n> > +\tauto &cmap = context.ctrlMap;\n> > +\tcmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > +\t\t\t\t\t\t\t kMaxColourTemperature,\n> > +\t\t\t\t\t\t\t kDefaultColourTemperature);\n> > +\n> > \tInterpolator> gainCurve;\n> > \tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> > \tif (ret < 0)\n> > @@ -68,6 +77,7 @@ int Awb::configure(IPAContext &context,\n> > \tcontext.activeState.awb.gains.manual = RGB{ 1.0 };\n> > \tcontext.activeState.awb.gains.automatic = RGB{ 1.0 };\n> > \tcontext.activeState.awb.autoEnabled = true;\n> > +\tcontext.activeState.awb.temperatureK = kDefaultColourTemperature;\n> > \n> > \t/*\n> > \t * Define the measurement window for AWB as a centered rectangle\n> > @@ -101,19 +111,37 @@ void Awb::queueRequest(IPAContext &context,\n> > \t\t\t<< (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> > \t}\n> > \n> > +\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> > +\n> > +\tif (awb.autoEnabled)\n> > +\t\treturn;\n> > +\n> > \tconst auto &colourGains = controls.get(controls::ColourGains);\n> > -\tif (colourGains && !awb.autoEnabled) {\n> > +\tconst auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > +\tbool update = false;\n> > +\tif (colourGains) {\n> > \t\tawb.gains.manual.r() = (*colourGains)[0];\n> > \t\tawb.gains.manual.b() = (*colourGains)[1];\n> > +\t\t/*\n> > +\t\t * \\todo: Colour temperature reported in metadata is now\n> > +\t\t * incorrect, as we can't deduce the temperature from the gains.\n> > +\t\t * This will be fixed with the bayes AWB algorithm.\n> > +\t\t */\n> \n> Will we require all cameras to implement bayesian AWB ? If not, the the\n> controls documentation need to allow that. I suppose that\n> ColourTemperature will then not be reported as a settable control by the\n> camera, so application will know.\n\nThis case happens when setting the colour gains. So making the\nColourTemperature an output only won't help here. We could stop\nreporting it in metadata. But that is for later.\n\n> \n> I'm fine clarifying the documentation on top of this series. Can you\n> make sure it gets done ?\n> \n> Reviewed-by: Laurent Pinchart \n\nThanks!\n\nCheers,\nStefan\n\n> \n> > +\t\tupdate = true;\n> > +\t} else if (colourTemperature && colourGainCurve_) {\n> > +\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> > +\t\tawb.gains.manual.r() = gains[0];\n> > +\t\tawb.gains.manual.b() = gains[1];\n> > +\t\tawb.temperatureK = *colourTemperature;\n> > +\t\tupdate = true;\n> > +\t}\n> > \n> > +\tif (update)\n> > \t\tLOG(RkISP1Awb, Debug)\n> > \t\t\t<< \"Set colour gains to \" << awb.gains.manual;\n> > -\t}\n> > \n> > -\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> > -\n> > -\tif (!awb.autoEnabled)\n> > -\t\tframeContext.awb.gains = awb.gains.manual;\n> > +\tframeContext.awb.gains = awb.gains.manual;\n> > +\tframeContext.awb.temperatureK = awb.temperatureK;\n> > }\n> > \n> > /**\n> > @@ -126,8 +154,10 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > \t * This is the latest time we can read the active state. This is the\n> > \t * most up-to-date automatic values we can read.\n> > \t */\n> > -\tif (frameContext.awb.autoEnabled)\n> > +\tif (frameContext.awb.autoEnabled) {\n> > \t\tframeContext.awb.gains = context.activeState.awb.gains.automatic;\n> > +\t\tframeContext.awb.temperatureK = context.activeState.awb.temperatureK;\n> > +\t}\n> > \n> > \tauto gainConfig = params->block();\n> > \tgainConfig.setEnabled(true);\n> > @@ -206,7 +236,7 @@ void Awb::process(IPAContext &context,\n> > \t\t\tstatic_cast(frameContext.awb.gains.r()),\n> > \t\t\tstatic_cast(frameContext.awb.gains.b())\n> > \t\t});\n> > -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> > +\tmetadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);\n> > \n> > \tif (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {\n> > \t\tLOG(RkISP1Awb, Error) << \"AWB data is missing in statistics\";\n> > @@ -281,9 +311,6 @@ void Awb::process(IPAContext &context,\n> > \n> > \tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> > \n> > -\t/* Metadata shall contain the up to date measurement */\n> > -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> > -\n> > \t/*\n> > \t * Estimate the red and blue gains to apply in a grey world. The green\n> > \t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index deb8c196f1b8..4b50015beee8 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -135,6 +135,7 @@ struct IPAFrameContext : public FrameContext {\n> > \tstruct {\n> > \t\tRGB gains;\n> > \t\tbool autoEnabled;\n> > +\t\tunsigned int temperatureK;\n> > \t} awb;\n> > \n> > \tstruct {\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 AF9E2C3272\n\tfor ;\n\tFri, 20 Dec 2024 10:07:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BE7A6848F;\n\tFri, 20 Dec 2024 11:07:21 +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 2FF7662C8A\n\tfor ;\n\tFri, 20 Dec 2024 11:07:18 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:4eeb:7fa7:1fd0:c13d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2A7B7E2;\n\tFri, 20 Dec 2024 11:06:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"e7ATEjkZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734689198;\n\tbh=34fwmgoEhJuhYorivfA0DBCmBJw+gSsJyAJAlv+xPh0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e7ATEjkZMh4GYWIi7ylapSNblNEn7VotB4femVn76Y7B4J0eOV2/kLXTkuha9jK+9\n\t1CLjgKp3BwaNrtaBza15fskb1YKPAktSPA9u2n9jm4u5gZN+wu9tnSrgjmDgiGSEde\n\tFEmRuATwPu8+ShpOLo5/YZydVmqo5SCX9LYj6GfY=","Date":"Fri, 20 Dec 2024 11:07:14 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"","References":"<20241219175729.293782-1-stefan.klug@ideasonboard.com>\n\t<20241219175729.293782-4-stefan.klug@ideasonboard.com>\n\t<20241220095457.GB6745@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241220095457.GB6745@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":32896,"web_url":"https://patchwork.libcamera.org/comment/32896/","msgid":"<20241220101658.GD8313@pendragon.ideasonboard.com>","date":"2024-12-20T10:16:58","subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 20, 2024 at 11:07:14AM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> Thank you for the review. \n> \n> On Fri, Dec 20, 2024 at 11:54:57AM +0200, Laurent Pinchart wrote:\n> > Hi Stefan,\n> > \n> > Thank you for the patch.\n> > \n> > On Thu, Dec 19, 2024 at 06:57:20PM +0100, Stefan Klug wrote:\n> > > There are many use-cases (tuning-validation, working in static\n> > > environments) where a manual ColourTemperature control is helpful.\n> > > Implement that by interpolating and applying the white balance gains\n> > > from the tuning file according to the requested colour temperature. If\n> > > colour gains are provided on the same request, they take precedence.\n> > > Store the colour temperature used for a given frame in the frame context\n> > > and report that in metadata.\n> > > \n> > > Note that in the automatic case, the colour gains are still based on the\n> > > gray world model and the CT curve from the tuning file get ignored.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > Reviewed-by: Paul Elder \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > ---\n> > > \n> > > Changes in v6:\n> > > - Updated commit message\n> > > - Moved retrieval of controls to the place where needed\n> > > - Output the colour temperature applied to a frame in metadata\n> > > ---\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 51 +++++++++++++++++++++++--------\n> > > src/ipa/rkisp1/ipa_context.h | 1 +\n> > > 2 files changed, 40 insertions(+), 12 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index e23f67a96edf..cffaa06a22c1 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {\n> > > \n> > > LOG_DEFINE_CATEGORY(RkISP1Awb)\n> > > \n> > > +constexpr int32_t kMinColourTemperature = 2500;\n> > > +constexpr int32_t kMaxColourTemperature = 10000;\n> > > +constexpr int32_t kDefaultColourTemperature = 5000;\n> > > +\n> > > /* Minimum mean value below which AWB can't operate. */\n> > > constexpr double kMeanMinThreshold = 2.0;\n> > > \n> > > @@ -44,8 +48,13 @@ Awb::Awb()\n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::init\n> > > */\n> > > -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > +int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > > {\n> > > +\tauto &cmap = context.ctrlMap;\n> > > +\tcmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,\n> > > +\t\t\t\t\t\t\t kMaxColourTemperature,\n> > > +\t\t\t\t\t\t\t kDefaultColourTemperature);\n> > > +\n> > > \tInterpolator> gainCurve;\n> > > \tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> > > \tif (ret < 0)\n> > > @@ -68,6 +77,7 @@ int Awb::configure(IPAContext &context,\n> > > \tcontext.activeState.awb.gains.manual = RGB{ 1.0 };\n> > > \tcontext.activeState.awb.gains.automatic = RGB{ 1.0 };\n> > > \tcontext.activeState.awb.autoEnabled = true;\n> > > +\tcontext.activeState.awb.temperatureK = kDefaultColourTemperature;\n> > > \n> > > \t/*\n> > > \t * Define the measurement window for AWB as a centered rectangle\n> > > @@ -101,19 +111,37 @@ void Awb::queueRequest(IPAContext &context,\n> > > \t\t\t<< (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> > > \t}\n> > > \n> > > +\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> > > +\n> > > +\tif (awb.autoEnabled)\n> > > +\t\treturn;\n> > > +\n> > > \tconst auto &colourGains = controls.get(controls::ColourGains);\n> > > -\tif (colourGains && !awb.autoEnabled) {\n> > > +\tconst auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > > +\tbool update = false;\n> > > +\tif (colourGains) {\n> > > \t\tawb.gains.manual.r() = (*colourGains)[0];\n> > > \t\tawb.gains.manual.b() = (*colourGains)[1];\n> > > +\t\t/*\n> > > +\t\t * \\todo: Colour temperature reported in metadata is now\n> > > +\t\t * incorrect, as we can't deduce the temperature from the gains.\n> > > +\t\t * This will be fixed with the bayes AWB algorithm.\n> > > +\t\t */\n> > \n> > Will we require all cameras to implement bayesian AWB ? If not, the the\n> > controls documentation need to allow that. I suppose that\n> > ColourTemperature will then not be reported as a settable control by the\n> > camera, so application will know.\n> \n> This case happens when setting the colour gains. So making the\n> ColourTemperature an output only won't help here. We could stop\n> reporting it in metadata. But that is for later.\n\nWhat I meant is that, it CT is output only, we could document that it\ndoesn't get computed from gains (and likely also that it doesn't get\nreported in embedded data in manual mode).\n\n> > I'm fine clarifying the documentation on top of this series. Can you\n> > make sure it gets done ?\n> > \n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks!\n> \n> > > +\t\tupdate = true;\n> > > +\t} else if (colourTemperature && colourGainCurve_) {\n> > > +\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> > > +\t\tawb.gains.manual.r() = gains[0];\n> > > +\t\tawb.gains.manual.b() = gains[1];\n> > > +\t\tawb.temperatureK = *colourTemperature;\n> > > +\t\tupdate = true;\n> > > +\t}\n> > > \n> > > +\tif (update)\n> > > \t\tLOG(RkISP1Awb, Debug)\n> > > \t\t\t<< \"Set colour gains to \" << awb.gains.manual;\n> > > -\t}\n> > > \n> > > -\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> > > -\n> > > -\tif (!awb.autoEnabled)\n> > > -\t\tframeContext.awb.gains = awb.gains.manual;\n> > > +\tframeContext.awb.gains = awb.gains.manual;\n> > > +\tframeContext.awb.temperatureK = awb.temperatureK;\n> > > }\n> > > \n> > > /**\n> > > @@ -126,8 +154,10 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > \t * This is the latest time we can read the active state. This is the\n> > > \t * most up-to-date automatic values we can read.\n> > > \t */\n> > > -\tif (frameContext.awb.autoEnabled)\n> > > +\tif (frameContext.awb.autoEnabled) {\n> > > \t\tframeContext.awb.gains = context.activeState.awb.gains.automatic;\n> > > +\t\tframeContext.awb.temperatureK = context.activeState.awb.temperatureK;\n> > > +\t}\n> > > \n> > > \tauto gainConfig = params->block();\n> > > \tgainConfig.setEnabled(true);\n> > > @@ -206,7 +236,7 @@ void Awb::process(IPAContext &context,\n> > > \t\t\tstatic_cast(frameContext.awb.gains.r()),\n> > > \t\t\tstatic_cast(frameContext.awb.gains.b())\n> > > \t\t});\n> > > -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> > > +\tmetadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);\n> > > \n> > > \tif (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {\n> > > \t\tLOG(RkISP1Awb, Error) << \"AWB data is missing in statistics\";\n> > > @@ -281,9 +311,6 @@ void Awb::process(IPAContext &context,\n> > > \n> > > \tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> > > \n> > > -\t/* Metadata shall contain the up to date measurement */\n> > > -\tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> > > -\n> > > \t/*\n> > > \t * Estimate the red and blue gains to apply in a grey world. The green\n> > > \t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index deb8c196f1b8..4b50015beee8 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -135,6 +135,7 @@ struct IPAFrameContext : public FrameContext {\n> > > \tstruct {\n> > > \t\tRGB gains;\n> > > \t\tbool autoEnabled;\n> > > +\t\tunsigned int temperatureK;\n> > > \t} awb;\n> > > \n> > > \tstruct {","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 6EC00C32A3\n\tfor ;\n\tFri, 20 Dec 2024 10:17:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D28C668492;\n\tFri, 20 Dec 2024 11:17:08 +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 3B91362C8A\n\tfor ;\n\tFri, 20 Dec 2024 11:17:06 +0100 (CET)","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 9BCB47E2;\n\tFri, 20 Dec 2024 11:16:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KUTo5mNZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734689786;\n\tbh=bC4mC5W+dDw36zbruht6uT6EzUDB6eTS98DR1UO9RIw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KUTo5mNZrRbqeFm4STido/GxxzVuBB+ciQxaA7lCiA0XC9cA6veAoTsNdRXlsfIw5\n\ts5UMsmVJh6+vlFTNRPya0PzD8AyUpDqhN/yb1cA4XM9KodknbvV6hGl73kTUKmY77O\n\tFFwef2Dky1agRj/u9xKQuq82GsP6yuiD4mLCn4ys=","Date":"Fri, 20 Dec 2024 12:16:58 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature\n\tcontrol","Message-ID":"<20241220101658.GD8313@pendragon.ideasonboard.com>","References":"<20241219175729.293782-1-stefan.klug@ideasonboard.com>\n\t<20241219175729.293782-4-stefan.klug@ideasonboard.com>\n\t<20241220095457.GB6745@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\" "}}]