[libcamera-devel] [PATCH v2 08/13] ipa: ipu3: agc: Simplify division of exposure/gain

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 20, 2021 at 05:46:02PM +0200, Jean-Michel Hautbois wrote:
> Until now, the algorithm makes complex assumptions when dividing the
> exposure and analogue gain values. Instead, use a simpler clamping of
> the shutter speed first, and then of the analogue gain, based on the
> limits configured.
> 
> While at it, correct a bug which makes the algorithm not use the
> filtered result from filterExposure().

This is still two changes in the same patch, I would have split it in
two.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 36 +++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 1bae1eb9..84e41834 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -146,7 +146,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  				    << " Gain " << gain;
>  
>  		currentExposure_ = currentExposureNoDg_ * newGain;
> +		utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>  		utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> +
>  		utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
>  		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>  		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> @@ -155,23 +157,23 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  		/* \todo: estimate if we need to desaturate */
>  		filterExposure();
>  
> -		utils::Duration newExposure = 0.0s;
> -		if (currentShutter < maxShutterSpeed) {
> -			exposure = std::clamp<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_,
> -							minExposureLines_,
> -							maxExposureLines_);
> -			newExposure = currentExposure_ / exposure;
> -			gain = std::clamp(gain * currentExposure_ / newExposure,
> -					  kMinGain, kMaxGain);
> -		} else if (currentShutter >= maxShutterSpeed) {
> -			gain = std::clamp(gain * currentExposure_ / currentExposureNoDg_,
> -					  kMinGain, kMaxGain);
> -			newExposure = currentExposure_ / gain;
> -			exposure = std::clamp<uint32_t>(exposure * currentExposure_ / newExposure,
> -							minExposureLines_,
> -							maxExposureLines_);
> -		}
> -		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> +		utils::Duration exposureValue = filteredExposure_;
> +		utils::Duration shutterTime = minShutterSpeed;
> +
> +		/*
> +		 * Push the shutter time up to the maximum first, and only then
> +		 * increase the gain.
> +		 */
> +		shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> +							  minShutterSpeed, maxShutterSpeed);
> +		double stepGain = std::clamp(exposureValue / shutterTime,
> +					     kMinGain, kMaxGain);
> +		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +				    << shutterTime << " and "
> +				    << stepGain;
> +
> +		exposure = shutterTime / lineDuration_;
> +		gain = stepGain;
>  	}
>  	lastFrame_ = frameCount_;
>  }

-- 
Regards,

Laurent Pinchart