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

14124 diff mbox series

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.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 37 ++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 10 deletions(-)

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@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_;
>  }