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

[Umang Jain]

Hi JM,

Thank you for the patch.

On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:
> 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.
>
> As the exposure value is updated in the beginning of the computation,
> there is no need to initialize effectiveExposureValue anymore in the
> configure call, and it can be a local variable and not a class variable
> anymore.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
> ---
> v3: make effectiveExposureValue a local variable
> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++-----------------
>   src/ipa/ipu3/algorithms/agc.h   |  1 -
>   2 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index a34f070e..d736f21c 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -80,8 +80,7 @@ static constexpr double kNormalizedLumaTarget = 0.16;
>   
>   Agc::Agc()
>   	: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),
> -	  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
> -	  prevExposureValue_(0s)
> +	  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s)
>   {
>   }
>   
> @@ -112,10 +111,6 @@ 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_;
> -
>   	return 0;
>   }
>   
> @@ -222,6 +217,13 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   
>   	/* Calculate the shutter time in seconds */
>   	utils::Duration currentShutter = exposure * lineDuration_;
> +
> +	/*
> +	 * Update the exposure value for the next computation using the values
> +	 * of exposure and gain really used by the sensor.
> +	 */
> +	utils::Duration effectiveExposureValue = currentShutter * analogueGain;
> +
>   	LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>   			    << " Shutter speed " << currentShutter
>   			    << " Gain " << analogueGain
> @@ -239,7 +241,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   	 * 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_;
>   
> @@ -271,16 +273,6 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   	/* Update the estimated exposure and gain. */
>   	frameContext.agc.exposure = shutterTime / lineDuration_;
>   	frameContext.agc.gain = 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 79736283..1d085b40 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -54,7 +54,6 @@ private:
>   
>   	utils::Duration filteredExposure_;
>   	utils::Duration currentExposure_;
> -	utils::Duration prevExposureValue_;
>   
>   	uint32_t stride_;
>   };