[libcamera-devel] [PATCH 08/22] ipa: ipu3: agc: Update previous exposure value

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-08 13:13:36)
> Previous exposure value is calculated based on the estimated shutter

Previously ?

if so
Previously, the exposure value was calculated based...

> time and gain applied. Now that we have the real values for the current
> frame, use those before estimating the next one.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index e4048d40..475e715f 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -193,15 +193,12 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
>         /* Estimate the gain needed to have the proportion wanted */
>         double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> -       if (std::abs(evGain - 1.0) < 0.01) {

Hrm ... we'd not long ago added this, but I guess it's fine as the
development path.


> -               LOG(IPU3Agc, Debug) << "We are well exposed (iqMean = "
> -                                   << iqMean_ << ")";
> -               return;
> -       }
> -
>         /* extracted from Rpi::Agc::computeTargetExposure */
>         /* Calculate the shutter time in seconds */
>         utils::Duration currentShutter = exposure * lineDuration_;

New line here please, my eyes really struggle to separate or group content when
it's condensed as

	/* line comment like the code */
	code as long as the comment;
	/* line comment also here */
	more code batched together;

The code blurs into the comments too easily, and it all looks like one
block.


> +       /* Update the exposure value for the next computation. */
> +       prevExposureValue_ = currentShutter * analogueGain;
> +

Is there a reason this moves earlier in the function? Does it get used
before the end? If so - it's not the 'previousExposureValue' is it?

Perhaps I haven't understood the meaning of the specific variables. It's
just hard to see how we are in a current frame, calculating the
'previous' exposure... just before we use it.

>         LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>                             << " Shutter speed " << currentShutter
>                             << " Gain " << analogueGain
> @@ -250,16 +247,6 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
>  
>         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;
>  }
>  
>  /**
> -- 
> 2.32.0
>