[libcamera-devel] [PATCH v3 12/14] ipa: ipu3: agc: Refactor condition on exposure correction

[Laurent Pinchart]

Hi Jean-Michel,

On Fri, Oct 22, 2021 at 07:56:50AM +0200, Jean-Michel Hautbois wrote:
> On 22/10/2021 06:27, Laurent Pinchart wrote:
> > On Thu, Oct 21, 2021 at 06:43:59PM +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.
> >>
> >> Keep the indentation level removal and simply return when the change is
> >> so small there is no need to apply the newly calculated value.
> > 
> > The first two paragraph seems unrelated to the version of this patch,
> > and the "keep" in the third paragraph refers to how this has changed
> > compared to v2. Please rewrite the commit message to correspond to the
> > code changes.
> 
> Let's try:
> '''
> Each time we test the brightness with the desired the target, we verify
> if the change is small enough to consider that we are correctly exposed
> or not. Simplify the reading by removing one level of indentation, and
> returning from the function when the change is too small.
> '''

How is the first sentence even relevant ? If you want to lower
indentation by returning early, just say so, there's no need to explain
what the code does.

> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp | 91 ++++++++++++++++-----------------
> >>   1 file changed, 45 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index 43ef89df..7ff3029b 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -138,60 +138,59 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >>   	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
> >>   		return;
> >>   
> >> -	/* Are we correctly exposed ? */

I haven't noticed before, the comment could be kept.

> >> -	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> >> -		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;

And is there a particular reason to remove the log message ?

Please do *not* hide changes in a commit whose log message only mentions
indentation changes.

> >> -	} else {
> >> -		double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> >> +	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1)
> >> +		return;
> >>   
> >> -		/* 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;
> >> +	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> >>   
> >> -		currentExposure_ = prevExposureValue_ * evGain;
> >> -		utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> >> -		utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> >> +	/* 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;
> >>   
> >> -		utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> >> -		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >> -		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> >> -				    << ", maximum is " << maxTotalExposure;
> >> +	currentExposure_ = prevExposureValue_ * evGain;
> >> +	utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> >> +	utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> >>   
> >> -		/* \todo: estimate if we need to desaturate */
> >> -		filterExposure();
> >> +	utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> >> +	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >> +	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> >> +			    << ", maximum is " << maxTotalExposure;
> >>   
> >> -		utils::Duration exposureValue = filteredExposure_;
> >> -		utils::Duration shutterTime = minShutterSpeed;
> >> +	/* \todo: estimate if we need to desaturate */
> >> +	filterExposure();
> >>   
> >> -		/*
> >> -		 * 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;
> >> +	utils::Duration exposureValue = filteredExposure_;
> >> +	utils::Duration shutterTime = minShutterSpeed;
> >>   
> >> -		exposure = shutterTime / lineDuration_;
> >> -		analogueGain = stepGain;
> >> +	/*
> >> +	* 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.
> >> +	 *
> >> +	 * \todo Obtain the values of the exposure time and analog gain
> >> +	 * that were actually used by the sensor, either from embedded
> >> +	 * data when available, or from the delayed controls
> >> +	 * infrastructure in case a slow down caused a mismatch.
> >> +	 */
> >> +	prevExposureValue_ = shutterTime * analogueGain;
> >>   
> >> -		/*
> >> -		 * Update the exposure value for the next process call.
> >> -		 *
> >> -		 * \todo Obtain the values of the exposure time and analog gain
> >> -		 * that were actually used by the sensor, either from embedded
> >> -		 * data when available, or from the delayed controls
> >> -		 * infrastructure in case a slow down caused a mismatch.
> >> -		 */
> >> -		prevExposureValue_ = shutterTime * analogueGain;
> >> -	}
> >>   	lastFrame_ = frameCount_;
> > 
> > This is a functional change, with no mention of it (and no explanation
> > of why it's correct) in the commit message.
> 
> Mmmh, if is "just" about removing one else and a LOG() message... I 'm 
> failing to see the functional change here...

Before this patch, lastFrame_ was set for both branches. With this
patch, as you return early, lastFrame_ doesn't get set when the change
is small.

> >>   }
> >>   

-- 
Regards,

Laurent Pinchart