[{"id":35030,"web_url":"https://patchwork.libcamera.org/comment/35030/","msgid":"<20250723045349.GB32411@pendragon.ideasonboard.com>","date":"2025-07-23T04:53:49","subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush, David,\n\nOn Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote:\n> From: David Plowman \n> \n> Previously these were handled in the AGC/AEC exposure update\n> calculations by explicitly driving a higher digital gain to \"cancel\n> out\" any colour gains that were less than 1.\n> \n> Now we're ignoring this in the AGC and leaving it to the IPA code to\n> normalise all the gains so that the smallest is 1. We don't regard\n> this as a \"real\" increase because one of the colour channels (just not\n> necessarily the green one) still gets the minimum gain possible.\n> \n> We do, however, update the statistics calculations so that they\n> reflect any such digital gain increase, so that images are driven to\n> the correct level.\n\nI'm checking if this is something we should replicate in other IPA\nmodules. The commit message doesn't document the reason for this change.\nCould you briefly explain the rationale ?\n\n> Signed-off-by: David Plowman \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++---------------\n> src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++----\n> src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++---\n> 3 files changed, 54 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index b2999364bf27..ae0cb148893a 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n> \n> \tDuration fixedExposureTime = limitExposureTime(fixedExposureTime_);\n> \tif (fixedExposureTime && fixedAnalogueGain_) {\n> -\t\t/* We're going to reset the algorithm here with these fixed values. */\n> -\t\tfetchAwbStatus(metadata);\n> -\t\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> -\t\tASSERT(minColourGain != 0.0);\n> -\n> \t\t/* This is the equivalent of computeTargetExposure and applyDigitalGain. */\n> \t\ttarget_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_;\n> -\t\ttarget_.totalExposure = target_.totalExposureNoDG / minColourGain;\n> +\t\ttarget_.totalExposure = target_.totalExposureNoDG;\n> \n> \t\t/* Equivalent of filterExposure. This resets any \"history\". */\n> \t\tfiltered_ = target_;\n> @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n> \t\t */\n> \n> \t\tdouble ratio = lastSensitivity / cameraMode.sensitivity;\n> -\t\ttarget_.totalExposureNoDG *= ratio;\n> \t\ttarget_.totalExposure *= ratio;\n> -\t\tfiltered_.totalExposureNoDG *= ratio;\n> +\t\ttarget_.totalExposureNoDG = target_.totalExposure;\n> \t\tfiltered_.totalExposure *= ratio;\n> +\t\tfiltered_.totalExposureNoDG = filtered_.totalExposure;\n> \n> \t\tdivideUpExposure();\n> \t} else {\n> @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> \t}\n> \n> \t/* Factor in the AWB correction if needed. */\n> -\tif (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb)\n> -\t\tsum *= RGB{ { awb.gainR, awb.gainG, awb.gainB } };\n> +\tif (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {\n> +\t\tdouble minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 });\n> +\t\tminColourGain = std::max(minColourGain, 1.0);\n> +\t\tRGB colourGains{ { awb.gainR, awb.gainG, awb.gainB } };\n> +\t\tcolourGains /= minColourGain;\n> +\t\tsum *= colourGains;\n> +\t}\n> \n> \tdouble ySum = ipa::rec601LuminanceFromRGB(sum);\n> \n> @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> void AgcChannel::computeTargetExposure(double gain)\n> {\n> \tif (status_.fixedExposureTime && status_.fixedAnalogueGain) {\n> -\t\t/*\n> -\t\t * When analogue gain and exposure time are both fixed, we need\n> -\t\t * to drive the total exposure so that we end up with a digital\n> -\t\t * gain of at least 1/minColourGain. Otherwise we'd desaturate\n> -\t\t * channels causing white to go cyan or magenta.\n> -\t\t */\n> -\t\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> -\t\tASSERT(minColourGain != 0.0);\n> \t\ttarget_.totalExposure =\n> -\t\t\tstatus_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain;\n> +\t\t\tstatus_.fixedExposureTime * status_.fixedAnalogueGain;\n> \t} else {\n> \t\t/*\n> \t\t * The statistics reflect the image without digital gain, so the final\n> @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel\n> \n> bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)\n> {\n> -\tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> -\tASSERT(minColourGain != 0.0);\n> -\tdouble dg = 1.0 / minColourGain;\n> -\t/*\n> -\t * I think this pipeline subtracts black level and rescales before we\n> -\t * get the stats, so no need to worry about it.\n> -\t */\n> -\tLOG(RPiAgc, Debug) << \"after AWB, target dg \" << dg << \" gain \" << gain\n> -\t\t\t << \" target_Y \" << targetY;\n> +\tdouble dg = 1.0;\n> +\n> \t/*\n> \t * Finally, if we're trying to reduce exposure but the target_Y is\n> \t * \"close\" to 1.0, then the gain computed for that constraint will be\n> diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp\n> index bb50a9e05904..e1a804f533bb 100644\n> --- a/src/ipa/rpi/pisp/pisp.cpp\n> +++ b/src/ipa/rpi/pisp/pisp.cpp\n> @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n> \tpisp_wbg_config wbg;\n> \tpisp_fe_rgby_config rgby = {};\n> \tdouble dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0;\n> +\tdouble minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n> +\t/* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n> +\tdouble extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> \n> -\twbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10);\n> -\twbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10);\n> -\twbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10);\n> +\t/*\n> +\t * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any\n> +\t * channels (which would cause saturated pixels to go cyan or magenta).\n> +\t * Doing this doesn't really apply more gain than necessary, because one of the\n> +\t * channels is always getting the minimum gain possible. For this reason we also\n> +\t * don't change the values that we report externally.\n> +\t */\n> +\tdouble gainR = awbStatus->gainR * extraGain;\n> +\tdouble gainG = awbStatus->gainG * extraGain;\n> +\tdouble gainB = awbStatus->gainB * extraGain;\n> +\n> +\twbg.gain_r = clampField(dg * gainR, 14, 10);\n> +\twbg.gain_g = clampField(dg * gainG, 14, 10);\n> +\twbg.gain_b = clampField(dg * gainB, 14, 10);\n> \n> \t/*\n> \t * The YCbCr conversion block should contain the appropriate YCbCr\n> @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n> \tbe_->GetYcbcr(csc);\n> \n> \t/* The CSC coefficients already have the << 10 scaling applied. */\n> -\trgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14);\n> -\trgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14);\n> -\trgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14);\n> +\trgby.gain_r = clampField(csc.coeffs[0] * gainR, 14);\n> +\trgby.gain_g = clampField(csc.coeffs[1] * gainG, 14);\n> +\trgby.gain_b = clampField(csc.coeffs[2] * gainB, 14);\n> \n> \tLOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> \t\t\t << awbStatus->gainB;\n> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> index ba43e4741584..8a7a37c870ed 100644\n> --- a/src/ipa/rpi/vc4/vc4.cpp\n> +++ b/src/ipa/rpi/vc4/vc4.cpp\n> @@ -63,7 +63,8 @@ private:\n> \tbool validateIspControls();\n> \n> \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> -\tvoid applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> +\tvoid applyDG(const struct AgcPrepareStatus *dgStatus,\n> +\t\t const struct AwbStatus *awbStatus, ControlList &ctrls);\n> \tvoid applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> \tvoid applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> \tvoid applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> \t\tapplyCCM(ccmStatus, ctrls);\n> \n> \tAgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> -\tif (dgStatus)\n> -\t\tapplyDG(dgStatus, ctrls);\n> +\tapplyDG(dgStatus, awbStatus, ctrls);\n> \n> \tAlscStatus *lsStatus = rpiMetadata.getLocked(\"alsc.status\");\n> \tif (lsStatus)\n> @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \t\t static_cast(awbStatus->gainB * 1000));\n> }\n> \n> -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,\n> +\t\t const struct AwbStatus *awbStatus, ControlList &ctrls)\n> {\n> +\tdouble digitalGain = dgStatus ? dgStatus->digitalGain : 1.0;\n> +\n> +\tif (awbStatus) {\n> +\t\t/*\n> +\t\t * We must apply sufficient extra digital gain to stop any of the channel gains being\n> +\t\t * less than 1, which would cause saturation artifacts. Note that one of the colour\n> +\t\t * channels is still getting the minimum possible gain, so it's not a \"real\" gain\n> +\t\t * increase.\n> +\t\t */\n> +\t\tdouble minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n> +\t\t/* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n> +\t\tdouble extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> +\t\tdigitalGain *= extraGain;\n> +\t}\n> +\n> \tctrls.set(V4L2_CID_DIGITAL_GAIN,\n> -\t\t static_cast(dgStatus->digitalGain * 1000));\n> +\t\t static_cast(digitalGain * 1000));\n> }\n> \n> void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)","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 CC37FC3237\n\tfor ;\n\tWed, 23 Jul 2025 04:53:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E21B69045;\n\tWed, 23 Jul 2025 06:53:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0280C69025\n\tfor ;\n\tWed, 23 Jul 2025 06:53:51 +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 UTF8SMTPSA id 9F596DF3;\n\tWed, 23 Jul 2025 06:53:13 +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=\"Fw0qE+80\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753246393;\n\tbh=qO4s4OX/8Fwunf9WEXca0OmWQPCantkeoFsR/rbU5j4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Fw0qE+80OdfjptPvnKXmkSRsJtLbnbNF7WyXQvsiRJezJ39aq9Bq0q6DlePDRsc55\n\tpsrD64Dol6RjupHyJEj/1cThZYbU7M+gbQZyTvMCN9xLL7KmTuPoufeQCLYzz7z/u1\n\t5ArDYi7+tU2CcnwyMwyQoUacpEdGIQQy/B1sAE/w=","Date":"Wed, 23 Jul 2025 07:53:49 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ","Subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","Message-ID":"<20250723045349.GB32411@pendragon.ideasonboard.com>","References":"<20250721074853.1463358-1-naush@raspberrypi.com>\n\t<20250721074853.1463358-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250721074853.1463358-2-naush@raspberrypi.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":35033,"web_url":"https://patchwork.libcamera.org/comment/35033/","msgid":"","date":"2025-07-23T07:24:58","subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nWell, I think it's all a matter of personal taste really, and to be honest,\nthis was so long ago my recollections are a bit hazy.\n\nI think it's just a slight simplification of the AGC. Previously, if AWB\ndecided the red gain was 0.8, then the AGC had to see this, and compensate\nby applying a global digital gain of 1/0.8 = 1.25, so that multiplied\ntogether, the red gain is now 1. Arguably, this would mean the AGC should\npossibly target a point only 0.8x its nominal target, because that extra\ndigital gain would make up for it.\n\nIt seems I slightly changed my view on that. and decided I didn't care\nabout it. I preferred to simplify the AGC so as not to worry about that,\nand make the AWB normalise the gains to the red gain comes out as 1. So the\nimage comes out a teeny bit brighter, but it's a very marginal effect. In\npractice, the only cameras we have that suffer from all this (which is when\nthe red component goes very high in very low CT illumination like 3000K),\nare Omnivision ones, where even then the gain never goes below about 0.95 -\nso it's a 5% effect maximum.\n\nFrom my point of view, it's a take-it-or-leave-it thing, go with personal\npreference. Dunno if that helps!\n\nDavid\n\nOn Wed, 23 Jul 2025 at 05:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush, David,\n>\n> On Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote:\n> > From: David Plowman \n> >\n> > Previously these were handled in the AGC/AEC exposure update\n> > calculations by explicitly driving a higher digital gain to \"cancel\n> > out\" any colour gains that were less than 1.\n> >\n> > Now we're ignoring this in the AGC and leaving it to the IPA code to\n> > normalise all the gains so that the smallest is 1. We don't regard\n> > this as a \"real\" increase because one of the colour channels (just not\n> > necessarily the green one) still gets the minimum gain possible.\n> >\n> > We do, however, update the statistics calculations so that they\n> > reflect any such digital gain increase, so that images are driven to\n> > the correct level.\n>\n> I'm checking if this is something we should replicate in other IPA\n> modules. The commit message doesn't document the reason for this change.\n> Could you briefly explain the rationale ?\n>\n> > Signed-off-by: David Plowman \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++---------------\n> > src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++----\n> > src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++---\n> > 3 files changed, 54 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index b2999364bf27..ae0cb148893a 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const\n> &cameraMode,\n> >\n> > Duration fixedExposureTime = limitExposureTime(fixedExposureTime_);\n> > if (fixedExposureTime && fixedAnalogueGain_) {\n> > - /* We're going to reset the algorithm here with these\n> fixed values. */\n> > - fetchAwbStatus(metadata);\n> > - double minColourGain = std::min({ awb_.gainR, awb_.gainG,\n> awb_.gainB, 1.0 });\n> > - ASSERT(minColourGain != 0.0);\n> > -\n> > /* This is the equivalent of computeTargetExposure and\n> applyDigitalGain. */\n> > target_.totalExposureNoDG = fixedExposureTime_ *\n> fixedAnalogueGain_;\n> > - target_.totalExposure = target_.totalExposureNoDG /\n> minColourGain;\n> > + target_.totalExposure = target_.totalExposureNoDG;\n> >\n> > /* Equivalent of filterExposure. This resets any\n> \"history\". */\n> > filtered_ = target_;\n> > @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const\n> &cameraMode,\n> > */\n> >\n> > double ratio = lastSensitivity / cameraMode.sensitivity;\n> > - target_.totalExposureNoDG *= ratio;\n> > target_.totalExposure *= ratio;\n> > - filtered_.totalExposureNoDG *= ratio;\n> > + target_.totalExposureNoDG = target_.totalExposure;\n> > filtered_.totalExposure *= ratio;\n> > + filtered_.totalExposureNoDG = filtered_.totalExposure;\n> >\n> > divideUpExposure();\n> > } else {\n> > @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats,\n> AwbStatus const &awb,\n> > }\n> >\n> > /* Factor in the AWB correction if needed. */\n> > - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb)\n> > - sum *= RGB{ { awb.gainR, awb.gainG, awb.gainB } };\n> > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {\n> > + double minColourGain = std::min({ awb.gainR, awb.gainG,\n> awb.gainB, 1.0 });\n> > + minColourGain = std::max(minColourGain, 1.0);\n> > + RGB colourGains{ { awb.gainR, awb.gainG, awb.gainB\n> } };\n> > + colourGains /= minColourGain;\n> > + sum *= colourGains;\n> > + }\n> >\n> > double ySum = ipa::rec601LuminanceFromRGB(sum);\n> >\n> > @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr\n> &statistics, Metadata *imageMetadata,\n> > void AgcChannel::computeTargetExposure(double gain)\n> > {\n> > if (status_.fixedExposureTime && status_.fixedAnalogueGain) {\n> > - /*\n> > - * When analogue gain and exposure time are both fixed, we\n> need\n> > - * to drive the total exposure so that we end up with a\n> digital\n> > - * gain of at least 1/minColourGain. Otherwise we'd\n> desaturate\n> > - * channels causing white to go cyan or magenta.\n> > - */\n> > - double minColourGain = std::min({ awb_.gainR, awb_.gainG,\n> awb_.gainB, 1.0 });\n> > - ASSERT(minColourGain != 0.0);\n> > target_.totalExposure =\n> > - status_.fixedExposureTime *\n> status_.fixedAnalogueGain / minColourGain;\n> > + status_.fixedExposureTime *\n> status_.fixedAnalogueGain;\n> > } else {\n> > /*\n> > * The statistics reflect the image without digital gain,\n> so the final\n> > @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const\n> AgcChannelTotalExposures &channel\n> >\n> > bool AgcChannel::applyDigitalGain(double gain, double targetY, bool\n> channelBound)\n> > {\n> > - double minColourGain = std::min({ awb_.gainR, awb_.gainG,\n> awb_.gainB, 1.0 });\n> > - ASSERT(minColourGain != 0.0);\n> > - double dg = 1.0 / minColourGain;\n> > - /*\n> > - * I think this pipeline subtracts black level and rescales before\n> we\n> > - * get the stats, so no need to worry about it.\n> > - */\n> > - LOG(RPiAgc, Debug) << \"after AWB, target dg \" << dg << \" gain \" <<\n> gain\n> > - << \" target_Y \" << targetY;\n> > + double dg = 1.0;\n> > +\n> > /*\n> > * Finally, if we're trying to reduce exposure but the target_Y is\n> > * \"close\" to 1.0, then the gain computed for that constraint will\n> be\n> > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp\n> > index bb50a9e05904..e1a804f533bb 100644\n> > --- a/src/ipa/rpi/pisp/pisp.cpp\n> > +++ b/src/ipa/rpi/pisp/pisp.cpp\n> > @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus,\n> const AgcPrepareStatus *agcPr\n> > pisp_wbg_config wbg;\n> > pisp_fe_rgby_config rgby = {};\n> > double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0;\n> > + double minColourGain = std::min({ awbStatus->gainR,\n> awbStatus->gainG, awbStatus->gainB, 1.0 });\n> > + /* The 0.1 here doesn't mean much, but just stops arithmetic\n> errors and extreme behaviour. */\n> > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> >\n> > - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10);\n> > - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10);\n> > - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10);\n> > + /*\n> > + * Apply an extra gain of 1 / minColourGain so as not to apply < 1\n> gains to any\n> > + * channels (which would cause saturated pixels to go cyan or\n> magenta).\n> > + * Doing this doesn't really apply more gain than necessary,\n> because one of the\n> > + * channels is always getting the minimum gain possible. For this\n> reason we also\n> > + * don't change the values that we report externally.\n> > + */\n> > + double gainR = awbStatus->gainR * extraGain;\n> > + double gainG = awbStatus->gainG * extraGain;\n> > + double gainB = awbStatus->gainB * extraGain;\n> > +\n> > + wbg.gain_r = clampField(dg * gainR, 14, 10);\n> > + wbg.gain_g = clampField(dg * gainG, 14, 10);\n> > + wbg.gain_b = clampField(dg * gainB, 14, 10);\n> >\n> > /*\n> > * The YCbCr conversion block should contain the appropriate YCbCr\n> > @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus,\n> const AgcPrepareStatus *agcPr\n> > be_->GetYcbcr(csc);\n> >\n> > /* The CSC coefficients already have the << 10 scaling applied. */\n> > - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14);\n> > - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14);\n> > - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14);\n> > + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14);\n> > + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14);\n> > + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14);\n> >\n> > LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \"\n> B: \"\n> > << awbStatus->gainB;\n> > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> > index ba43e4741584..8a7a37c870ed 100644\n> > --- a/src/ipa/rpi/vc4/vc4.cpp\n> > +++ b/src/ipa/rpi/vc4/vc4.cpp\n> > @@ -63,7 +63,8 @@ private:\n> > bool validateIspControls();\n> >\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList\n> &ctrls);\n> > + void applyDG(const struct AgcPrepareStatus *dgStatus,\n> > + const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > void applyCCM(const struct CcmStatus *ccmStatus, ControlList\n> &ctrls);\n> > void applyBlackLevel(const struct BlackLevelStatus\n> *blackLevelStatus, ControlList &ctrls);\n> > void applyGamma(const struct ContrastStatus *contrastStatus,\n> ControlList &ctrls);\n> > @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]]\n> const PrepareParams ¶ms,\n> > applyCCM(ccmStatus, ctrls);\n> >\n> > AgcPrepareStatus *dgStatus =\n> rpiMetadata.getLocked(\"agc.prepare_status\");\n> > - if (dgStatus)\n> > - applyDG(dgStatus, ctrls);\n> > + applyDG(dgStatus, awbStatus, ctrls);\n> >\n> > AlscStatus *lsStatus =\n> rpiMetadata.getLocked(\"alsc.status\");\n> > if (lsStatus)\n> > @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gainB * 1000));\n> > }\n> >\n> > -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,\n> ControlList &ctrls)\n> > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,\n> > + const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > {\n> > + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0;\n> > +\n> > + if (awbStatus) {\n> > + /*\n> > + * We must apply sufficient extra digital gain to stop any\n> of the channel gains being\n> > + * less than 1, which would cause saturation artifacts.\n> Note that one of the colour\n> > + * channels is still getting the minimum possible gain, so\n> it's not a \"real\" gain\n> > + * increase.\n> > + */\n> > + double minColourGain = std::min({ awbStatus->gainR,\n> awbStatus->gainG, awbStatus->gainB, 1.0 });\n> > + /* The 0.1 here doesn't mean much, but just stops\n> arithmetic errors and extreme behaviour. */\n> > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> > + digitalGain *= extraGain;\n> > + }\n> > +\n> > ctrls.set(V4L2_CID_DIGITAL_GAIN,\n> > - static_cast(dgStatus->digitalGain * 1000));\n> > + static_cast(digitalGain * 1000));\n> > }\n> >\n> > void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList\n> &ctrls)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DA4F8BDCC1\n\tfor ;\n\tWed, 23 Jul 2025 07:25:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D691B69041;\n\tWed, 23 Jul 2025 09:25:14 +0200 (CEST)","from mail-qt1-x830.google.com (mail-qt1-x830.google.com\n\t[IPv6:2607:f8b0:4864:20::830])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34B7069041\n\tfor ;\n\tWed, 23 Jul 2025 09:25:12 +0200 (CEST)","by mail-qt1-x830.google.com with SMTP id\n\td75a77b69052e-4aaaf1a63c1so48266881cf.3\n\tfor ;\n\tWed, 23 Jul 2025 00:25:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"JrqfRyNP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1753255511; x=1753860311;\n\tdarn=lists.libcamera.org; \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=TjGIJSF9PgySjkfg51xZo6N0Ku6Z82g39d3yL3vDvKA=;\n\tb=JrqfRyNPI4KdkRRQjinqBXEezMb7NaG65FVx2X10XJYj/kXMGHnzFbHk/a5DA6a/zp\n\t2CBB2KXYSmIzrdrnZVpuX07sz+5JkJGKXuOAWDGZtk9S1AIS0E66aC6H2jUsaPOGW1C0\n\tUYmhcdDkZVrSsAJUje/gB3mb9EeBMi1xU1dLAL8pptt5K1JV2uNYD2He09y2qtsAaOJ1\n\tUpX46XUVb/gJdSp4bTwRo1NxSO9l1yD6QzhOKzuEpyRAiAWSc4g7W7pP1qBUsEGOsllt\n\tScr0osGxJO5fh5ex82Aa/+dNScWwd+HVTkk4MeOJNAnOQSnD3YtYbQ8+gG9YYmFXPVV2\n\tpZxQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1753255511; x=1753860311;\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=TjGIJSF9PgySjkfg51xZo6N0Ku6Z82g39d3yL3vDvKA=;\n\tb=H3D/yY+IYO9vtEMgPmIU6thtFmBt1sGawR9BYhUzvaPhXAi282aqShgPA1ORZwlqoE\n\tHORa5Ea/quvpKu96A3C831kIyJknPC5pe6dK7oKNqrb809xRkkoQKf1ErNIRf10hcf9l\n\tCmJKxcBanJZMnuyN4cmVQGn1YnU+83wghLgfjAWdpV7R6EY6/TDg/7omwBIX6UV9RyJm\n\tgygnapNotlAfnuWCG9RE6nASp/fHcbLzhIujf44tP0FeLpBkNAOaeswnm9hnHrmKOwGk\n\tWAQ2csTa/btzCKnmLvKpPZQUK68u36c521s0JF8OK4gAwMM46lv0z8YmhznW6+P4CXJd\n\tPIQw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW1zP5x3KHvVBmkhatB5ZEh4Acn6H3YVrFaAbTScjT1RvnY1y2HpAnbCXvo3pP3/UNgJvilfxf73f6nsns5Vwc=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzHv0uU5iYg1kRy9H5NlHUJT+/96vA6qOCPOC2ZaeqZCCuK/m+8\n\tWF6GUohhh+OAaUmN/2AUTA4LSAyGAWCf0Zyym8KYsZo0oSRPgciKIrV8CDmLhCmmOjI3a/6FaYI\n\tzGaVQF1HZbQ7GGnf3lYYOTc6VZUY0pSq6AfaM6H9nWSUE+DBh/MIVbW8=","X-Gm-Gg":"ASbGncvvlmMcvxzhB1/rXhO7ZN/Esoui6ubppMl4tQLHG9z7nX9oGCuQDtK11dcOoPz\n\tEqagr1k0d4a0ThlI0EfxIuzl2CoE5JwoEESJJsaT4UM8f4sLs+FWl/qHLv3xhhV0hbn/K/yq38Z\n\tFWhl6LBx5IIJkrDUV5GkhytuLDuhVpyJz2/TrDxTuMf8Mabu0t/GcgEQvzH8xd1dDrRyluZ3gzm\n\tlomxApYOvZYrHb0yB78fO56cnAKvBSCq16fxLs=","X-Google-Smtp-Source":"AGHT+IE9xcpqWlKI3zP3/Sla8x39jhpDN/qaXGdL3WD72Dp2SpLLsXa8J2QTaPMaALWTjClMyGN2n/TGzAl3YWidj9k=","X-Received":"by 2002:ac8:7d84:0:b0:4ab:95a7:71d7 with SMTP id\n\td75a77b69052e-4ae6dff1adcmr22136441cf.53.1753255510641;\n\tWed, 23 Jul 2025 00:25:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20250721074853.1463358-1-naush@raspberrypi.com>\n\t<20250721074853.1463358-2-naush@raspberrypi.com>\n\t<20250723045349.GB32411@pendragon.ideasonboard.com>","In-Reply-To":"<20250723045349.GB32411@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Wed, 23 Jul 2025 08:24:58 +0100","X-Gm-Features":"Ac12FXxK7CYRLCj21Bm1H03HCh1mL2J7e_nK7tZ5KcjrwDYTgZ2vAd1lUJ2qJ48","Message-ID":"","Subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","To":"Laurent Pinchart ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000e9b069063a939ba5\"","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":35062,"web_url":"https://patchwork.libcamera.org/comment/35062/","msgid":"<20250723193248.GD11202@pendragon.ideasonboard.com>","date":"2025-07-23T19:32:48","subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Jul 23, 2025 at 08:24:58AM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Well, I think it's all a matter of personal taste really, and to be honest,\n> this was so long ago my recollections are a bit hazy.\n> \n> I think it's just a slight simplification of the AGC. Previously, if AWB\n> decided the red gain was 0.8, then the AGC had to see this, and compensate\n> by applying a global digital gain of 1/0.8 = 1.25, so that multiplied\n> together, the red gain is now 1. Arguably, this would mean the AGC should\n> possibly target a point only 0.8x its nominal target, because that extra\n> digital gain would make up for it.\n> \n> It seems I slightly changed my view on that. and decided I didn't care\n> about it. I preferred to simplify the AGC so as not to worry about that,\n> and make the AWB normalise the gains to the red gain comes out as 1. So the\n> image comes out a teeny bit brighter, but it's a very marginal effect. In\n> practice, the only cameras we have that suffer from all this (which is when\n> the red component goes very high in very low CT illumination like 3000K),\n> are Omnivision ones, where even then the gain never goes below about 0.95 -\n> so it's a 5% effect maximum.\n> \n> From my point of view, it's a take-it-or-leave-it thing, go with personal\n> preference. Dunno if that helps!\n\nIt does, thanks.\n\n> On Wed, 23 Jul 2025 at 05:53, Laurent Pinchart wrote:\n> > On Mon, Jul 21, 2025 at 08:47:21AM +0100, Naushir Patuck wrote:\n> > > From: David Plowman \n> > >\n> > > Previously these were handled in the AGC/AEC exposure update\n> > > calculations by explicitly driving a higher digital gain to \"cancel\n> > > out\" any colour gains that were less than 1.\n> > >\n> > > Now we're ignoring this in the AGC and leaving it to the IPA code to\n> > > normalise all the gains so that the smallest is 1. We don't regard\n> > > this as a \"real\" increase because one of the colour channels (just not\n> > > necessarily the green one) still gets the minimum gain possible.\n> > >\n> > > We do, however, update the statistics calculations so that they\n> > > reflect any such digital gain increase, so that images are driven to\n> > > the correct level.\n> >\n> > I'm checking if this is something we should replicate in other IPA\n> > modules. The commit message doesn't document the reason for this change.\n> > Could you briefly explain the rationale ?\n> >\n> > > Signed-off-by: David Plowman \n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Naushir Patuck \n> > > ---\n> > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++---------------\n> > > src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++----\n> > > src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++---\n> > > 3 files changed, 54 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > index b2999364bf27..ae0cb148893a 100644\n> > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n> > >\n> > > Duration fixedExposureTime = limitExposureTime(fixedExposureTime_);\n> > > if (fixedExposureTime && fixedAnalogueGain_) {\n> > > - /* We're going to reset the algorithm here with these fixed values. */\n> > > - fetchAwbStatus(metadata);\n> > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> > > - ASSERT(minColourGain != 0.0);\n> > > -\n> > > /* This is the equivalent of computeTargetExposure and applyDigitalGain. */\n> > > target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_;\n> > > - target_.totalExposure = target_.totalExposureNoDG / minColourGain;\n> > > + target_.totalExposure = target_.totalExposureNoDG;\n> > >\n> > > /* Equivalent of filterExposure. This resets any \"history\". */\n> > > filtered_ = target_;\n> > > @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode,\n> > > */\n> > >\n> > > double ratio = lastSensitivity / cameraMode.sensitivity;\n> > > - target_.totalExposureNoDG *= ratio;\n> > > target_.totalExposure *= ratio;\n> > > - filtered_.totalExposureNoDG *= ratio;\n> > > + target_.totalExposureNoDG = target_.totalExposure;\n> > > filtered_.totalExposure *= ratio;\n> > > + filtered_.totalExposureNoDG = filtered_.totalExposure;\n> > >\n> > > divideUpExposure();\n> > > } else {\n> > > @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > }\n> > >\n> > > /* Factor in the AWB correction if needed. */\n> > > - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb)\n> > > - sum *= RGB{ { awb.gainR, awb.gainG, awb.gainB } };\n> > > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {\n> > > + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 });\n> > > + minColourGain = std::max(minColourGain, 1.0);\n> > > + RGB colourGains{ { awb.gainR, awb.gainG, awb.gainB } };\n> > > + colourGains /= minColourGain;\n> > > + sum *= colourGains;\n> > > + }\n> > >\n> > > double ySum = ipa::rec601LuminanceFromRGB(sum);\n> > >\n> > > @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> > > void AgcChannel::computeTargetExposure(double gain)\n> > > {\n> > > if (status_.fixedExposureTime && status_.fixedAnalogueGain) {\n> > > - /*\n> > > - * When analogue gain and exposure time are both fixed, we need\n> > > - * to drive the total exposure so that we end up with a digital\n> > > - * gain of at least 1/minColourGain. Otherwise we'd desaturate\n> > > - * channels causing white to go cyan or magenta.\n> > > - */\n> > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> > > - ASSERT(minColourGain != 0.0);\n> > > target_.totalExposure =\n> > > - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain;\n> > > + status_.fixedExposureTime * status_.fixedAnalogueGain;\n> > > } else {\n> > > /*\n> > > * The statistics reflect the image without digital gain, so the final\n> > > @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel\n> > >\n> > > bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)\n> > > {\n> > > - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> > > - ASSERT(minColourGain != 0.0);\n> > > - double dg = 1.0 / minColourGain;\n> > > - /*\n> > > - * I think this pipeline subtracts black level and rescales before we\n> > > - * get the stats, so no need to worry about it.\n> > > - */\n> > > - LOG(RPiAgc, Debug) << \"after AWB, target dg \" << dg << \" gain \" << gain\n> > > - << \" target_Y \" << targetY;\n> > > + double dg = 1.0;\n> > > +\n> > > /*\n> > > * Finally, if we're trying to reduce exposure but the target_Y is\n> > > * \"close\" to 1.0, then the gain computed for that constraint will be\n> > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp\n> > > index bb50a9e05904..e1a804f533bb 100644\n> > > --- a/src/ipa/rpi/pisp/pisp.cpp\n> > > +++ b/src/ipa/rpi/pisp/pisp.cpp\n> > > @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n> > > pisp_wbg_config wbg;\n> > > pisp_fe_rgby_config rgby = {};\n> > > double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0;\n> > > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n> > > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n> > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> > >\n> > > - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10);\n> > > - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10);\n> > > - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10);\n> > > + /*\n> > > + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any\n> > > + * channels (which would cause saturated pixels to go cyan or magenta).\n> > > + * Doing this doesn't really apply more gain than necessary, because one of the\n> > > + * channels is always getting the minimum gain possible. For this reason we also\n> > > + * don't change the values that we report externally.\n> > > + */\n> > > + double gainR = awbStatus->gainR * extraGain;\n> > > + double gainG = awbStatus->gainG * extraGain;\n> > > + double gainB = awbStatus->gainB * extraGain;\n> > > +\n> > > + wbg.gain_r = clampField(dg * gainR, 14, 10);\n> > > + wbg.gain_g = clampField(dg * gainG, 14, 10);\n> > > + wbg.gain_b = clampField(dg * gainB, 14, 10);\n> > >\n> > > /*\n> > > * The YCbCr conversion block should contain the appropriate YCbCr\n> > > @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr\n> > > be_->GetYcbcr(csc);\n> > >\n> > > /* The CSC coefficients already have the << 10 scaling applied. */\n> > > - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14);\n> > > - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14);\n> > > - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14);\n> > > + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14);\n> > > + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14);\n> > > + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14);\n> > >\n> > > LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> > > << awbStatus->gainB;\n> > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> > > index ba43e4741584..8a7a37c870ed 100644\n> > > --- a/src/ipa/rpi/vc4/vc4.cpp\n> > > +++ b/src/ipa/rpi/vc4/vc4.cpp\n> > > @@ -63,7 +63,8 @@ private:\n> > > bool validateIspControls();\n> > >\n> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > > - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> > > + void applyDG(const struct AgcPrepareStatus *dgStatus,\n> > > + const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > > void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> > > void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> > > void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> > > @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> > > applyCCM(ccmStatus, ctrls);\n> > >\n> > > AgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> > > - if (dgStatus)\n> > > - applyDG(dgStatus, ctrls);\n> > > + applyDG(dgStatus, awbStatus, ctrls);\n> > >\n> > > AlscStatus *lsStatus = rpiMetadata.getLocked(\"alsc.status\");\n> > > if (lsStatus)\n> > > @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > static_cast(awbStatus->gainB * 1000));\n> > > }\n> > >\n> > > -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> > > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,\n> > > + const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > {\n> > > + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0;\n> > > +\n> > > + if (awbStatus) {\n> > > + /*\n> > > + * We must apply sufficient extra digital gain to stop any of the channel gains being\n> > > + * less than 1, which would cause saturation artifacts. Note that one of the colour\n> > > + * channels is still getting the minimum possible gain, so it's not a \"real\" gain\n> > > + * increase.\n> > > + */\n> > > + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });\n> > > + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */\n> > > + double extraGain = 1.0 / std::max({ minColourGain, 0.1 });\n> > > + digitalGain *= extraGain;\n> > > + }\n> > > +\n> > > ctrls.set(V4L2_CID_DIGITAL_GAIN,\n> > > - static_cast(dgStatus->digitalGain * 1000));\n> > > + static_cast(digitalGain * 1000));\n> > > }\n> > >\n> > > void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)","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 A673FC3237\n\tfor ;\n\tWed, 23 Jul 2025 19:32:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B8F06906B;\n\tWed, 23 Jul 2025 21:32:53 +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 E22D0614ED\n\tfor ;\n\tWed, 23 Jul 2025 21:32:51 +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 UTF8SMTPSA id 09428EBA;\n\tWed, 23 Jul 2025 21:32:12 +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=\"l2m//j7v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753299133;\n\tbh=LjAOrYVDziMI6kW618Gq7W2cTCdWa0KDbS0fnWA+TLE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l2m//j7v16JfEqbpxbRyY5oQxvdeUs8uVFWwRcdYeiZVVRNMlKJcq6Q77MIbeojeZ\n\tyurgwj9fbAU3IwOGIaGRqfAlW4v/POBbcB2imeiE1sg5GcZbDPp8InFqNgRBqm37BS\n\tPl+J0ZDBgaT7/9tf4JxhjtxbXtoT8s67/i53g93k=","Date":"Wed, 23 Jul 2025 22:32:48 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/7] ipa: rpi: agc: Change handling of colour gains\n\tless than 1","Message-ID":"<20250723193248.GD11202@pendragon.ideasonboard.com>","References":"<20250721074853.1463358-1-naush@raspberrypi.com>\n\t<20250721074853.1463358-2-naush@raspberrypi.com>\n\t<20250723045349.GB32411@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\" "}}]