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

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-10 19:58:55)
> Previously, the exposure value was calculated based on the estimated
> shutter time and gain applied. Now that we have the real values for the
> current frame, use those before estimating the next one and rename the
> variable accordingly.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 32 +++++++++++---------------------
>  src/ipa/ipu3/algorithms/agc.h   |  2 +-
>  2 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index ee37a9d5..38667e61 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -73,7 +73,7 @@ static constexpr uint32_t kMaxLuminance = 255;
>  Agc::Agc()
>         : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),
>           maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
> -         prevExposureValue_(0s)
> +         effectiveExposureValue_(0s)
>  {
>  }
>  
> @@ -104,9 +104,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>         context.frameContext.agc.gain = minAnalogueGain_;
>         context.frameContext.agc.exposure = minExposureLines_;
>  
> -       prevExposureValue_ = context.frameContext.agc.gain
> -                          * context.frameContext.agc.exposure
> -                          * lineDuration_;
> +       effectiveExposureValue_ = context.frameContext.agc.gain

Ahhh nice. 'Effective' sounds so much clearer than 'Previous' in this
context.

> +                               * context.frameContext.agc.exposure
> +                               * lineDuration_;
>  
>         return 0;
>  }
> @@ -201,16 +201,16 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
>          */
>         double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> -       if (std::abs(evGain - 1.0) < 0.01) {
> -               LOG(IPU3Agc, Debug) << "We are well exposed (iqMean = "
> -                                   << iqMean_ << ")";
> -               return;
> -       }

Is this intentional? I thought we only just added this in this series?

> -
>         /* extracted from Rpi::Agc::computeTargetExposure */
>  
>         /* Calculate the shutter time in seconds */
>         utils::Duration currentShutter = exposure * lineDuration_;

I'd have an empty line here.

> +       /*
> +        * Update the exposure value for the next computation using the values
> +        * of exposure and gain really used by the sensor.
> +        */
> +       effectiveExposureValue_ = currentShutter * analogueGain;
> +
>         LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>                             << " Shutter speed " << currentShutter
>                             << " Gain " << analogueGain
> @@ -228,7 +228,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
>          * Calculate the current exposure value for the scene as the latest
>          * exposure value applied multiplied by the new estimated gain.
>          */
> -       currentExposure_ = prevExposureValue_ * evGain;
> +       currentExposure_ = effectiveExposureValue_ * evGain;
>         utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>         utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>  
> @@ -259,16 +259,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;
>  }
>  
>  /**
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 0a9152a9..51174639 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -54,7 +54,7 @@ private:
>  
>         utils::Duration filteredExposure_;
>         utils::Duration currentExposure_;
> -       utils::Duration prevExposureValue_;
> +       utils::Duration effectiveExposureValue_;
>  
>         uint32_t stride_;
>  };
> -- 
> 2.32.0
>