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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 20, 2021 at 05:46:05PM +0200, Jean-Michel Hautbois wrote:
> Until now, we can't know when the exposure and gains applied in the
> IPAIPU3::setControls() are really applied (it can be several frames). We
> don't want to use the values calculated as if they are already applied,
> and this is done by testing the frame number.
> 
> 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 the exposure
> and analogue gain my evolve a bit but it should not be visible to the
> user.

Repeating my comment from v1,

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

I'd rather work on dropping the kFrameSkipCount and using the correct
exposure time and gain values.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 78 ++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 41 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 19f3a420..0417dc99 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -138,53 +138,49 @@ 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_;
> +	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> -		/* extracted from Rpi::Agc::computeTargetExposure */
> -		utils::Duration currentShutter = exposure * lineDuration_;
> -		currentExposureNoDg_ = currentShutter * analogueGain;
> -		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> -				    << " Shutter speed " << currentShutter
> -				    << " Gain " << analogueGain
> -				    << " Needed ev gain " << evGain;
> +	/* extracted from Rpi::Agc::computeTargetExposure */
> +	utils::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;
> -		utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> -		utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> +	currentExposure_ = prevExposureValue_ * evGain;
> +	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_
> -				    << ", maximum is " << maxTotalExposure;
> +	utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> +	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> +	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> +			    << ", maximum is " << maxTotalExposure;
>  
> -		/* \todo: estimate if we need to desaturate */
> -		filterExposure();
> +	/* \todo: estimate if we need to desaturate */
> +	filterExposure();
>  
> -		utils::Duration exposureValue = filteredExposure_;
> -		utils::Duration shutterTime = minShutterSpeed;
> +	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_;
> +	analogueGain = stepGain;
> +
> +	/* Update the exposure value for the next process call */
> +	prevExposureValue_ = shutterTime * analogueGain;
>  
> -		/*
> -		 * 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_;
> -		analogueGain = stepGain;
> -
> -		/* Update the exposure value for the next process call */
> -		prevExposureValue_ = shutterTime * analogueGain;
> -	}
>  	lastFrame_ = frameCount_;
>  }
>  

-- 
Regards,

Laurent Pinchart