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

[Jean-Michel Hautbois]

On 10/11/2021 23:33, Kieran Bingham wrote:
> 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?

It is not :-/

> 
>> -
>>          /* 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
>>