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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 13, 2021 at 05:41:20PM +0200, Jean-Michel Hautbois wrote:
> We are not using the filtered result from filterExposure() and we make
> complex assumptions when dividing the exposure and analogue gain values.
> 
> Use the same mechanism as in RPiController::Agc::divideUpExposure() with
> the fixed limits defined previously. This simplifies the understanding,
> and corrects a bug in the analogue gain calculus.

That's two fixes in one patch.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 37 ++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index e58a8a8d..3ec1c60c 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -153,17 +153,34 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  		/* \todo: estimate if we need to desaturate */
>  		filterExposure();
>  
> -		Duration newExposure = 0.0s;
> -		if (currentShutter < kMaxShutterSpeed) {
> -			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);
> -			newExposure = currentExposure_ / exposure;
> -			gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> -		} else if (currentShutter >= kMaxShutterSpeed) {
> -			gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> -			newExposure = currentExposure_ / gain;
> -			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);
> +		Duration exposureValue = filteredExposure_;
> +		Duration shutterTime = kMinShutterSpeed;
> +		double stepGain = kMinGain;

Where does "step" come from ?

> +
> +		if (shutterTime * stepGain < exposureValue) {
> +			Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> +			if (maxShutterMinGain >= exposureValue) {
> +				LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> +				shutterTime = exposureValue / stepGain;
> +			} else {
> +				shutterTime = kMaxShutterSpeed;
> +			}
> +
> +			Duration maxGainShutter = kMaxGain * shutterTime;
> +			if (maxGainShutter >= exposureValue) {
> +				stepGain = exposureValue / shutterTime;
> +				LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> +			} else {
> +				stepGain = kMaxGain;
> +			}
>  		}

If I understand the code correctly, this is essentially

		shutterTime = std::clamp(exposureValue / kMinGain,
					 kMinShutterSpeed, kMaxShutterSpeed);
		stepGain = std::clamp(exposureValue / shutterTime,
				      kMinGain, kMaxGain);

Isn't it easier to read ? You could even add a comment before:

		/*
		 * Push the shutter time up to the maximum first, and only then
		 * increase the gain.
		 */

The mechanism is fairly different from what is implemented in the RPi
IPA, as you don't take into account fixed exposure time or fixed gains,
or stages. I thus wouldn't indicate in the commit message that you're
doing the same as RPi.

> -		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> +
> +		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +				<< shutterTime << " and "
> +				<< stepGain;
> +
> +		exposure = shutterTime / lineDuration_;
> +		gain = stepGain;
>  	}
>  	lastFrame_ = frameCount_;
>  }

-- 
Regards,

Laurent Pinchart