[{"id":35354,"web_url":"https://patchwork.libcamera.org/comment/35354/","msgid":"<a1b8f9a7-525f-4e4d-be11-e6ce38a841c6@ideasonboard.com>","date":"2025-08-12T11:46:26","subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan - thanks for the patch. This helper always hurts my brain.\n\nOn 08/08/2025 15:12, Stefan Klug wrote:\n> Calculate the error introduced by quantization as \"quantization gain\"\n> and return it separately from splitExposure(). It is not included in the\n> digital gain, to not silently ignore the limits imposed by the AGC\n> configuration.\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> ---\n>   src/ipa/ipu3/algorithms/agc.cpp         |  4 +--\n>   src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--\n>   src/ipa/libipa/agc_mean_luminance.h     |  2 +-\n>   src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------\n>   src/ipa/libipa/exposure_mode_helper.h   |  2 +-\n>   src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---\n>   6 files changed, 44 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 39d0aebb0838..da045640d569 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>   \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n>   \n>   \tutils::Duration newExposureTime;\n> -\tdouble aGain, dGain;\n> -\tstd::tie(newExposureTime, aGain, dGain) =\n> +\tdouble aGain, qGain, dGain;\n> +\tstd::tie(newExposureTime, aGain, qGain, dGain) =\n>   \t\tcalculateNewEv(context.activeState.agc.constraintMode,\n>   \t\t\t       context.activeState.agc.exposureMode, hist,\n>   \t\t\t       effectiveExposureValue);\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 58c4dfc9add2..695453e7ceea 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)\n>    *\n>    * Calculate a new exposure value to try to obtain the target. The calculated\n>    * exposure value is filtered to prevent rapid changes from frame to frame, and\n> - * divided into exposure time, analogue and digital gain.\n> + * divided into exposure time, analogue, quantization and digital gain.\n>    *\n> - * \\return Tuple of exposure time, analogue gain, and digital gain\n> + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n> + * gain\n>    */\n> -std::tuple<utils::Duration, double, double>\n> +std::tuple<utils::Duration, double, double, double>\n>   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>   \t\t\t\t uint32_t exposureModeIndex,\n>   \t\t\t\t const Histogram &yHist,\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index a7c7e006af42..741ccf986625 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -68,7 +68,7 @@ public:\n>   \t\treturn controls_;\n>   \t}\n>   \n> -\tstd::tuple<utils::Duration, double, double>\n> +\tstd::tuple<utils::Duration, double, double, double>\n>   \tcalculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,\n>   \t\t       const Histogram &yHist, utils::Duration effectiveExposureValue);\n>   \n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index 7d470ebe487c..79b8e6758faf 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons\n>    * required exposure, the helper falls-back to simply maximising the exposure\n>    * time first, followed by analogue gain, followed by digital gain.\n>    *\n> - * \\return Tuple of exposure time, analogue gain, and digital gain\n> + * During the calculations the gain missed due to quantization is recorded and\n> + * returned as quantization gain. The quantization gain is not included in the\n> + * digital gain. So to exactly apply the given exposure, both quantization gain\n> + * and digital gain must be applied.\n> + *\n> + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n> + * gain\n>    */\n> -std::tuple<utils::Duration, double, double>\n> +std::tuple<utils::Duration, double, double, double>\n>   ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>   {\n>   \tASSERT(maxExposureTime_);\n>   \tASSERT(maxGain_);\n>   \n> +\tutils::Duration exposureTime;\n> +\tdouble gain;\n> +\tdouble quantGain;\n>   \tbool gainFixed = minGain_ == maxGain_;\n>   \tbool exposureTimeFixed = minExposureTime_ == maxExposureTime_;\n>   \n> @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>   \t * There's no point entering the loop if we cannot change either gain\n>   \t * nor exposure time anyway.\n>   \t */\n> -\tif (exposureTimeFixed && gainFixed)\n> -\t\treturn { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };\n> +\tif (exposureTimeFixed && gainFixed) {\n> +\t\texposureTime = clampExposureTime(minExposureTime_, &quantGain);\n> +\t\tgain = clampGain(minGain_ * quantGain, &quantGain);\n\nSo this rolls the gain lost to quantization of the exposure time into \nanalogue gain - why go for that automatically, but not also roll the \ngain lost to quantization of the analogue gain into digital gain \nautomatically?\n\n> +\n> +\t\treturn { exposureTime, gain, quantGain,\n> +\t\t\t exposure / (minExposureTime_ * minGain_ * quantGain) };\n\nI am surprised to see quantGain here...for example with the following \nvalues:\n\n\nexposure = 10000us\nminGain = 2.0\nminExposureTime_ = 4000us\nlineLength_ = 30us\nSay 16 steps of analogue gain from 1x to 16x\n\nThen\n\nfollowing clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566\nfollowing clampGain() gain = 2.0 and quantGain = 1.00250626566\n\nSo I would expect digital gain to be calculated as 1.25 and for the \nalgorithm to have to apply both dGain and qGain as digital gain such \nthat 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?\n\n\n> +\t}\n>   \n> -\tutils::Duration exposureTime;\n>   \tdouble stageGain = clampGain(1.0);\n>   \tdouble lastStageGain = stageGain;\n> -\tdouble gain;\n>   \n>   \tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> -\t\tutils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);\n> +\t\tutils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],\n> +\t\t\t\t\t\t\t\t      &quantGain);\n>   \t\tstageGain = clampGain(gains_[stage]);\n>   \n>   \t\t/*\n> @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>   \n>   \t\t/* Clamp the gain to lastStageGain and regulate exposureTime. */\n>   \t\tif (stageExposureTime * lastStageGain >= exposure) {\n> -\t\t\texposureTime = clampExposureTime(exposure / lastStageGain);\n> -\t\t\tgain = clampGain(exposure / exposureTime);\n> +\t\t\texposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);\n> +\t\t\tgain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>   \n> -\t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> +\t\t\treturn { exposureTime, gain, quantGain,\n> +\t\t\t\t exposure / (exposureTime * gain * quantGain) };\n>   \t\t}\n>   \n>   \t\t/* Clamp the exposureTime to stageExposureTime and regulate gain. */\n>   \t\tif (stageExposureTime * stageGain >= exposure) {\n>   \t\t\texposureTime = stageExposureTime;\n> -\t\t\tgain = clampGain(exposure / exposureTime);\n> +\t\t\tgain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>   \n> -\t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> +\t\t\treturn { exposureTime, gain, quantGain,\n> +\t\t\t\t exposure / (exposureTime * gain * quantGain) };\n>   \t\t}\n>   \n>   \t\tlastStageGain = stageGain;\n> @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>   \t * stages to use then the default stageGain of 1.0 is used so that\n>   \t * exposure time is maxed before gain is touched at all.\n>   \t */\n> -\texposureTime = clampExposureTime(exposure / stageGain);\n> -\tgain = clampGain(exposure / exposureTime);\n> +\texposureTime = clampExposureTime(exposure / stageGain, &quantGain);\n> +\tgain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>   \n> -\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> +\treturn { exposureTime, gain, quantGain,\n> +\t\t exposure / (exposureTime * gain * quantGain) };\n>   }\n>   \n>   /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index 8701fae9a26e..cc811e9fde18 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -30,7 +30,7 @@ public:\n>   \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n>   \t\t       double minGain, double maxGain);\n>   \n> -\tstd::tuple<utils::Duration, double, double>\n> +\tstd::tuple<utils::Duration, double, double, double>\n>   \tsplitExposure(utils::Duration exposure) const;\n>   \n>   \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 35440b67e999..0a29326841fb 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>   \tsetExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n>   \n>   \tutils::Duration newExposureTime;\n> -\tdouble aGain, dGain;\n> -\tstd::tie(newExposureTime, aGain, dGain) =\n> +\tdouble aGain, qGain, dGain;\n> +\tstd::tie(newExposureTime, aGain, qGain, dGain) =\n\nI think you'll also have to fix this for the other users of \nAgcMeanLuminance; IPU3 and C55 at least.\n\nThanks\nDan\n\n>   \t\tcalculateNewEv(frameContext.agc.constraintMode,\n>   \t\t\t       frameContext.agc.exposureMode,\n>   \t\t\t       hist, effectiveExposureValue);\n>   \n>   \tLOG(RkISP1Agc, Debug)\n> -\t\t<< \"Divided up exposure time, analogue gain and digital gain are \"\n> -\t\t<< newExposureTime << \", \" << aGain << \" and \" << dGain;\n> +\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n> +\t\t<< \" and digital gain are \" << newExposureTime << \", \" << aGain\n> +\t\t<< \", \" << qGain << \" and \" << dGain;\n>   \n>   \tIPAActiveState &activeState = context.activeState;\n>   \t/* Update the estimated exposure and gain. */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0B986BDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 12 Aug 2025 11:46:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B38CF69243;\n\tTue, 12 Aug 2025 13:46:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A42C161444\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Aug 2025 13:46:29 +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 B7FA43A4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Aug 2025 13:45:36 +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=\"N1wMdDWB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754999136;\n\tbh=UDPPopL9ZZw8l3wIQVAm5Z+C1L90NUbV43XhaVmZmYo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=N1wMdDWBGqVjhdNUnVIFbDp68NQV/vfoc3q5iFw1XgETAK6uXZzIuhlM1i2KQ7cUG\n\ttmryYBP0qQb9GcJjwCg0lkMBgxxxtfZPTJ2VLpVIhzAhfLzdyV1pZmGgPw/zeF8sEA\n\t+2oqVT66iFmiO9oM7ZvKzyaZy1olFPDJeehpKOhM=","Message-ID":"<a1b8f9a7-525f-4e4d-be11-e6ce38a841c6@ideasonboard.com>","Date":"Tue, 12 Aug 2025 12:46:26 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","To":"libcamera-devel@lists.libcamera.org","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-9-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally <dan.scally@ideasonboard.com>","In-Reply-To":"<20250808141315.413839-9-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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35366,"web_url":"https://patchwork.libcamera.org/comment/35366/","msgid":"<175507642079.944315.11134961413625529030@localhost>","date":"2025-08-13T09:13:40","subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the review.\n\nQuoting Dan Scally (2025-08-12 13:46:26)\n> Hi Stefan - thanks for the patch. This helper always hurts my brain.\n> \n> On 08/08/2025 15:12, Stefan Klug wrote:\n> > Calculate the error introduced by quantization as \"quantization gain\"\n> > and return it separately from splitExposure(). It is not included in the\n> > digital gain, to not silently ignore the limits imposed by the AGC\n> > configuration.\n> > \n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > ---\n> >   src/ipa/ipu3/algorithms/agc.cpp         |  4 +--\n> >   src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--\n> >   src/ipa/libipa/agc_mean_luminance.h     |  2 +-\n> >   src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------\n> >   src/ipa/libipa/exposure_mode_helper.h   |  2 +-\n> >   src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---\n> >   6 files changed, 44 insertions(+), 26 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index 39d0aebb0838..da045640d569 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >       utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> >   \n> >       utils::Duration newExposureTime;\n> > -     double aGain, dGain;\n> > -     std::tie(newExposureTime, aGain, dGain) =\n> > +     double aGain, qGain, dGain;\n> > +     std::tie(newExposureTime, aGain, qGain, dGain) =\n> >               calculateNewEv(context.activeState.agc.constraintMode,\n> >                              context.activeState.agc.exposureMode, hist,\n> >                              effectiveExposureValue);\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 58c4dfc9add2..695453e7ceea 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)\n> >    *\n> >    * Calculate a new exposure value to try to obtain the target. The calculated\n> >    * exposure value is filtered to prevent rapid changes from frame to frame, and\n> > - * divided into exposure time, analogue and digital gain.\n> > + * divided into exposure time, analogue, quantization and digital gain.\n> >    *\n> > - * \\return Tuple of exposure time, analogue gain, and digital gain\n> > + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n> > + * gain\n> >    */\n> > -std::tuple<utils::Duration, double, double>\n> > +std::tuple<utils::Duration, double, double, double>\n> >   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> >                                uint32_t exposureModeIndex,\n> >                                const Histogram &yHist,\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index a7c7e006af42..741ccf986625 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -68,7 +68,7 @@ public:\n> >               return controls_;\n> >       }\n> >   \n> > -     std::tuple<utils::Duration, double, double>\n> > +     std::tuple<utils::Duration, double, double, double>\n> >       calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,\n> >                      const Histogram &yHist, utils::Duration effectiveExposureValue);\n> >   \n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > index 7d470ebe487c..79b8e6758faf 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons\n> >    * required exposure, the helper falls-back to simply maximising the exposure\n> >    * time first, followed by analogue gain, followed by digital gain.\n> >    *\n> > - * \\return Tuple of exposure time, analogue gain, and digital gain\n> > + * During the calculations the gain missed due to quantization is recorded and\n> > + * returned as quantization gain. The quantization gain is not included in the\n> > + * digital gain. So to exactly apply the given exposure, both quantization gain\n> > + * and digital gain must be applied.\n> > + *\n> > + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n> > + * gain\n> >    */\n> > -std::tuple<utils::Duration, double, double>\n> > +std::tuple<utils::Duration, double, double, double>\n> >   ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> >   {\n> >       ASSERT(maxExposureTime_);\n> >       ASSERT(maxGain_);\n> >   \n> > +     utils::Duration exposureTime;\n> > +     double gain;\n> > +     double quantGain;\n> >       bool gainFixed = minGain_ == maxGain_;\n> >       bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;\n> >   \n> > @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> >        * There's no point entering the loop if we cannot change either gain\n> >        * nor exposure time anyway.\n> >        */\n> > -     if (exposureTimeFixed && gainFixed)\n> > -             return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };\n> > +     if (exposureTimeFixed && gainFixed) {\n> > +             exposureTime = clampExposureTime(minExposureTime_, &quantGain);\n> > +             gain = clampGain(minGain_ * quantGain, &quantGain);\n> \n> So this rolls the gain lost to quantization of the exposure time into \n> analogue gain - why go for that automatically, but not also roll the \n> gain lost to quantization of the analogue gain into digital gain \n> automatically?\n\nIn this case exposureTime and gain are fixed, so clamp will always\nreturn the same result. So (I hope I didn't miss something) passing the\ngain through clamp is the same as saying:\n\ndouble quantGain2;\nexposureTime = clampExposureTime(minExposureTime_, &quantGain2);\ngain = clampGain(minGain_ * quantGain, &quantGain);\nquantGain *= quantGain2;\n\n> \n> > +\n> > +             return { exposureTime, gain, quantGain,\n> > +                      exposure / (minExposureTime_ * minGain_ * quantGain) };\n> \n> I am surprised to see quantGain here...for example with the following \n> values:\n> \n> \n> exposure = 10000us\n> minGain = 2.0\n> minExposureTime_ = 4000us\n> lineLength_ = 30us\n> Say 16 steps of analogue gain from 1x to 16x\n> \n> Then\n> \n> following clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566\n> following clampGain() gain = 2.0 and quantGain = 1.00250626566\n> \n> So I would expect digital gain to be calculated as 1.25 and for the \n> algorithm to have to apply both dGain and qGain as digital gain such \n> that 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?\n\nOhh, thanks, that is a good point. My conclusion is a bit different\nthough. I think the return should look like this:\n\nreturn { exposureTime, gain, quantGain,\n         exposure / (exposureTime * gain * quantGain) };\n\nSo that the digital gain is based on the values that are actually\napplied. Then the overall result is as you'd expect.\n\n> \n> \n> > +     }\n> >   \n> > -     utils::Duration exposureTime;\n> >       double stageGain = clampGain(1.0);\n> >       double lastStageGain = stageGain;\n> > -     double gain;\n> >   \n> >       for (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> > -             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);\n> > +             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],\n> > +                                                                   &quantGain);\n> >               stageGain = clampGain(gains_[stage]);\n> >   \n> >               /*\n> > @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> >   \n> >               /* Clamp the gain to lastStageGain and regulate exposureTime. */\n> >               if (stageExposureTime * lastStageGain >= exposure) {\n> > -                     exposureTime = clampExposureTime(exposure / lastStageGain);\n> > -                     gain = clampGain(exposure / exposureTime);\n> > +                     exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);\n> > +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n> >   \n> > -                     return { exposureTime, gain, exposure / (exposureTime * gain) };\n> > +                     return { exposureTime, gain, quantGain,\n> > +                              exposure / (exposureTime * gain * quantGain) };\n> >               }\n> >   \n> >               /* Clamp the exposureTime to stageExposureTime and regulate gain. */\n> >               if (stageExposureTime * stageGain >= exposure) {\n> >                       exposureTime = stageExposureTime;\n> > -                     gain = clampGain(exposure / exposureTime);\n> > +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n> >   \n> > -                     return { exposureTime, gain, exposure / (exposureTime * gain) };\n> > +                     return { exposureTime, gain, quantGain,\n> > +                              exposure / (exposureTime * gain * quantGain) };\n> >               }\n> >   \n> >               lastStageGain = stageGain;\n> > @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> >        * stages to use then the default stageGain of 1.0 is used so that\n> >        * exposure time is maxed before gain is touched at all.\n> >        */\n> > -     exposureTime = clampExposureTime(exposure / stageGain);\n> > -     gain = clampGain(exposure / exposureTime);\n> > +     exposureTime = clampExposureTime(exposure / stageGain, &quantGain);\n> > +     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n> >   \n> > -     return { exposureTime, gain, exposure / (exposureTime * gain) };\n> > +     return { exposureTime, gain, quantGain,\n> > +              exposure / (exposureTime * gain * quantGain) };\n> >   }\n> >   \n> >   /**\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > index 8701fae9a26e..cc811e9fde18 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.h\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -30,7 +30,7 @@ public:\n> >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> >                      double minGain, double maxGain);\n> >   \n> > -     std::tuple<utils::Duration, double, double>\n> > +     std::tuple<utils::Duration, double, double, double>\n> >       splitExposure(utils::Duration exposure) const;\n> >   \n> >       utils::Duration minExposureTime() const { return minExposureTime_; }\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 35440b67e999..0a29326841fb 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> >   \n> >       utils::Duration newExposureTime;\n> > -     double aGain, dGain;\n> > -     std::tie(newExposureTime, aGain, dGain) =\n> > +     double aGain, qGain, dGain;\n> > +     std::tie(newExposureTime, aGain, qGain, dGain) =\n> \n> I think you'll also have to fix this for the other users of \n> AgcMeanLuminance; IPU3 and C55 at least.\n\nYes, CI told me that also :-)\n\n> \n> Thanks\n> Dan\n\nThanks,\nStefan\n\n> \n> >               calculateNewEv(frameContext.agc.constraintMode,\n> >                              frameContext.agc.exposureMode,\n> >                              hist, effectiveExposureValue);\n> >   \n> >       LOG(RkISP1Agc, Debug)\n> > -             << \"Divided up exposure time, analogue gain and digital gain are \"\n> > -             << newExposureTime << \", \" << aGain << \" and \" << dGain;\n> > +             << \"Divided up exposure time, analogue gain, quantization gain\"\n> > +             << \" and digital gain are \" << newExposureTime << \", \" << aGain\n> > +             << \", \" << qGain << \" and \" << dGain;\n> >   \n> >       IPAActiveState &activeState = context.activeState;\n> >       /* Update the estimated exposure and gain. */\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6BB5DBEFBE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Aug 2025 09:13:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 927CD69249;\n\tWed, 13 Aug 2025 11:13:45 +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 E4AF761443\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Aug 2025 11:13:43 +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 5AE6E229;\n\tWed, 13 Aug 2025 11:12: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=\"Kc1OZ9jP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755076370;\n\tbh=N9yLCL+4c9LM1K1uTbwO+jTHsXYXStYLmUgd9AccDq4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Kc1OZ9jPuRj/BhQsilEM1OsnmUWmkrMf7Dl0luSut8XbXroOqPn8Ug6r0Pcv6AsLC\n\trlW6kATHjS0BxdhWCHXSK42SQYD9HnDJ1ExTLcxYx1KPsx5Tx7i1C/icFEp/4yNSm3\n\tEjMwfSTLzhcKvKghOPv+jMJMhtXJ5dqwVmtoExhI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<a1b8f9a7-525f-4e4d-be11-e6ce38a841c6@ideasonboard.com>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-9-stefan.klug@ideasonboard.com>\n\t<a1b8f9a7-525f-4e4d-be11-e6ce38a841c6@ideasonboard.com>","Subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Dan Scally <dan.scally@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 11:13:40 +0200","Message-ID":"<175507642079.944315.11134961413625529030@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35405,"web_url":"https://patchwork.libcamera.org/comment/35405/","msgid":"<76b30042-a0a1-43a1-8988-030c48e7dc18@ideasonboard.com>","date":"2025-08-14T11:34:31","subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 13/08/2025 10:13, Stefan Klug wrote:\n> Hi Dan,\n> \n> Thank you for the review.\n> \n> Quoting Dan Scally (2025-08-12 13:46:26)\n>> Hi Stefan - thanks for the patch. This helper always hurts my brain.\n>>\n>> On 08/08/2025 15:12, Stefan Klug wrote:\n>>> Calculate the error introduced by quantization as \"quantization gain\"\n>>> and return it separately from splitExposure(). It is not included in the\n>>> digital gain, to not silently ignore the limits imposed by the AGC\n>>> configuration.\n>>>\n>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n>>> ---\n>>>    src/ipa/ipu3/algorithms/agc.cpp         |  4 +--\n>>>    src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--\n>>>    src/ipa/libipa/agc_mean_luminance.h     |  2 +-\n>>>    src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------\n>>>    src/ipa/libipa/exposure_mode_helper.h   |  2 +-\n>>>    src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---\n>>>    6 files changed, 44 insertions(+), 26 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>>> index 39d0aebb0838..da045640d569 100644\n>>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>>> @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>>>        utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n>>>    \n>>>        utils::Duration newExposureTime;\n>>> -     double aGain, dGain;\n>>> -     std::tie(newExposureTime, aGain, dGain) =\n>>> +     double aGain, qGain, dGain;\n>>> +     std::tie(newExposureTime, aGain, qGain, dGain) =\n>>>                calculateNewEv(context.activeState.agc.constraintMode,\n>>>                               context.activeState.agc.exposureMode, hist,\n>>>                               effectiveExposureValue);\n>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>>> index 58c4dfc9add2..695453e7ceea 100644\n>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>>> @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)\n>>>     *\n>>>     * Calculate a new exposure value to try to obtain the target. The calculated\n>>>     * exposure value is filtered to prevent rapid changes from frame to frame, and\n>>> - * divided into exposure time, analogue and digital gain.\n>>> + * divided into exposure time, analogue, quantization and digital gain.\n>>>     *\n>>> - * \\return Tuple of exposure time, analogue gain, and digital gain\n>>> + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n>>> + * gain\n>>>     */\n>>> -std::tuple<utils::Duration, double, double>\n>>> +std::tuple<utils::Duration, double, double, double>\n>>>    AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>>>                                 uint32_t exposureModeIndex,\n>>>                                 const Histogram &yHist,\n>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n>>> index a7c7e006af42..741ccf986625 100644\n>>> --- a/src/ipa/libipa/agc_mean_luminance.h\n>>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n>>> @@ -68,7 +68,7 @@ public:\n>>>                return controls_;\n>>>        }\n>>>    \n>>> -     std::tuple<utils::Duration, double, double>\n>>> +     std::tuple<utils::Duration, double, double, double>\n>>>        calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,\n>>>                       const Histogram &yHist, utils::Duration effectiveExposureValue);\n>>>    \n>>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n>>> index 7d470ebe487c..79b8e6758faf 100644\n>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>>> @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons\n>>>     * required exposure, the helper falls-back to simply maximising the exposure\n>>>     * time first, followed by analogue gain, followed by digital gain.\n>>>     *\n>>> - * \\return Tuple of exposure time, analogue gain, and digital gain\n>>> + * During the calculations the gain missed due to quantization is recorded and\n>>> + * returned as quantization gain. The quantization gain is not included in the\n>>> + * digital gain. So to exactly apply the given exposure, both quantization gain\n>>> + * and digital gain must be applied.\n>>> + *\n>>> + * \\return Tuple of exposure time, analogue gain, quantization gain and digital\n>>> + * gain\n>>>     */\n>>> -std::tuple<utils::Duration, double, double>\n>>> +std::tuple<utils::Duration, double, double, double>\n>>>    ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>    {\n>>>        ASSERT(maxExposureTime_);\n>>>        ASSERT(maxGain_);\n>>>    \n>>> +     utils::Duration exposureTime;\n>>> +     double gain;\n>>> +     double quantGain;\n>>>        bool gainFixed = minGain_ == maxGain_;\n>>>        bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;\n>>>    \n>>> @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>         * There's no point entering the loop if we cannot change either gain\n>>>         * nor exposure time anyway.\n>>>         */\n>>> -     if (exposureTimeFixed && gainFixed)\n>>> -             return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };\n>>> +     if (exposureTimeFixed && gainFixed) {\n>>> +             exposureTime = clampExposureTime(minExposureTime_, &quantGain);\n>>> +             gain = clampGain(minGain_ * quantGain, &quantGain);\n>>\n>> So this rolls the gain lost to quantization of the exposure time into\n>> analogue gain - why go for that automatically, but not also roll the\n>> gain lost to quantization of the analogue gain into digital gain\n>> automatically?\n> \n> In this case exposureTime and gain are fixed, so clamp will always\n> return the same result. So (I hope I didn't miss something) passing the\n> gain through clamp is the same as saying:\n> \n> double quantGain2;\n> exposureTime = clampExposureTime(minExposureTime_, &quantGain2);\n> gain = clampGain(minGain_ * quantGain, &quantGain);\n> quantGain *= quantGain2;\n\nYes I think so, but the same pattern is in the calculations within the \nloop too where the exposure time and gain aren't fixed. Let me move this \nconversation down there so it's in the right context...\n\n> \n>>\n>>> +\n>>> +             return { exposureTime, gain, quantGain,\n>>> +                      exposure / (minExposureTime_ * minGain_ * quantGain) };\n>>\n>> I am surprised to see quantGain here...for example with the following\n>> values:\n>>\n>>\n>> exposure = 10000us\n>> minGain = 2.0\n>> minExposureTime_ = 4000us\n>> lineLength_ = 30us\n>> Say 16 steps of analogue gain from 1x to 16x\n>>\n>> Then\n>>\n>> following clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566\n>> following clampGain() gain = 2.0 and quantGain = 1.00250626566\n>>\n>> So I would expect digital gain to be calculated as 1.25 and for the\n>> algorithm to have to apply both dGain and qGain as digital gain such\n>> that 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?\n> \n> Ohh, thanks, that is a good point. My conclusion is a bit different\n> though. I think the return should look like this:\n> \n> return { exposureTime, gain, quantGain,\n>           exposure / (exposureTime * gain * quantGain) };\n> \n> So that the digital gain is based on the values that are actually\n> applied. Then the overall result is as you'd expect.\n\nThat also looks fine to me\n\n> \n>>\n>>\n>>> +     }\n>>>    \n>>> -     utils::Duration exposureTime;\n>>>        double stageGain = clampGain(1.0);\n>>>        double lastStageGain = stageGain;\n>>> -     double gain;\n>>>    \n>>>        for (unsigned int stage = 0; stage < gains_.size(); stage++) {\n>>> -             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);\n>>> +             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],\n>>> +                                                                   &quantGain);\n>>>                stageGain = clampGain(gains_[stage]);\n>>>    \n>>>                /*\n>>> @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>    \n>>>                /* Clamp the gain to lastStageGain and regulate exposureTime. */\n>>>                if (stageExposureTime * lastStageGain >= exposure) {\n>>> -                     exposureTime = clampExposureTime(exposure / lastStageGain);\n>>> -                     gain = clampGain(exposure / exposureTime);\n>>> +                     exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);\n>>> +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>>>    \n\nIf we have things set like:\n\nexposure = 14380us\nlineLength_ = 30us\nImx477 gain model: AnalogueGainLinear{ 0, 1024, -1, 1024 };\nexposure steps = 1000, 2000, 4000, 8000, 16000\ngain steps = 2.0, 3.8, 5.6, 7.4, 9.2\n\n(settings chosen to exacerbate problems which probably wouldn't appear \nall that often...)\n\nThen after two iterations of the loop we run clampExposureTime(14380 / \n3.8, &guantGain) and get\n\nexposureTime = 3780us\nquantGain = 1.00111389585\n\nAnd then with the implementation as-is we run clampGain(3.80847032337, \n&quantGain);\n\nIf the sensor were capable of perfectly granular exposure and gain \ncontrol we'd be setting 3784us and 3.8x, which means the exposure value \nlost to quantisation of the exposure time has been added back as \nanalogue gain. I don't have a problem with that, but it seems at odds \nwith the not automatically adding gain lost to quantisation of analogue \ngain into the digital gain - why do one but not the other? I suppose \nparticularly within this loop, since it shouldn't ever resort to digital \ngain to fulfil the requested exposure value under conditions of perfect \ngranularity.\n\nNow that I put numbers into the formula though I wonder if that needs \nlooking at...in the current implementation I think we would return here...\n\nexposureTime = 3780us\naGain = 3.80669\nqGain = 1.00046768278\ndGain = 0.998887343535\n\nAnd whilst that gets the right value it means that qGain * dGain = \n0.999354505945 which is probably something we want to avoid. I start to \nlean towards the view that it's simplest to calculate the gain needed to \ncorrect the quantisation of exposure and analogue gain separately and \ncombine them, without letting any correction for exposure quantisation \ngo into analogue gain:\n\nexposureTime = clampExposureTime(exposure / lastStageGain, &quantGain1);\ngain = clampGain(exposure / (exposureTime * quantGain), &quantGain2);\nquantGain = quantGain1 * quantGain2;\n\nreturn { exposureTime, gain, quantGain, exposure / (exposureTime * gain \n* quantGain) };\n\nThen we get:\n\nexposureTime = 3780, quantGain1 = 1.00111389585\naGain = 3.79259, quantGain2 = 1.00195380993\nqGain = 1.00306988212\ndGain = 1.0\n\nLike I said, this helper hurts my brain...what do you think?\n\nDan\n\n>>> -                     return { exposureTime, gain, exposure / (exposureTime * gain) };\n>>> +                     return { exposureTime, gain, quantGain,\n>>> +                              exposure / (exposureTime * gain * quantGain) };\n>>>                }\n>>>    \n>>>                /* Clamp the exposureTime to stageExposureTime and regulate gain. */\n>>>                if (stageExposureTime * stageGain >= exposure) {\n>>>                        exposureTime = stageExposureTime;\n>>> -                     gain = clampGain(exposure / exposureTime);\n>>> +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>>>    \n>>> -                     return { exposureTime, gain, exposure / (exposureTime * gain) };\n>>> +                     return { exposureTime, gain, quantGain,\n>>> +                              exposure / (exposureTime * gain * quantGain) };\n>>>                }\n>>>    \n>>>                lastStageGain = stageGain;\n>>> @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>         * stages to use then the default stageGain of 1.0 is used so that\n>>>         * exposure time is maxed before gain is touched at all.\n>>>         */\n>>> -     exposureTime = clampExposureTime(exposure / stageGain);\n>>> -     gain = clampGain(exposure / exposureTime);\n>>> +     exposureTime = clampExposureTime(exposure / stageGain, &quantGain);\n>>> +     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);\n>>>    \n>>> -     return { exposureTime, gain, exposure / (exposureTime * gain) };\n>>> +     return { exposureTime, gain, quantGain,\n>>> +              exposure / (exposureTime * gain * quantGain) };\n>>>    }\n>>>    \n>>>    /**\n>>> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n>>> index 8701fae9a26e..cc811e9fde18 100644\n>>> --- a/src/ipa/libipa/exposure_mode_helper.h\n>>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>>> @@ -30,7 +30,7 @@ public:\n>>>        void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n>>>                       double minGain, double maxGain);\n>>>    \n>>> -     std::tuple<utils::Duration, double, double>\n>>> +     std::tuple<utils::Duration, double, double, double>\n>>>        splitExposure(utils::Duration exposure) const;\n>>>    \n>>>        utils::Duration minExposureTime() const { return minExposureTime_; }\n>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>>> index 35440b67e999..0a29326841fb 100644\n>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>>> @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>>>        setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n>>>    \n>>>        utils::Duration newExposureTime;\n>>> -     double aGain, dGain;\n>>> -     std::tie(newExposureTime, aGain, dGain) =\n>>> +     double aGain, qGain, dGain;\n>>> +     std::tie(newExposureTime, aGain, qGain, dGain) =\n>>\n>> I think you'll also have to fix this for the other users of\n>> AgcMeanLuminance; IPU3 and C55 at least.\n> \n> Yes, CI told me that also :-)\n> \n>>\n>> Thanks\n>> Dan\n> \n> Thanks,\n> Stefan\n> \n>>\n>>>                calculateNewEv(frameContext.agc.constraintMode,\n>>>                               frameContext.agc.exposureMode,\n>>>                               hist, effectiveExposureValue);\n>>>    \n>>>        LOG(RkISP1Agc, Debug)\n>>> -             << \"Divided up exposure time, analogue gain and digital gain are \"\n>>> -             << newExposureTime << \", \" << aGain << \" and \" << dGain;\n>>> +             << \"Divided up exposure time, analogue gain, quantization gain\"\n>>> +             << \" and digital gain are \" << newExposureTime << \", \" << aGain\n>>> +             << \", \" << qGain << \" and \" << dGain;\n>>>    \n>>>        IPAActiveState &activeState = context.activeState;\n>>>        /* Update the estimated exposure and gain. */\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DE69CBDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 14 Aug 2025 11:34:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED82669254;\n\tThu, 14 Aug 2025 13:34:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A07A61444\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 Aug 2025 13:34:34 +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 1FE3556D;\n\tThu, 14 Aug 2025 13:33:40 +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=\"lkXw+FKr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755171220;\n\tbh=gFbU3zH2i095hw+N4xif6XpJzbdyBvgg6xhVv5TIMnU=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=lkXw+FKrX2SQ15erT06jUzNw3evFm8nvp8gAmx5l3owpdIL3BSSo4r+W08Ovisywq\n\t+gvph69FfJv/3WO+GQIacDeGIeP7JlaCkSFljjWJuwcQQghJwlhF/FXSXu/MaI28Aq\n\tpGFYRQVGgtHyUMOnho1vYMN+iWh9Qnb1lKq67jD8=","Message-ID":"<76b30042-a0a1-43a1-8988-030c48e7dc18@ideasonboard.com>","Date":"Thu, 14 Aug 2025 12:34:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 08/16] libipa: exposure_mode_helper: Calculate\n\tquantization gain in splitExposure()","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-9-stefan.klug@ideasonboard.com>\n\t<a1b8f9a7-525f-4e4d-be11-e6ce38a841c6@ideasonboard.com>\n\t<175507642079.944315.11134961413625529030@localhost>","Content-Language":"en-US","From":"Dan Scally <dan.scally@ideasonboard.com>","In-Reply-To":"<175507642079.944315.11134961413625529030@localhost>","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]