[{"id":35344,"web_url":"https://patchwork.libcamera.org/comment/35344/","msgid":"<175492341916.1641235.3291045781310871366@ping.linuxembedded.co.uk>","date":"2025-08-11T14:43:39","subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-08 15:12:47)\n> There are several occations where quantization can lead to visible\n> effects.\n> \n> In WDR mode it can happen that exposure times get set to very low values\n> (Sometimes 2-3 lines). This intentionally introduced underexposure is\n> corrected by the GWDR module. As exposure time is quantized by lines,\n> the smallest possible change in exposure time now results in a quite\n> visible change in perceived brightness.\n> \n> Sometimes the possible gain steps are also quite large leading to\n> visible jumps if e.g. if the exposure time is fixed.\n> \n> Mitigate that by applying a global gain to account for the error\n> introduced by the exposure quantization.\n> \n> ToDo: This needs perfect frame synchronous control of the sensor to work\n> properly which is not guaranteed in all cases. It still improves the\n> behavior with the current regulation and can easily be skipped, when\n> there is no compress algorithm.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++---\n> src/ipa/rkisp1/ipa_context.h | 2 ++\n> 2 files changed, 23 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 0a29326841fb..d34c041f9fe1 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> \n> + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> + context.camHelper.get());\n\nIt probably doesn't matter at the moment, but could/should the camHelper\nbe initialiased at 'init' time ? It shouldn't ever change during\nconfigure() right ? It's only the lineDuration that could change at\nconfigure time.\n\n> +\n\nThere's /so/ much (potential) duplication of code here in\nAgc::{init,configure,queueRequest} - that I have started to try to work\nout how to defer most of the work to the\nAgcMeanLumince::{init,configure,queueRequest} too. (See\nhttps://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my\nongoing development branch of that)\n\nBut I don't think I should block this work on the work I'm doing.\n\nI think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA\nAEGC and AWB implementations aligned for features and capabilities and\ncontrols.\n\nI think it should be possible that only the conversion of statistics and\nconversion will occur in the platform specific IPA for those platforms. \n\n> setLimits(context.configuration.sensor.minExposureTime,\n> context.configuration.sensor.maxExposureTime,\n> context.configuration.sensor.minAnalogueGain,\n> @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context,\n> if (!frameContext.agc.autoGainEnabled)\n> frameContext.agc.gain = agc.manual.gain;\n> \n> + if (!frameContext.agc.autoExposureEnabled &&\n> + !frameContext.agc.autoGainEnabled)\n> + frameContext.agc.quantizationGain = 1.0;\n> +\n> const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> if (meteringMode) {\n> frameContext.agc.updateMetering = agc.meteringMode != *meteringMode;\n> @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> {\n> uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> double activeAutoGain = context.activeState.agc.automatic.gain;\n> + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain;\n> \n> /* Populate exposure and gain in auto mode */\n> - if (frameContext.agc.autoExposureEnabled)\n> + if (frameContext.agc.autoExposureEnabled) {\n> frameContext.agc.exposure = activeAutoExposure;\n> - if (frameContext.agc.autoGainEnabled)\n> + frameContext.agc.quantizationGain = activeAutoQGain;\n> + }\n> + if (frameContext.agc.autoGainEnabled) {\n> frameContext.agc.gain = activeAutoGain;\n> + frameContext.agc.quantizationGain = activeAutoQGain;\n> + }\n> \n> /*\n> * Populate manual exposure and gain from the active auto values when\n> @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {\n> context.activeState.agc.manual.gain = activeAutoGain;\n> frameContext.agc.gain = activeAutoGain;\n> + frameContext.agc.quantizationGain = activeAutoQGain;\n> + }\n> +\n> + if (context.activeState.compress.supported) {\n> + frameContext.compress.enable = true;\n> + frameContext.compress.gain = frameContext.agc.quantizationGain;\n> }\n> \n> if (frame > 0 && !frameContext.agc.updateMetering)\n> @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> /* Update the estimated exposure and gain. */\n> activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> activeState.agc.automatic.gain = aGain;\n> -\n> + activeState.agc.automatic.quantizationGain = qGain;\n> /*\n> * Expand the target frame duration so that we do not run faster than\n> * the minimum frame duration when we have short exposures.\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 37a74215ce19..362ab2fda5fe 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -77,6 +77,7 @@ struct IPAActiveState {\n> struct {\n> uint32_t exposure;\n> double gain;\n> + double quantizationGain;\n> } automatic;\n> \n> bool autoExposureEnabled;\n> @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext {\n> uint32_t exposure;\n> double gain;\n> double exposureValue;\n> + double quantizationGain;\n> uint32_t vblank;\n> bool autoExposureEnabled;\n> bool autoGainEnabled;\n> -- \n> 2.48.1\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 898FCBEFBE\n\tfor ;\n\tMon, 11 Aug 2025 14:43:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD57769236;\n\tMon, 11 Aug 2025 16:43:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 356B369233\n\tfor ;\n\tMon, 11 Aug 2025 16:43:42 +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 0A7A6446;\n\tMon, 11 Aug 2025 16:42:50 +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=\"hBc2nlXn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754923370;\n\tbh=ZEe37xWgL1c0VJ8AzFb2Qt8clqRyupazksUsFLVYpLc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hBc2nlXn+g5wmKvk1iKaUcNUOlmboTjQ5apdIXRvBx5kttn5mrVDIuWtzWMsb6ek0\n\toewVM5S0M8W0pZVqWH4z/prgDWbsLkVP5htL9SXtAWVCYLoXS2qhP+ZKhRgSrUBjhs\n\tP6S/XcQ4IoIzz+i8Lx0c9g4SFL0tAVRiVnHIuLY8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250808141315.413839-10-stefan.klug@ideasonboard.com>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-10-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 11 Aug 2025 15:43:39 +0100","Message-ID":"<175492341916.1641235.3291045781310871366@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":35367,"web_url":"https://patchwork.libcamera.org/comment/35367/","msgid":"<175507719834.1225443.5506367591697261085@localhost>","date":"2025-08-13T09:26:38","subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nQuoting Kieran Bingham (2025-08-11 16:43:39)\n> Quoting Stefan Klug (2025-08-08 15:12:47)\n> > There are several occations where quantization can lead to visible\n> > effects.\n> > \n> > In WDR mode it can happen that exposure times get set to very low values\n> > (Sometimes 2-3 lines). This intentionally introduced underexposure is\n> > corrected by the GWDR module. As exposure time is quantized by lines,\n> > the smallest possible change in exposure time now results in a quite\n> > visible change in perceived brightness.\n> > \n> > Sometimes the possible gain steps are also quite large leading to\n> > visible jumps if e.g. if the exposure time is fixed.\n> > \n> > Mitigate that by applying a global gain to account for the error\n> > introduced by the exposure quantization.\n> > \n> > ToDo: This needs perfect frame synchronous control of the sensor to work\n> > properly which is not guaranteed in all cases. It still improves the\n> > behavior with the current regulation and can easily be skipped, when\n> > there is no compress algorithm.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++---\n> > src/ipa/rkisp1/ipa_context.h | 2 ++\n> > 2 files changed, 23 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 0a29326841fb..d34c041f9fe1 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> > \n> > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > + context.camHelper.get());\n> \n> It probably doesn't matter at the moment, but could/should the camHelper\n> be initialiased at 'init' time ? It shouldn't ever change during\n> configure() right ? It's only the lineDuration that could change at\n> configure time.\n\nYes, you're right that would be possible, but it also means another\nfunction and a wider interface. I'm undecided...\n\n> \n> > +\n> \n> There's /so/ much (potential) duplication of code here in\n> Agc::{init,configure,queueRequest} - that I have started to try to work\n> out how to defer most of the work to the\n> AgcMeanLumince::{init,configure,queueRequest} too. (See\n> https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my\n> ongoing development branch of that)\n> \n> But I don't think I should block this work on the work I'm doing.\n> \n> I think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA\n> AEGC and AWB implementations aligned for features and capabilities and\n> controls.\n> \n> I think it should be possible that only the conversion of statistics and\n> conversion will occur in the platform specific IPA for those platforms. \n\nYes, I think that development is sensible. It is maybe a bit of a\nphilosophical question: Does libipa provide basic building blocks and\nevery IPA creates it's own \"camera\" or is there a roughly defined libipa\n\"camera\" and the IPA contains only glue code.\n\nWe are now moving into the second direction which I think makes sense.\nlibipa can still serve as the basic block provider in case anyone wants\nto implement a completely different \"camera\" (which I doubt will\nhapppen).\n\nIf possible I'd like to still keep those topics separate to not pull in\ntoo many architectural changes into this series.\n\nBest regards,\nStefan\n\n> \n> > setLimits(context.configuration.sensor.minExposureTime,\n> > context.configuration.sensor.maxExposureTime,\n> > context.configuration.sensor.minAnalogueGain,\n> > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context,\n> > if (!frameContext.agc.autoGainEnabled)\n> > frameContext.agc.gain = agc.manual.gain;\n> > \n> > + if (!frameContext.agc.autoExposureEnabled &&\n> > + !frameContext.agc.autoGainEnabled)\n> > + frameContext.agc.quantizationGain = 1.0;\n> > +\n> > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > if (meteringMode) {\n> > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode;\n> > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > {\n> > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain;\n> > \n> > /* Populate exposure and gain in auto mode */\n> > - if (frameContext.agc.autoExposureEnabled)\n> > + if (frameContext.agc.autoExposureEnabled) {\n> > frameContext.agc.exposure = activeAutoExposure;\n> > - if (frameContext.agc.autoGainEnabled)\n> > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > + }\n> > + if (frameContext.agc.autoGainEnabled) {\n> > frameContext.agc.gain = activeAutoGain;\n> > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > + }\n> > \n> > /*\n> > * Populate manual exposure and gain from the active auto values when\n> > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {\n> > context.activeState.agc.manual.gain = activeAutoGain;\n> > frameContext.agc.gain = activeAutoGain;\n> > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > + }\n> > +\n> > + if (context.activeState.compress.supported) {\n> > + frameContext.compress.enable = true;\n> > + frameContext.compress.gain = frameContext.agc.quantizationGain;\n> > }\n> > \n> > if (frame > 0 && !frameContext.agc.updateMetering)\n> > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > /* Update the estimated exposure and gain. */\n> > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > activeState.agc.automatic.gain = aGain;\n> > -\n> > + activeState.agc.automatic.quantizationGain = qGain;\n> > /*\n> > * Expand the target frame duration so that we do not run faster than\n> > * the minimum frame duration when we have short exposures.\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 37a74215ce19..362ab2fda5fe 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -77,6 +77,7 @@ struct IPAActiveState {\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > + double quantizationGain;\n> > } automatic;\n> > \n> > bool autoExposureEnabled;\n> > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext {\n> > uint32_t exposure;\n> > double gain;\n> > double exposureValue;\n> > + double quantizationGain;\n> > uint32_t vblank;\n> > bool autoExposureEnabled;\n> > bool autoGainEnabled;\n> > -- \n> > 2.48.1\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 2FC5ABDCC1\n\tfor ;\n\tWed, 13 Aug 2025 09:26:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F35A69249;\n\tWed, 13 Aug 2025 11:26:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C6D361443\n\tfor ;\n\tWed, 13 Aug 2025 11:26:42 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:2c15:6671:9c33:a3cb])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 6E1F3229;\n\tWed, 13 Aug 2025 11:25:48 +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=\"ikmK2aSZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755077148;\n\tbh=/XnIZ9+qNtUpyvqMxpQgZgyRVSQbDCQ2qUNh544d8bY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ikmK2aSZ3kT2qHpuPP8BHDtRh56Ff5ui5JRckXRv5QNY/y0b6DwrmTzscpECI3vPq\n\t/cy7QUMCbgep/sAAlw0Btgp7fiIbRIKLbSRyztKMCMKn7XW95MsNW8jrCvd77uFmV1\n\tUYWYjZ21WTuyCLtRY943aPrYu/j8+pg4HWlMVoAM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175492341916.1641235.3291045781310871366@ping.linuxembedded.co.uk>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-10-stefan.klug@ideasonboard.com>\n\t<175492341916.1641235.3291045781310871366@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","From":"Stefan Klug ","Cc":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 11:26:38 +0200","Message-ID":"<175507719834.1225443.5506367591697261085@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":35368,"web_url":"https://patchwork.libcamera.org/comment/35368/","msgid":"<175507883096.1222897.8967861226341817544@ping.linuxembedded.co.uk>","date":"2025-08-13T09:53:50","subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-13 10:26:38)\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> Quoting Kieran Bingham (2025-08-11 16:43:39)\n> > Quoting Stefan Klug (2025-08-08 15:12:47)\n> > > There are several occations where quantization can lead to visible\n> > > effects.\n> > > \n> > > In WDR mode it can happen that exposure times get set to very low values\n> > > (Sometimes 2-3 lines). This intentionally introduced underexposure is\n> > > corrected by the GWDR module. As exposure time is quantized by lines,\n> > > the smallest possible change in exposure time now results in a quite\n> > > visible change in perceived brightness.\n> > > \n> > > Sometimes the possible gain steps are also quite large leading to\n> > > visible jumps if e.g. if the exposure time is fixed.\n> > > \n> > > Mitigate that by applying a global gain to account for the error\n> > > introduced by the exposure quantization.\n> > > \n> > > ToDo: This needs perfect frame synchronous control of the sensor to work\n> > > properly which is not guaranteed in all cases. It still improves the\n> > > behavior with the current regulation and can easily be skipped, when\n> > > there is no compress algorithm.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++---\n> > > src/ipa/rkisp1/ipa_context.h | 2 ++\n> > > 2 files changed, 23 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 0a29326841fb..d34c041f9fe1 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> > > \n> > > + AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > > + context.camHelper.get());\n> > \n> > It probably doesn't matter at the moment, but could/should the camHelper\n> > be initialiased at 'init' time ? It shouldn't ever change during\n> > configure() right ? It's only the lineDuration that could change at\n> > configure time.\n> \n> Yes, you're right that would be possible, but it also means another\n> function and a wider interface. I'm undecided...\n\nI think the common controls that AgcMeanLumince supports/creates should\nalready move to the corresponding AgcMeanLuminance::init() anyway so I\ndon't see this as a wider interface...\n\nhttps://git.uk.ideasonboard.com/kbingham/libcamera/src/commit/829f07e27b707a1696873c3d9109ff17c8dd0ff7/src/ipa/libipa/agc_mean_luminance.cpp#L396\n\n\nBut it doesn't have to be part of this series...\n\n\n> > > +\n> > \n> > There's /so/ much (potential) duplication of code here in\n> > Agc::{init,configure,queueRequest} - that I have started to try to work\n> > out how to defer most of the work to the\n> > AgcMeanLumince::{init,configure,queueRequest} too. (See\n> > https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18 for my\n> > ongoing development branch of that)\n> > \n> > But I don't think I should block this work on the work I'm doing.\n> > \n> > I think we need to do better to keep IPU3,RKISP1,Mali-C55,SoftIPA\n> > AEGC and AWB implementations aligned for features and capabilities and\n> > controls.\n> > \n> > I think it should be possible that only the conversion of statistics and\n> > conversion will occur in the platform specific IPA for those platforms. \n> \n> Yes, I think that development is sensible. It is maybe a bit of a\n> philosophical question: Does libipa provide basic building blocks and\n> every IPA creates it's own \"camera\" or is there a roughly defined libipa\n> \"camera\" and the IPA contains only glue code.\n> \n> We are now moving into the second direction which I think makes sense.\n> libipa can still serve as the basic block provider in case anyone wants\n> to implement a completely different \"camera\" (which I doubt will\n> happpen).\n> \n> If possible I'd like to still keep those topics separate to not pull in\n> too many architectural changes into this series.\n> \n\nAbsolutely - I don't want to block this series on a huge refactor.\n\nI think libipa can serve as helpers and reference implementations alike.\n\nHaving a full featured 'agc_mean_luminace' doesn't prevent someone\nmaking an IPA use helpers and implement things differently - my goal is\njust to reduce the burden on 'us' re-implementing the same thing 5 times\nin different places :D (and make it easy for new platforms to just\n'slot' in an algorithm when desired).\n\nI'll leave it to you to decide for this series:\n\nReviewed-by: Kieran Bingham \n\n> Best regards,\n> Stefan\n> \n> > \n> > > setLimits(context.configuration.sensor.minExposureTime,\n> > > context.configuration.sensor.maxExposureTime,\n> > > context.configuration.sensor.minAnalogueGain,\n> > > @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > if (!frameContext.agc.autoGainEnabled)\n> > > frameContext.agc.gain = agc.manual.gain;\n> > > \n> > > + if (!frameContext.agc.autoExposureEnabled &&\n> > > + !frameContext.agc.autoGainEnabled)\n> > > + frameContext.agc.quantizationGain = 1.0;\n> > > +\n> > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > if (meteringMode) {\n> > > frameContext.agc.updateMetering = agc.meteringMode != *meteringMode;\n> > > @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > {\n> > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > + double activeAutoQGain = context.activeState.agc.automatic.quantizationGain;\n> > > \n> > > /* Populate exposure and gain in auto mode */\n> > > - if (frameContext.agc.autoExposureEnabled)\n> > > + if (frameContext.agc.autoExposureEnabled) {\n> > > frameContext.agc.exposure = activeAutoExposure;\n> > > - if (frameContext.agc.autoGainEnabled)\n> > > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > > + }\n> > > + if (frameContext.agc.autoGainEnabled) {\n> > > frameContext.agc.gain = activeAutoGain;\n> > > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > > + }\n> > > \n> > > /*\n> > > * Populate manual exposure and gain from the active auto values when\n> > > @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {\n> > > context.activeState.agc.manual.gain = activeAutoGain;\n> > > frameContext.agc.gain = activeAutoGain;\n> > > + frameContext.agc.quantizationGain = activeAutoQGain;\n> > > + }\n> > > +\n> > > + if (context.activeState.compress.supported) {\n> > > + frameContext.compress.enable = true;\n> > > + frameContext.compress.gain = frameContext.agc.quantizationGain;\n> > > }\n> > > \n> > > if (frame > 0 && !frameContext.agc.updateMetering)\n> > > @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > /* Update the estimated exposure and gain. */\n> > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > activeState.agc.automatic.gain = aGain;\n> > > -\n> > > + activeState.agc.automatic.quantizationGain = qGain;\n> > > /*\n> > > * Expand the target frame duration so that we do not run faster than\n> > > * the minimum frame duration when we have short exposures.\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 37a74215ce19..362ab2fda5fe 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -77,6 +77,7 @@ struct IPAActiveState {\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double quantizationGain;\n> > > } automatic;\n> > > \n> > > bool autoExposureEnabled;\n> > > @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext {\n> > > uint32_t exposure;\n> > > double gain;\n> > > double exposureValue;\n> > > + double quantizationGain;\n> > > uint32_t vblank;\n> > > bool autoExposureEnabled;\n> > > bool autoGainEnabled;\n> > > -- \n> > > 2.48.1\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 BC07ABEFBE\n\tfor ;\n\tWed, 13 Aug 2025 09:53:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF3B961460;\n\tWed, 13 Aug 2025 11:53:55 +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 7F1C361443\n\tfor ;\n\tWed, 13 Aug 2025 11:53:53 +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 0BBE3129;\n\tWed, 13 Aug 2025 11:53:00 +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=\"kmu2ls+Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755078780;\n\tbh=/tJTNHoLvi5LDR6eyzcSwP9OYbjAC0cMHOLPjlTNCLM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=kmu2ls+YfLSnod4Yp6aSSwdmrSQy8zvoBvYe6eu7Vh7hVyQdo467zRBEApuHD0ilg\n\t84tuFQxMXzTF5UWoPAyDlG75lw3IyipVqKRCEZWbf0dfd3HI/0b6IC+4USKukiej0R\n\tiRrCTdxWbGWQa/buFhoPdmDmBoyn3vTXkuEDMuvs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175507719834.1225443.5506367591697261085@localhost>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-10-stefan.klug@ideasonboard.com>\n\t<175492341916.1641235.3291045781310871366@ping.linuxembedded.co.uk>\n\t<175507719834.1225443.5506367591697261085@localhost>","Subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","From":"Kieran Bingham ","Cc":"","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 10:53:50 +0100","Message-ID":"<175507883096.1222897.8967861226341817544@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":35406,"web_url":"https://patchwork.libcamera.org/comment/35406/","msgid":"<7788cc88-bfb1-4cdd-9a52-cc347ba66e29@ideasonboard.com>","date":"2025-08-14T12:12:07","subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 08/08/2025 15:12, Stefan Klug wrote:\n> There are several occations where quantization can lead to visible\n> effects.\n> \n> In WDR mode it can happen that exposure times get set to very low values\n> (Sometimes 2-3 lines). This intentionally introduced underexposure is\n> corrected by the GWDR module. As exposure time is quantized by lines,\n> the smallest possible change in exposure time now results in a quite\n> visible change in perceived brightness.\n> \n> Sometimes the possible gain steps are also quite large leading to\n> visible jumps if e.g. if the exposure time is fixed.\n> \n> Mitigate that by applying a global gain to account for the error\n> introduced by the exposure quantization.\n> \n> ToDo: This needs perfect frame synchronous control of the sensor to work\n> properly which is not guaranteed in all cases. It still improves the\n> behavior with the current regulation and can easily be skipped, when\n> there is no compress algorithm.\n> \n> Signed-off-by: Stefan Klug \n\nLooks ok to me:\n\nReviewed-by: Daniel Scally \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 24 +++++++++++++++++++++---\n> src/ipa/rkisp1/ipa_context.h | 2 ++\n> 2 files changed, 23 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 0a29326841fb..d34c041f9fe1 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -199,6 +199,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> \tcontext.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> \n> +\tAgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> +\t\t\t\t context.camHelper.get());\n> +\n> \tsetLimits(context.configuration.sensor.minExposureTime,\n> \t\t context.configuration.sensor.maxExposureTime,\n> \t\t context.configuration.sensor.minAnalogueGain,\n> @@ -283,6 +286,10 @@ void Agc::queueRequest(IPAContext &context,\n> \tif (!frameContext.agc.autoGainEnabled)\n> \t\tframeContext.agc.gain = agc.manual.gain;\n> \n> +\tif (!frameContext.agc.autoExposureEnabled &&\n> +\t !frameContext.agc.autoGainEnabled)\n> +\t\tframeContext.agc.quantizationGain = 1.0;\n> +\n> \tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> \tif (meteringMode) {\n> \t\tframeContext.agc.updateMetering = agc.meteringMode != *meteringMode;\n> @@ -336,12 +343,17 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> {\n> \tuint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> \tdouble activeAutoGain = context.activeState.agc.automatic.gain;\n> +\tdouble activeAutoQGain = context.activeState.agc.automatic.quantizationGain;\n> \n> \t/* Populate exposure and gain in auto mode */\n> -\tif (frameContext.agc.autoExposureEnabled)\n> +\tif (frameContext.agc.autoExposureEnabled) {\n> \t\tframeContext.agc.exposure = activeAutoExposure;\n> -\tif (frameContext.agc.autoGainEnabled)\n> +\t\tframeContext.agc.quantizationGain = activeAutoQGain;\n> +\t}\n> +\tif (frameContext.agc.autoGainEnabled) {\n> \t\tframeContext.agc.gain = activeAutoGain;\n> +\t\tframeContext.agc.quantizationGain = activeAutoQGain;\n> +\t}\n> \n> \t/*\n> \t * Populate manual exposure and gain from the active auto values when\n> @@ -354,6 +366,12 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \tif (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {\n> \t\tcontext.activeState.agc.manual.gain = activeAutoGain;\n> \t\tframeContext.agc.gain = activeAutoGain;\n> +\t\tframeContext.agc.quantizationGain = activeAutoQGain;\n> +\t}\n> +\n> +\tif (context.activeState.compress.supported) {\n> +\t\tframeContext.compress.enable = true;\n> +\t\tframeContext.compress.gain = frameContext.agc.quantizationGain;\n> \t}\n> \n> \tif (frame > 0 && !frameContext.agc.updateMetering)\n> @@ -582,7 +600,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t/* Update the estimated exposure and gain. */\n> \tactiveState.agc.automatic.exposure = newExposureTime / lineDuration;\n> \tactiveState.agc.automatic.gain = aGain;\n> -\n> +\tactiveState.agc.automatic.quantizationGain = qGain;\n> \t/*\n> \t * Expand the target frame duration so that we do not run faster than\n> \t * the minimum frame duration when we have short exposures.\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 37a74215ce19..362ab2fda5fe 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -77,6 +77,7 @@ struct IPAActiveState {\n> \t\tstruct {\n> \t\t\tuint32_t exposure;\n> \t\t\tdouble gain;\n> +\t\t\tdouble quantizationGain;\n> \t\t} automatic;\n> \n> \t\tbool autoExposureEnabled;\n> @@ -135,6 +136,7 @@ struct IPAFrameContext : public FrameContext {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t\tdouble exposureValue;\n> +\t\tdouble quantizationGain;\n> \t\tuint32_t vblank;\n> \t\tbool autoExposureEnabled;\n> \t\tbool autoGainEnabled;","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 891FCBEFBE\n\tfor ;\n\tThu, 14 Aug 2025 12:12:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F51369254;\n\tThu, 14 Aug 2025 14:12:11 +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 336D861444\n\tfor ;\n\tThu, 14 Aug 2025 14:12:10 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EABF46DF\n\tfor ;\n\tThu, 14 Aug 2025 14:11:15 +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=\"mNtSxAp3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755173476;\n\tbh=fGxgkubUKAqTZVmBmvypml32hD37mUjYoPhOCrhi0bw=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mNtSxAp3tejdWts4PRJD7PeKlE4wcxXLrnosVHasdZbnniJil9q1Ab6vWhb8nCEaq\n\tm2PvtXW4gO6U9MnTwxFTy6S+6anPLHR9dTd3eHf/4orfCMaSWLeYOj7rqich8hA7ia\n\tP2xMZFtU4JviksVP/UhH6BICqvSFASii9sRQFCIA=","Message-ID":"<7788cc88-bfb1-4cdd-9a52-cc347ba66e29@ideasonboard.com>","Date":"Thu, 14 Aug 2025 13:12:07 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 09/16] ipa: rkisp1: agc: Add correction for exposure\n\tquantization","To":"libcamera-devel@lists.libcamera.org","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-10-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20250808141315.413839-10-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]