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

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-10-13 16:41:23)
> 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.

Oh excellent ... looks like here comes the refactor I was half
suggesting in the previous patch...

It's hard to see if there are any specific changes in here, and I assume
it's just the ultimate removal of the
 "if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {"

and associated re-indentation.

If so:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

I'd be tempted to move that lastFrame_ = frameCount_ up to just after it
gets used by the opening checks, as it's not used by any of the code
following.

Unless perhaps it's better at the end to signify it's only updated after
the calculations are completed. Either way, it's fine really.

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