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

[Jean-Michel Hautbois]

Hi Laurent,

On 21/10/2021 04:35, Laurent Pinchart wrote:
> 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.

Yes, that's why I changed the commit message a bit. You may have 
oscillations, but those would be very small, and given the sensitivity 
of the sensors, you may not even notice it (we have a low-pass filter).

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

Yes, we need to do that, certainly based on DelayedControls, as we don't 
have embedded data in all sensors. I am not sure on how to do it 
efficiently, so any input will be appreciated ;-).

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