[libcamera-devel] [PATCH 11/13] ipa: ipu3: agc: Remove condition on exposure correction

[Laurent Pinchart]

Hi Jean-Michel,

(CC'ing David)

On Wed, Oct 13, 2021 at 05:41:23PM +0200, Jean-Michel Hautbois wrote:
> When the exposure is estimated, we verify if it changed enough for
> exposure and gain to be updated.
> 
> There is no need for that because we are now filtering the value with
> the previous one correctly, so if the change is very small is won't
> change the exposure and analogue gain.

I fail to see why a small change won't change the exposure and analogue
gain. As far as I understand (maybe David can confirm), this removes a
hysteresis, which I believe will cause oscillations.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 50 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 7efe0907..b922bcdf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -132,61 +132,57 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
>  		return;
>  
> -	/* Are we correctly exposed ? */
> -	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> -		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -	} else {
> -		double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> -
> -		/* extracted from Rpi::Agc::computeTargetExposure */
> -		Duration currentShutter = exposure * lineDuration_;
> -		currentExposureNoDg_ = currentShutter * analogueGain;
> -		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> -				    << " Shutter speed " << currentShutter
> -				    << " Gain " << analogueGain
> -				    << " Needed ev gain " << evGain;
> -
> -		currentExposure_ = prevExposureValue * evGain;
> -		Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> -		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> -		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> -				    << " maximum is " << maxTotalExposure;
> -
> -		/* \todo: estimate if we need to desaturate */
> -		filterExposure();
> -
> -		Duration exposureValue = filteredExposure_;
> -		Duration shutterTime = kMinShutterSpeed;
> -		double stepGain = kMinGain;
> -
> -		if (shutterTime * stepGain < exposureValue) {
> -			Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> -			if (maxShutterMinGain >= exposureValue) {
> -				LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> -				shutterTime = exposureValue / stepGain;
> -			} else {
> -				shutterTime = kMaxShutterSpeed;
> -			}
> +	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> +
> +	/* extracted from Rpi::Agc::computeTargetExposure */
> +	Duration currentShutter = exposure * lineDuration_;
> +	currentExposureNoDg_ = currentShutter * analogueGain;
> +	LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> +				<< " Shutter speed " << currentShutter
> +				<< " Gain " << analogueGain
> +				<< " Needed ev gain " << evGain;
> +
> +	currentExposure_ = prevExposureValue * evGain;
> +	Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> +	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> +	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> +				<< " maximum is " << maxTotalExposure;
> +
> +	/* \todo: estimate if we need to desaturate */
> +	filterExposure();
> +
> +	Duration exposureValue = filteredExposure_;
> +	Duration shutterTime = kMinShutterSpeed;
> +	double stepGain = kMinGain;
> +
> +	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;
> -			}
> +		Duration maxGainShutter = kMaxGain * shutterTime;
> +		if (maxGainShutter >= exposureValue) {
> +			stepGain = exposureValue / shutterTime;
> +			LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> +		} else {
> +			stepGain = kMaxGain;
>  		}
> +	}
>  
> -		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> -				<< shutterTime << " and "
> -				<< stepGain;
> +	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +			<< shutterTime << " and "
> +			<< stepGain;
>  
> -		exposure = shutterTime / lineDuration_;
> -		analogueGain = stepGain;
> +	exposure = shutterTime / lineDuration_;
> +	analogueGain = stepGain;
> +
> +	/* Update the exposure value for the next process call */
> +	prevExposureValue = shutterTime * analogueGain;
>  
> -		/* Update the exposure value for the next process call */
> -		prevExposureValue = shutterTime * analogueGain;
> -	}
>  	lastFrame_ = frameCount_;
>  }
>  

-- 
Regards,

Laurent Pinchart