[v2,08/16] libipa: exposure_mode_helper: Calculate quantization gain in splitExposure()
diff mbox series

Message ID 20250808141315.413839-9-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Implement WDR algorithm
Related show

Commit Message

Stefan Klug Aug. 8, 2025, 2:12 p.m. UTC 
Calculate the error introduced by quantization as "quantization gain"
and return it separately from splitExposure(). It is not included in the
digital gain, to not silently ignore the limits imposed by the AGC
configuration.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp         |  4 +--
 src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--
 src/ipa/libipa/agc_mean_luminance.h     |  2 +-
 src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------
 src/ipa/libipa/exposure_mode_helper.h   |  2 +-
 src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---
 6 files changed, 44 insertions(+), 26 deletions(-)

Comments

Dan Scally Aug. 12, 2025, 11:46 a.m. UTC | #1 
Hi Stefan - thanks for the patch. This helper always hurts my brain.

On 08/08/2025 15:12, Stefan Klug wrote:
> Calculate the error introduced by quantization as "quantization gain"
> and return it separately from splitExposure(). It is not included in the
> digital gain, to not silently ignore the limits imposed by the AGC
> configuration.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/ipa/ipu3/algorithms/agc.cpp         |  4 +--
>   src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--
>   src/ipa/libipa/agc_mean_luminance.h     |  2 +-
>   src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------
>   src/ipa/libipa/exposure_mode_helper.h   |  2 +-
>   src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---
>   6 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 39d0aebb0838..da045640d569 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>   
>   	utils::Duration newExposureTime;
> -	double aGain, dGain;
> -	std::tie(newExposureTime, aGain, dGain) =
> +	double aGain, qGain, dGain;
> +	std::tie(newExposureTime, aGain, qGain, dGain) =
>   		calculateNewEv(context.activeState.agc.constraintMode,
>   			       context.activeState.agc.exposureMode, hist,
>   			       effectiveExposureValue);
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 58c4dfc9add2..695453e7ceea 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
>    *
>    * Calculate a new exposure value to try to obtain the target. The calculated
>    * exposure value is filtered to prevent rapid changes from frame to frame, and
> - * divided into exposure time, analogue and digital gain.
> + * divided into exposure time, analogue, quantization and digital gain.
>    *
> - * \return Tuple of exposure time, analogue gain, and digital gain
> + * \return Tuple of exposure time, analogue gain, quantization gain and digital
> + * gain
>    */
> -std::tuple<utils::Duration, double, double>
> +std::tuple<utils::Duration, double, double, double>
>   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>   				 uint32_t exposureModeIndex,
>   				 const Histogram &yHist,
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index a7c7e006af42..741ccf986625 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -68,7 +68,7 @@ public:
>   		return controls_;
>   	}
>   
> -	std::tuple<utils::Duration, double, double>
> +	std::tuple<utils::Duration, double, double, double>
>   	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
>   		       const Histogram &yHist, utils::Duration effectiveExposureValue);
>   
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index 7d470ebe487c..79b8e6758faf 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons
>    * required exposure, the helper falls-back to simply maximising the exposure
>    * time first, followed by analogue gain, followed by digital gain.
>    *
> - * \return Tuple of exposure time, analogue gain, and digital gain
> + * During the calculations the gain missed due to quantization is recorded and
> + * returned as quantization gain. The quantization gain is not included in the
> + * digital gain. So to exactly apply the given exposure, both quantization gain
> + * and digital gain must be applied.
> + *
> + * \return Tuple of exposure time, analogue gain, quantization gain and digital
> + * gain
>    */
> -std::tuple<utils::Duration, double, double>
> +std::tuple<utils::Duration, double, double, double>
>   ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   {
>   	ASSERT(maxExposureTime_);
>   	ASSERT(maxGain_);
>   
> +	utils::Duration exposureTime;
> +	double gain;
> +	double quantGain;
>   	bool gainFixed = minGain_ == maxGain_;
>   	bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;
>   
> @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   	 * There's no point entering the loop if we cannot change either gain
>   	 * nor exposure time anyway.
>   	 */
> -	if (exposureTimeFixed && gainFixed)
> -		return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };
> +	if (exposureTimeFixed && gainFixed) {
> +		exposureTime = clampExposureTime(minExposureTime_, &quantGain);
> +		gain = clampGain(minGain_ * quantGain, &quantGain);

So this rolls the gain lost to quantization of the exposure time into 
analogue gain - why go for that automatically, but not also roll the 
gain lost to quantization of the analogue gain into digital gain 
automatically?

> +
> +		return { exposureTime, gain, quantGain,
> +			 exposure / (minExposureTime_ * minGain_ * quantGain) };

I am surprised to see quantGain here...for example with the following 
values:


exposure = 10000us
minGain = 2.0
minExposureTime_ = 4000us
lineLength_ = 30us
Say 16 steps of analogue gain from 1x to 16x

Then

following clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566
following clampGain() gain = 2.0 and quantGain = 1.00250626566

So I would expect digital gain to be calculated as 1.25 and for the 
algorithm to have to apply both dGain and qGain as digital gain such 
that 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?


> +	}
>   
> -	utils::Duration exposureTime;
>   	double stageGain = clampGain(1.0);
>   	double lastStageGain = stageGain;
> -	double gain;
>   
>   	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
> -		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);
> +		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
> +								      &quantGain);
>   		stageGain = clampGain(gains_[stage]);
>   
>   		/*
> @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   
>   		/* Clamp the gain to lastStageGain and regulate exposureTime. */
>   		if (stageExposureTime * lastStageGain >= exposure) {
> -			exposureTime = clampExposureTime(exposure / lastStageGain);
> -			gain = clampGain(exposure / exposureTime);
> +			exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);
> +			gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>   
> -			return { exposureTime, gain, exposure / (exposureTime * gain) };
> +			return { exposureTime, gain, quantGain,
> +				 exposure / (exposureTime * gain * quantGain) };
>   		}
>   
>   		/* Clamp the exposureTime to stageExposureTime and regulate gain. */
>   		if (stageExposureTime * stageGain >= exposure) {
>   			exposureTime = stageExposureTime;
> -			gain = clampGain(exposure / exposureTime);
> +			gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>   
> -			return { exposureTime, gain, exposure / (exposureTime * gain) };
> +			return { exposureTime, gain, quantGain,
> +				 exposure / (exposureTime * gain * quantGain) };
>   		}
>   
>   		lastStageGain = stageGain;
> @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   	 * stages to use then the default stageGain of 1.0 is used so that
>   	 * exposure time is maxed before gain is touched at all.
>   	 */
> -	exposureTime = clampExposureTime(exposure / stageGain);
> -	gain = clampGain(exposure / exposureTime);
> +	exposureTime = clampExposureTime(exposure / stageGain, &quantGain);
> +	gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>   
> -	return { exposureTime, gain, exposure / (exposureTime * gain) };
> +	return { exposureTime, gain, quantGain,
> +		 exposure / (exposureTime * gain * quantGain) };
>   }
>   
>   /**
> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> index 8701fae9a26e..cc811e9fde18 100644
> --- a/src/ipa/libipa/exposure_mode_helper.h
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -30,7 +30,7 @@ public:
>   	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>   		       double minGain, double maxGain);
>   
> -	std::tuple<utils::Duration, double, double>
> +	std::tuple<utils::Duration, double, double, double>
>   	splitExposure(utils::Duration exposure) const;
>   
>   	utils::Duration minExposureTime() const { return minExposureTime_; }
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 35440b67e999..0a29326841fb 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
>   
>   	utils::Duration newExposureTime;
> -	double aGain, dGain;
> -	std::tie(newExposureTime, aGain, dGain) =
> +	double aGain, qGain, dGain;
> +	std::tie(newExposureTime, aGain, qGain, dGain) =

I think you'll also have to fix this for the other users of 
AgcMeanLuminance; IPU3 and C55 at least.

Thanks
Dan

>   		calculateNewEv(frameContext.agc.constraintMode,
>   			       frameContext.agc.exposureMode,
>   			       hist, effectiveExposureValue);
>   
>   	LOG(RkISP1Agc, Debug)
> -		<< "Divided up exposure time, analogue gain and digital gain are "
> -		<< newExposureTime << ", " << aGain << " and " << dGain;
> +		<< "Divided up exposure time, analogue gain, quantization gain"
> +		<< " and digital gain are " << newExposureTime << ", " << aGain
> +		<< ", " << qGain << " and " << dGain;
>   
>   	IPAActiveState &activeState = context.activeState;
>   	/* Update the estimated exposure and gain. */
Stefan Klug Aug. 13, 2025, 9:13 a.m. UTC | #2 
Hi Dan,

Thank you for the review.

Quoting Dan Scally (2025-08-12 13:46:26)
> Hi Stefan - thanks for the patch. This helper always hurts my brain.
> 
> On 08/08/2025 15:12, Stefan Klug wrote:
> > Calculate the error introduced by quantization as "quantization gain"
> > and return it separately from splitExposure(). It is not included in the
> > digital gain, to not silently ignore the limits imposed by the AGC
> > configuration.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/ipa/ipu3/algorithms/agc.cpp         |  4 +--
> >   src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--
> >   src/ipa/libipa/agc_mean_luminance.h     |  2 +-
> >   src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------
> >   src/ipa/libipa/exposure_mode_helper.h   |  2 +-
> >   src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---
> >   6 files changed, 44 insertions(+), 26 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 39d0aebb0838..da045640d569 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >       utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> >   
> >       utils::Duration newExposureTime;
> > -     double aGain, dGain;
> > -     std::tie(newExposureTime, aGain, dGain) =
> > +     double aGain, qGain, dGain;
> > +     std::tie(newExposureTime, aGain, qGain, dGain) =
> >               calculateNewEv(context.activeState.agc.constraintMode,
> >                              context.activeState.agc.exposureMode, hist,
> >                              effectiveExposureValue);
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 58c4dfc9add2..695453e7ceea 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
> >    *
> >    * Calculate a new exposure value to try to obtain the target. The calculated
> >    * exposure value is filtered to prevent rapid changes from frame to frame, and
> > - * divided into exposure time, analogue and digital gain.
> > + * divided into exposure time, analogue, quantization and digital gain.
> >    *
> > - * \return Tuple of exposure time, analogue gain, and digital gain
> > + * \return Tuple of exposure time, analogue gain, quantization gain and digital
> > + * gain
> >    */
> > -std::tuple<utils::Duration, double, double>
> > +std::tuple<utils::Duration, double, double, double>
> >   AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> >                                uint32_t exposureModeIndex,
> >                                const Histogram &yHist,
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index a7c7e006af42..741ccf986625 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -68,7 +68,7 @@ public:
> >               return controls_;
> >       }
> >   
> > -     std::tuple<utils::Duration, double, double>
> > +     std::tuple<utils::Duration, double, double, double>
> >       calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
> >                      const Histogram &yHist, utils::Duration effectiveExposureValue);
> >   
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 7d470ebe487c..79b8e6758faf 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons
> >    * required exposure, the helper falls-back to simply maximising the exposure
> >    * time first, followed by analogue gain, followed by digital gain.
> >    *
> > - * \return Tuple of exposure time, analogue gain, and digital gain
> > + * During the calculations the gain missed due to quantization is recorded and
> > + * returned as quantization gain. The quantization gain is not included in the
> > + * digital gain. So to exactly apply the given exposure, both quantization gain
> > + * and digital gain must be applied.
> > + *
> > + * \return Tuple of exposure time, analogue gain, quantization gain and digital
> > + * gain
> >    */
> > -std::tuple<utils::Duration, double, double>
> > +std::tuple<utils::Duration, double, double, double>
> >   ExposureModeHelper::splitExposure(utils::Duration exposure) const
> >   {
> >       ASSERT(maxExposureTime_);
> >       ASSERT(maxGain_);
> >   
> > +     utils::Duration exposureTime;
> > +     double gain;
> > +     double quantGain;
> >       bool gainFixed = minGain_ == maxGain_;
> >       bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;
> >   
> > @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
> >        * There's no point entering the loop if we cannot change either gain
> >        * nor exposure time anyway.
> >        */
> > -     if (exposureTimeFixed && gainFixed)
> > -             return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };
> > +     if (exposureTimeFixed && gainFixed) {
> > +             exposureTime = clampExposureTime(minExposureTime_, &quantGain);
> > +             gain = clampGain(minGain_ * quantGain, &quantGain);
> 
> So this rolls the gain lost to quantization of the exposure time into 
> analogue gain - why go for that automatically, but not also roll the 
> gain lost to quantization of the analogue gain into digital gain 
> automatically?

In this case exposureTime and gain are fixed, so clamp will always
return the same result. So (I hope I didn't miss something) passing the
gain through clamp is the same as saying:

double quantGain2;
exposureTime = clampExposureTime(minExposureTime_, &quantGain2);
gain = clampGain(minGain_ * quantGain, &quantGain);
quantGain *= quantGain2;

> 
> > +
> > +             return { exposureTime, gain, quantGain,
> > +                      exposure / (minExposureTime_ * minGain_ * quantGain) };
> 
> I am surprised to see quantGain here...for example with the following 
> values:
> 
> 
> exposure = 10000us
> minGain = 2.0
> minExposureTime_ = 4000us
> lineLength_ = 30us
> Say 16 steps of analogue gain from 1x to 16x
> 
> Then
> 
> following clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566
> following clampGain() gain = 2.0 and quantGain = 1.00250626566
> 
> So I would expect digital gain to be calculated as 1.25 and for the 
> algorithm to have to apply both dGain and qGain as digital gain such 
> that 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?

Ohh, thanks, that is a good point. My conclusion is a bit different
though. I think the return should look like this:

return { exposureTime, gain, quantGain,
         exposure / (exposureTime * gain * quantGain) };

So that the digital gain is based on the values that are actually
applied. Then the overall result is as you'd expect.

> 
> 
> > +     }
> >   
> > -     utils::Duration exposureTime;
> >       double stageGain = clampGain(1.0);
> >       double lastStageGain = stageGain;
> > -     double gain;
> >   
> >       for (unsigned int stage = 0; stage < gains_.size(); stage++) {
> > -             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);
> > +             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
> > +                                                                   &quantGain);
> >               stageGain = clampGain(gains_[stage]);
> >   
> >               /*
> > @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
> >   
> >               /* Clamp the gain to lastStageGain and regulate exposureTime. */
> >               if (stageExposureTime * lastStageGain >= exposure) {
> > -                     exposureTime = clampExposureTime(exposure / lastStageGain);
> > -                     gain = clampGain(exposure / exposureTime);
> > +                     exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);
> > +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
> >   
> > -                     return { exposureTime, gain, exposure / (exposureTime * gain) };
> > +                     return { exposureTime, gain, quantGain,
> > +                              exposure / (exposureTime * gain * quantGain) };
> >               }
> >   
> >               /* Clamp the exposureTime to stageExposureTime and regulate gain. */
> >               if (stageExposureTime * stageGain >= exposure) {
> >                       exposureTime = stageExposureTime;
> > -                     gain = clampGain(exposure / exposureTime);
> > +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
> >   
> > -                     return { exposureTime, gain, exposure / (exposureTime * gain) };
> > +                     return { exposureTime, gain, quantGain,
> > +                              exposure / (exposureTime * gain * quantGain) };
> >               }
> >   
> >               lastStageGain = stageGain;
> > @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
> >        * stages to use then the default stageGain of 1.0 is used so that
> >        * exposure time is maxed before gain is touched at all.
> >        */
> > -     exposureTime = clampExposureTime(exposure / stageGain);
> > -     gain = clampGain(exposure / exposureTime);
> > +     exposureTime = clampExposureTime(exposure / stageGain, &quantGain);
> > +     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
> >   
> > -     return { exposureTime, gain, exposure / (exposureTime * gain) };
> > +     return { exposureTime, gain, quantGain,
> > +              exposure / (exposureTime * gain * quantGain) };
> >   }
> >   
> >   /**
> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > index 8701fae9a26e..cc811e9fde18 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.h
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -30,7 +30,7 @@ public:
> >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                      double minGain, double maxGain);
> >   
> > -     std::tuple<utils::Duration, double, double>
> > +     std::tuple<utils::Duration, double, double, double>
> >       splitExposure(utils::Duration exposure) const;
> >   
> >       utils::Duration minExposureTime() const { return minExposureTime_; }
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 35440b67e999..0a29326841fb 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> >   
> >       utils::Duration newExposureTime;
> > -     double aGain, dGain;
> > -     std::tie(newExposureTime, aGain, dGain) =
> > +     double aGain, qGain, dGain;
> > +     std::tie(newExposureTime, aGain, qGain, dGain) =
> 
> I think you'll also have to fix this for the other users of 
> AgcMeanLuminance; IPU3 and C55 at least.

Yes, CI told me that also :-)

> 
> Thanks
> Dan

Thanks,
Stefan

> 
> >               calculateNewEv(frameContext.agc.constraintMode,
> >                              frameContext.agc.exposureMode,
> >                              hist, effectiveExposureValue);
> >   
> >       LOG(RkISP1Agc, Debug)
> > -             << "Divided up exposure time, analogue gain and digital gain are "
> > -             << newExposureTime << ", " << aGain << " and " << dGain;
> > +             << "Divided up exposure time, analogue gain, quantization gain"
> > +             << " and digital gain are " << newExposureTime << ", " << aGain
> > +             << ", " << qGain << " and " << dGain;
> >   
> >       IPAActiveState &activeState = context.activeState;
> >       /* Update the estimated exposure and gain. */
>
Dan Scally Aug. 14, 2025, 11:34 a.m. UTC | #3 
Hi Stefan

On 13/08/2025 10:13, Stefan Klug wrote:
> Hi Dan,
> 
> Thank you for the review.
> 
> Quoting Dan Scally (2025-08-12 13:46:26)
>> Hi Stefan - thanks for the patch. This helper always hurts my brain.
>>
>> On 08/08/2025 15:12, Stefan Klug wrote:
>>> Calculate the error introduced by quantization as "quantization gain"
>>> and return it separately from splitExposure(). It is not included in the
>>> digital gain, to not silently ignore the limits imposed by the AGC
>>> configuration.
>>>
>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>>> ---
>>>    src/ipa/ipu3/algorithms/agc.cpp         |  4 +--
>>>    src/ipa/libipa/agc_mean_luminance.cpp   |  7 ++--
>>>    src/ipa/libipa/agc_mean_luminance.h     |  2 +-
>>>    src/ipa/libipa/exposure_mode_helper.cpp | 46 +++++++++++++++++--------
>>>    src/ipa/libipa/exposure_mode_helper.h   |  2 +-
>>>    src/ipa/rkisp1/algorithms/agc.cpp       |  9 ++---
>>>    6 files changed, 44 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>>> index 39d0aebb0838..da045640d569 100644
>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>> @@ -222,8 +222,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>>        utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>>>    
>>>        utils::Duration newExposureTime;
>>> -     double aGain, dGain;
>>> -     std::tie(newExposureTime, aGain, dGain) =
>>> +     double aGain, qGain, dGain;
>>> +     std::tie(newExposureTime, aGain, qGain, dGain) =
>>>                calculateNewEv(context.activeState.agc.constraintMode,
>>>                               context.activeState.agc.exposureMode, hist,
>>>                               effectiveExposureValue);
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>>> index 58c4dfc9add2..695453e7ceea 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>>> @@ -566,11 +566,12 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
>>>     *
>>>     * Calculate a new exposure value to try to obtain the target. The calculated
>>>     * exposure value is filtered to prevent rapid changes from frame to frame, and
>>> - * divided into exposure time, analogue and digital gain.
>>> + * divided into exposure time, analogue, quantization and digital gain.
>>>     *
>>> - * \return Tuple of exposure time, analogue gain, and digital gain
>>> + * \return Tuple of exposure time, analogue gain, quantization gain and digital
>>> + * gain
>>>     */
>>> -std::tuple<utils::Duration, double, double>
>>> +std::tuple<utils::Duration, double, double, double>
>>>    AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>>>                                 uint32_t exposureModeIndex,
>>>                                 const Histogram &yHist,
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
>>> index a7c7e006af42..741ccf986625 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.h
>>> +++ b/src/ipa/libipa/agc_mean_luminance.h
>>> @@ -68,7 +68,7 @@ public:
>>>                return controls_;
>>>        }
>>>    
>>> -     std::tuple<utils::Duration, double, double>
>>> +     std::tuple<utils::Duration, double, double, double>
>>>        calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
>>>                       const Histogram &yHist, utils::Duration effectiveExposureValue);
>>>    
>>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
>>> index 7d470ebe487c..79b8e6758faf 100644
>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp
>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
>>> @@ -178,14 +178,23 @@ double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons
>>>     * required exposure, the helper falls-back to simply maximising the exposure
>>>     * time first, followed by analogue gain, followed by digital gain.
>>>     *
>>> - * \return Tuple of exposure time, analogue gain, and digital gain
>>> + * During the calculations the gain missed due to quantization is recorded and
>>> + * returned as quantization gain. The quantization gain is not included in the
>>> + * digital gain. So to exactly apply the given exposure, both quantization gain
>>> + * and digital gain must be applied.
>>> + *
>>> + * \return Tuple of exposure time, analogue gain, quantization gain and digital
>>> + * gain
>>>     */
>>> -std::tuple<utils::Duration, double, double>
>>> +std::tuple<utils::Duration, double, double, double>
>>>    ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>    {
>>>        ASSERT(maxExposureTime_);
>>>        ASSERT(maxGain_);
>>>    
>>> +     utils::Duration exposureTime;
>>> +     double gain;
>>> +     double quantGain;
>>>        bool gainFixed = minGain_ == maxGain_;
>>>        bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;
>>>    
>>> @@ -193,16 +202,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>         * There's no point entering the loop if we cannot change either gain
>>>         * nor exposure time anyway.
>>>         */
>>> -     if (exposureTimeFixed && gainFixed)
>>> -             return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };
>>> +     if (exposureTimeFixed && gainFixed) {
>>> +             exposureTime = clampExposureTime(minExposureTime_, &quantGain);
>>> +             gain = clampGain(minGain_ * quantGain, &quantGain);
>>
>> So this rolls the gain lost to quantization of the exposure time into
>> analogue gain - why go for that automatically, but not also roll the
>> gain lost to quantization of the analogue gain into digital gain
>> automatically?
> 
> In this case exposureTime and gain are fixed, so clamp will always
> return the same result. So (I hope I didn't miss something) passing the
> gain through clamp is the same as saying:
> 
> double quantGain2;
> exposureTime = clampExposureTime(minExposureTime_, &quantGain2);
> gain = clampGain(minGain_ * quantGain, &quantGain);
> quantGain *= quantGain2;

Yes I think so, but the same pattern is in the calculations within the 
loop too where the exposure time and gain aren't fixed. Let me move this 
conversation down there so it's in the right context...

> 
>>
>>> +
>>> +             return { exposureTime, gain, quantGain,
>>> +                      exposure / (minExposureTime_ * minGain_ * quantGain) };
>>
>> I am surprised to see quantGain here...for example with the following
>> values:
>>
>>
>> exposure = 10000us
>> minGain = 2.0
>> minExposureTime_ = 4000us
>> lineLength_ = 30us
>> Say 16 steps of analogue gain from 1x to 16x
>>
>> Then
>>
>> following clampExposureTime exposureTime = 3990us, quantGain = 1.00250626566
>> following clampGain() gain = 2.0 and quantGain = 1.00250626566
>>
>> So I would expect digital gain to be calculated as 1.25 and for the
>> algorithm to have to apply both dGain and qGain as digital gain such
>> that 3990 * 2.0 * 1.00250626566 * 1.25 = 10000...is that not the intention?
> 
> Ohh, thanks, that is a good point. My conclusion is a bit different
> though. I think the return should look like this:
> 
> return { exposureTime, gain, quantGain,
>           exposure / (exposureTime * gain * quantGain) };
> 
> So that the digital gain is based on the values that are actually
> applied. Then the overall result is as you'd expect.

That also looks fine to me

> 
>>
>>
>>> +     }
>>>    
>>> -     utils::Duration exposureTime;
>>>        double stageGain = clampGain(1.0);
>>>        double lastStageGain = stageGain;
>>> -     double gain;
>>>    
>>>        for (unsigned int stage = 0; stage < gains_.size(); stage++) {
>>> -             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);
>>> +             utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
>>> +                                                                   &quantGain);
>>>                stageGain = clampGain(gains_[stage]);
>>>    
>>>                /*
>>> @@ -215,18 +228,20 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>    
>>>                /* Clamp the gain to lastStageGain and regulate exposureTime. */
>>>                if (stageExposureTime * lastStageGain >= exposure) {
>>> -                     exposureTime = clampExposureTime(exposure / lastStageGain);
>>> -                     gain = clampGain(exposure / exposureTime);
>>> +                     exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);
>>> +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>>>    

If we have things set like:

exposure = 14380us
lineLength_ = 30us
Imx477 gain model: AnalogueGainLinear{ 0, 1024, -1, 1024 };
exposure steps = 1000, 2000, 4000, 8000, 16000
gain steps = 2.0, 3.8, 5.6, 7.4, 9.2

(settings chosen to exacerbate problems which probably wouldn't appear 
all that often...)

Then after two iterations of the loop we run clampExposureTime(14380 / 
3.8, &guantGain) and get

exposureTime = 3780us
quantGain = 1.00111389585

And then with the implementation as-is we run clampGain(3.80847032337, 
&quantGain);

If the sensor were capable of perfectly granular exposure and gain 
control we'd be setting 3784us and 3.8x, which means the exposure value 
lost to quantisation of the exposure time has been added back as 
analogue gain. I don't have a problem with that, but it seems at odds 
with the not automatically adding gain lost to quantisation of analogue 
gain into the digital gain - why do one but not the other? I suppose 
particularly within this loop, since it shouldn't ever resort to digital 
gain to fulfil the requested exposure value under conditions of perfect 
granularity.

Now that I put numbers into the formula though I wonder if that needs 
looking at...in the current implementation I think we would return here...

exposureTime = 3780us
aGain = 3.80669
qGain = 1.00046768278
dGain = 0.998887343535

And whilst that gets the right value it means that qGain * dGain = 
0.999354505945 which is probably something we want to avoid. I start to 
lean towards the view that it's simplest to calculate the gain needed to 
correct the quantisation of exposure and analogue gain separately and 
combine them, without letting any correction for exposure quantisation 
go into analogue gain:

exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain1);
gain = clampGain(exposure / (exposureTime * quantGain), &quantGain2);
quantGain = quantGain1 * quantGain2;

return { exposureTime, gain, quantGain, exposure / (exposureTime * gain 
* quantGain) };

Then we get:

exposureTime = 3780, quantGain1 = 1.00111389585
aGain = 3.79259, quantGain2 = 1.00195380993
qGain = 1.00306988212
dGain = 1.0

Like I said, this helper hurts my brain...what do you think?

Dan

>>> -                     return { exposureTime, gain, exposure / (exposureTime * gain) };
>>> +                     return { exposureTime, gain, quantGain,
>>> +                              exposure / (exposureTime * gain * quantGain) };
>>>                }
>>>    
>>>                /* Clamp the exposureTime to stageExposureTime and regulate gain. */
>>>                if (stageExposureTime * stageGain >= exposure) {
>>>                        exposureTime = stageExposureTime;
>>> -                     gain = clampGain(exposure / exposureTime);
>>> +                     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>>>    
>>> -                     return { exposureTime, gain, exposure / (exposureTime * gain) };
>>> +                     return { exposureTime, gain, quantGain,
>>> +                              exposure / (exposureTime * gain * quantGain) };
>>>                }
>>>    
>>>                lastStageGain = stageGain;
>>> @@ -239,10 +254,11 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>         * stages to use then the default stageGain of 1.0 is used so that
>>>         * exposure time is maxed before gain is touched at all.
>>>         */
>>> -     exposureTime = clampExposureTime(exposure / stageGain);
>>> -     gain = clampGain(exposure / exposureTime);
>>> +     exposureTime = clampExposureTime(exposure / stageGain, &quantGain);
>>> +     gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
>>>    
>>> -     return { exposureTime, gain, exposure / (exposureTime * gain) };
>>> +     return { exposureTime, gain, quantGain,
>>> +              exposure / (exposureTime * gain * quantGain) };
>>>    }
>>>    
>>>    /**
>>> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
>>> index 8701fae9a26e..cc811e9fde18 100644
>>> --- a/src/ipa/libipa/exposure_mode_helper.h
>>> +++ b/src/ipa/libipa/exposure_mode_helper.h
>>> @@ -30,7 +30,7 @@ public:
>>>        void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>>>                       double minGain, double maxGain);
>>>    
>>> -     std::tuple<utils::Duration, double, double>
>>> +     std::tuple<utils::Duration, double, double, double>
>>>        splitExposure(utils::Duration exposure) const;
>>>    
>>>        utils::Duration minExposureTime() const { return minExposureTime_; }
>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>>> index 35440b67e999..0a29326841fb 100644
>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>>> @@ -567,15 +567,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>>        setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
>>>    
>>>        utils::Duration newExposureTime;
>>> -     double aGain, dGain;
>>> -     std::tie(newExposureTime, aGain, dGain) =
>>> +     double aGain, qGain, dGain;
>>> +     std::tie(newExposureTime, aGain, qGain, dGain) =
>>
>> I think you'll also have to fix this for the other users of
>> AgcMeanLuminance; IPU3 and C55 at least.
> 
> Yes, CI told me that also :-)
> 
>>
>> Thanks
>> Dan
> 
> Thanks,
> Stefan
> 
>>
>>>                calculateNewEv(frameContext.agc.constraintMode,
>>>                               frameContext.agc.exposureMode,
>>>                               hist, effectiveExposureValue);
>>>    
>>>        LOG(RkISP1Agc, Debug)
>>> -             << "Divided up exposure time, analogue gain and digital gain are "
>>> -             << newExposureTime << ", " << aGain << " and " << dGain;
>>> +             << "Divided up exposure time, analogue gain, quantization gain"
>>> +             << " and digital gain are " << newExposureTime << ", " << aGain
>>> +             << ", " << qGain << " and " << dGain;
>>>    
>>>        IPAActiveState &activeState = context.activeState;
>>>        /* Update the estimated exposure and gain. */
>>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 39d0aebb0838..da045640d569 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -222,8 +222,8 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
 
 	utils::Duration newExposureTime;
-	double aGain, dGain;
-	std::tie(newExposureTime, aGain, dGain) =
+	double aGain, qGain, dGain;
+	std::tie(newExposureTime, aGain, qGain, dGain) =
 		calculateNewEv(context.activeState.agc.constraintMode,
 			       context.activeState.agc.exposureMode, hist,
 			       effectiveExposureValue);
diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 58c4dfc9add2..695453e7ceea 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -566,11 +566,12 @@  utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)
  *
  * Calculate a new exposure value to try to obtain the target. The calculated
  * exposure value is filtered to prevent rapid changes from frame to frame, and
- * divided into exposure time, analogue and digital gain.
+ * divided into exposure time, analogue, quantization and digital gain.
  *
- * \return Tuple of exposure time, analogue gain, and digital gain
+ * \return Tuple of exposure time, analogue gain, quantization gain and digital
+ * gain
  */
-std::tuple<utils::Duration, double, double>
+std::tuple<utils::Duration, double, double, double>
 AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
 				 uint32_t exposureModeIndex,
 				 const Histogram &yHist,
diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
index a7c7e006af42..741ccf986625 100644
--- a/src/ipa/libipa/agc_mean_luminance.h
+++ b/src/ipa/libipa/agc_mean_luminance.h
@@ -68,7 +68,7 @@  public:
 		return controls_;
 	}
 
-	std::tuple<utils::Duration, double, double>
+	std::tuple<utils::Duration, double, double, double>
 	calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,
 		       const Histogram &yHist, utils::Duration effectiveExposureValue);
 
diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
index 7d470ebe487c..79b8e6758faf 100644
--- a/src/ipa/libipa/exposure_mode_helper.cpp
+++ b/src/ipa/libipa/exposure_mode_helper.cpp
@@ -178,14 +178,23 @@  double ExposureModeHelper::clampGain(double gain, double *quantizationGain) cons
  * required exposure, the helper falls-back to simply maximising the exposure
  * time first, followed by analogue gain, followed by digital gain.
  *
- * \return Tuple of exposure time, analogue gain, and digital gain
+ * During the calculations the gain missed due to quantization is recorded and
+ * returned as quantization gain. The quantization gain is not included in the
+ * digital gain. So to exactly apply the given exposure, both quantization gain
+ * and digital gain must be applied.
+ *
+ * \return Tuple of exposure time, analogue gain, quantization gain and digital
+ * gain
  */
-std::tuple<utils::Duration, double, double>
+std::tuple<utils::Duration, double, double, double>
 ExposureModeHelper::splitExposure(utils::Duration exposure) const
 {
 	ASSERT(maxExposureTime_);
 	ASSERT(maxGain_);
 
+	utils::Duration exposureTime;
+	double gain;
+	double quantGain;
 	bool gainFixed = minGain_ == maxGain_;
 	bool exposureTimeFixed = minExposureTime_ == maxExposureTime_;
 
@@ -193,16 +202,20 @@  ExposureModeHelper::splitExposure(utils::Duration exposure) const
 	 * There's no point entering the loop if we cannot change either gain
 	 * nor exposure time anyway.
 	 */
-	if (exposureTimeFixed && gainFixed)
-		return { minExposureTime_, minGain_, exposure / (minExposureTime_ * minGain_) };
+	if (exposureTimeFixed && gainFixed) {
+		exposureTime = clampExposureTime(minExposureTime_, &quantGain);
+		gain = clampGain(minGain_ * quantGain, &quantGain);
+
+		return { exposureTime, gain, quantGain,
+			 exposure / (minExposureTime_ * minGain_ * quantGain) };
+	}
 
-	utils::Duration exposureTime;
 	double stageGain = clampGain(1.0);
 	double lastStageGain = stageGain;
-	double gain;
 
 	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
-		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage]);
+		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
+								      &quantGain);
 		stageGain = clampGain(gains_[stage]);
 
 		/*
@@ -215,18 +228,20 @@  ExposureModeHelper::splitExposure(utils::Duration exposure) const
 
 		/* Clamp the gain to lastStageGain and regulate exposureTime. */
 		if (stageExposureTime * lastStageGain >= exposure) {
-			exposureTime = clampExposureTime(exposure / lastStageGain);
-			gain = clampGain(exposure / exposureTime);
+			exposureTime = clampExposureTime(exposure / lastStageGain, &quantGain);
+			gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
 
-			return { exposureTime, gain, exposure / (exposureTime * gain) };
+			return { exposureTime, gain, quantGain,
+				 exposure / (exposureTime * gain * quantGain) };
 		}
 
 		/* Clamp the exposureTime to stageExposureTime and regulate gain. */
 		if (stageExposureTime * stageGain >= exposure) {
 			exposureTime = stageExposureTime;
-			gain = clampGain(exposure / exposureTime);
+			gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
 
-			return { exposureTime, gain, exposure / (exposureTime * gain) };
+			return { exposureTime, gain, quantGain,
+				 exposure / (exposureTime * gain * quantGain) };
 		}
 
 		lastStageGain = stageGain;
@@ -239,10 +254,11 @@  ExposureModeHelper::splitExposure(utils::Duration exposure) const
 	 * stages to use then the default stageGain of 1.0 is used so that
 	 * exposure time is maxed before gain is touched at all.
 	 */
-	exposureTime = clampExposureTime(exposure / stageGain);
-	gain = clampGain(exposure / exposureTime);
+	exposureTime = clampExposureTime(exposure / stageGain, &quantGain);
+	gain = clampGain((exposure / exposureTime) * quantGain, &quantGain);
 
-	return { exposureTime, gain, exposure / (exposureTime * gain) };
+	return { exposureTime, gain, quantGain,
+		 exposure / (exposureTime * gain * quantGain) };
 }
 
 /**
diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
index 8701fae9a26e..cc811e9fde18 100644
--- a/src/ipa/libipa/exposure_mode_helper.h
+++ b/src/ipa/libipa/exposure_mode_helper.h
@@ -30,7 +30,7 @@  public:
 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
 		       double minGain, double maxGain);
 
-	std::tuple<utils::Duration, double, double>
+	std::tuple<utils::Duration, double, double, double>
 	splitExposure(utils::Duration exposure) const;
 
 	utils::Duration minExposureTime() const { return minExposureTime_; }
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 35440b67e999..0a29326841fb 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -567,15 +567,16 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
 
 	utils::Duration newExposureTime;
-	double aGain, dGain;
-	std::tie(newExposureTime, aGain, dGain) =
+	double aGain, qGain, dGain;
+	std::tie(newExposureTime, aGain, qGain, dGain) =
 		calculateNewEv(frameContext.agc.constraintMode,
 			       frameContext.agc.exposureMode,
 			       hist, effectiveExposureValue);
 
 	LOG(RkISP1Agc, Debug)
-		<< "Divided up exposure time, analogue gain and digital gain are "
-		<< newExposureTime << ", " << aGain << " and " << dGain;
+		<< "Divided up exposure time, analogue gain, quantization gain"
+		<< " and digital gain are " << newExposureTime << ", " << aGain
+		<< ", " << qGain << " and " << dGain;
 
 	IPAActiveState &activeState = context.activeState;
 	/* Update the estimated exposure and gain. */