[{"id":34848,"web_url":"https://patchwork.libcamera.org/comment/34848/","msgid":"<175216347122.2538045.10881119114955882834@ping.linuxembedded.co.uk>","date":"2025-07-10T16:04:31","subject":"Re: [PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less\n\tthan 1","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nQuoting David Plowman (2025-06-17 09:29:49)\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> Signed-off-by: David Plowman \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 a5562760..a87dc194 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.gainR, awb.gainB } };\n\nThere's a merge conflict here because we've fixed the above to be RGB\ninsted of RRB.\n\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.gainR, awb.gainB } };\n\nWhich I have applied manually as a correction to this line ^\n\nbut unfortunately - [PATCH 4/7] ipa: rpi: Advance the delay context\ncounter even when IPAs don't run isn't applying at all for me.\n\nIt might be safer for me to ask you to rebase this series please? Then I\ncan push it through CI and merge.\n\n--\nRegards\n\nKieran","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 A45BBC3237\n\tfor ;\n\tThu, 10 Jul 2025 16:04:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F3D868F04;\n\tThu, 10 Jul 2025 18:04:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A34C568EE7\n\tfor ;\n\tThu, 10 Jul 2025 18:04: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 5E378B2B;\n\tThu, 10 Jul 2025 18:04:05 +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=\"cT3h7Yi9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752163445;\n\tbh=rg9UOkq4FwVJpeIAVIJSBduWecCW+8Bn8J0b27pH3fE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cT3h7Yi9mXrJW297PQWtdOHzV59TQ5aNiegzlPLJGHrW8XT5TVTXhiV3yQk8wJFD6\n\tOuu3TYMMFlut3EuvY5lWLhYZERpX+4yXaxC3CiuLuPa0NiFd5lD0XjjFPWkrEHlYvn\n\tEckxRBTVZrPZSO+jjKWu7ytFEmGrfsPcA6mc8Kwk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250617082956.5699-2-david.plowman@raspberrypi.com>","References":"<20250617082956.5699-1-david.plowman@raspberrypi.com>\n\t<20250617082956.5699-2-david.plowman@raspberrypi.com>","Subject":"Re: [PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less\n\tthan 1","From":"Kieran Bingham ","Cc":"David Plowman ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 10 Jul 2025 17:04:31 +0100","Message-ID":"<175216347122.2538045.10881119114955882834@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":34934,"web_url":"https://patchwork.libcamera.org/comment/34934/","msgid":"<175284546074.560048.2380556586489196512@ping.linuxembedded.co.uk>","date":"2025-07-18T13:31:00","subject":"Re: [PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less\n\tthan 1","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David, Naush,\n\nQuoting Kieran Bingham (2025-07-10 17:04:31)\n> Hi David,\n> \n> Quoting David Plowman (2025-06-17 09:29:49)\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> > Signed-off-by: David Plowman \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 a5562760..a87dc194 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.gainR, awb.gainB } };\n> \n> There's a merge conflict here because we've fixed the above to be RGB\n> insted of RRB.\n> \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.gainR, awb.gainB } };\n> \n> Which I have applied manually as a correction to this line ^\n> \n> but unfortunately - [PATCH 4/7] ipa: rpi: Advance the delay context\n> counter even when IPAs don't run isn't applying at all for me.\n> \n> It might be safer for me to ask you to rebase this series please? Then I\n> can push it through CI and merge.\n\nDo either of you have a rebased version of this I can push through CI?\n--\nRegards\n\nKieran","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 1ABC0C3237\n\tfor ;\n\tFri, 18 Jul 2025 13:31:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8543268FA0;\n\tFri, 18 Jul 2025 15:31:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 215646150F\n\tfor ;\n\tFri, 18 Jul 2025 15:31:04 +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 1C45259F5;\n\tFri, 18 Jul 2025 15:30:29 +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=\"Ok/3CuE5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752845429;\n\tbh=LLDCW7hseGpcLQzuhLckL7VQ6+wd29xL/SfyFZN+UUI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Ok/3CuE5nKjwvGvtqqRdXMqGlyl2J4ttJrbWT0gM+EU1YE+r9TAabKSYrm9dfpKco\n\tFsZOyNimPvV/mB+ZWz2Bm7cML9Sg7dUKzSnJEtTQTL2VKsxbuBRwFQsQ4C8sDgc++6\n\tLSFx+V5oHhi6+i81CnVpO5AsgOMjxkSo7NzaqYJI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175216347122.2538045.10881119114955882834@ping.linuxembedded.co.uk>","References":"<20250617082956.5699-1-david.plowman@raspberrypi.com>\n\t<20250617082956.5699-2-david.plowman@raspberrypi.com>\n\t<175216347122.2538045.10881119114955882834@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less\n\tthan 1","From":"Kieran Bingham ","Cc":"David Plowman ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tNaushir Patuck , ","Date":"Fri, 18 Jul 2025 14:31:00 +0100","Message-ID":"<175284546074.560048.2380556586489196512@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]