[libcamera-devel] [PATCH 3/4] ipa: ipu3: Remove local cached limits for shutter speed and gain

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Thu, Nov 25, 2021 at 11:21:42AM +0100, Jean-Michel Hautbois wrote:
> The limits for shutter speed and analogue gain are stored locally while
> those are only used in computeExposure(). Remove those local variables,
> and use the IPASessionConfiguration values directly.
> 
> While at it, set default analogue gain and shutter speed.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 42 ++++++++++++++++++---------------
>  src/ipa/ipu3/algorithms/agc.h   |  8 +------
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 2945a138..b822c79b 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -70,8 +70,7 @@ static constexpr uint32_t kNumStartupFrames = 10;
>  static constexpr double kRelativeLuminanceTarget = 0.16;
>  
>  Agc::Agc()
> -	: frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),
> -	  maxShutterSpeed_(0s), filteredExposure_(0s)
> +	: frameCount_(0), lineDuration_(0s), filteredExposure_(0s)
>  {
>  }
>  
> @@ -90,16 +89,10 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
>  
> -	minShutterSpeed_ = context.configuration.agc.minShutterSpeed;
> -	maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed,
> -				    kMaxShutterSpeed);
> -
> -	minAnalogueGain_ = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> -	maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
> -
>  	/* Configure the default exposure and gain. */
> -	context.frameContext.agc.gain = minAnalogueGain_;
> -	context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;
> +	context.frameContext.agc.gain =
> +		std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> +	context.frameContext.agc.exposure = 10ms / lineDuration_;

This is a functional change that should be explained in the commit
message.

>  
>  	return 0;
>  }
> @@ -174,17 +167,28 @@ utils::Duration Agc::filterExposure(utils::Duration currentExposure)
>  
>  /**
>   * \brief Estimate the new exposure and gain values
> - * \param[inout] frameContext The shared IPA frame Context
> + * \param[inout] context The shared IPA Context
>   * \param[in] yGain The gain calculated based on the relative luminance target
>   * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>   */
> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
> -			  double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
>  {
> +	IPASessionConfiguration &configuration = context.configuration;

Make it const.

> +	IPAFrameContext &frameContext = context.frameContext;
> +
>  	/* Get the effective exposure and gain applied on the sensor. */
>  	uint32_t exposure = frameContext.sensor.exposure;
>  	double analogueGain = frameContext.sensor.gain;
>  
> +	utils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;
> +	utils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,
> +						   kMaxShutterSpeed);
> +
> +	double minAnalogueGain = std::max(configuration.agc.minAnalogueGain,
> +					  kMinAnalogueGain);
> +	double maxAnalogueGain = std::min(configuration.agc.maxAnalogueGain,
> +					  kMaxAnalogueGain);

As those values are constant for a session, caching them in the Agc
class avoids recomputing them for every frame. Is there a particular
reason why you think we shouldn't cache them ?

> +
>  	/* Use the highest of the two gain estimates. */
>  	double evGain = std::max(yGain, iqMeanGain);
>  
> @@ -216,7 +220,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>  	utils::Duration currentExposure = effectiveExposureValue * evGain;
>  
>  	/* Clamp the exposure value to the min and max authorized */
> -	utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;
> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain;
>  	currentExposure = std::min(currentExposure, maxTotalExposure);
>  	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure
>  			    << ", maximum is " << maxTotalExposure;
> @@ -232,10 +236,10 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>  	* Push the shutter time up to the maximum first, and only then
>  	* increase the gain.
>  	*/
> -	shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
> -						  minShutterSpeed_, maxShutterSpeed_);
> +	shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,
> +						  minShutterSpeed, maxShutterSpeed);
>  	double stepGain = std::clamp(exposureValue / shutterTime,
> -				     minAnalogueGain_, maxAnalogueGain_);
> +				     minAnalogueGain, maxAnalogueGain);
>  	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>  			    << shutterTime << " and "
>  			    << stepGain;
> @@ -348,7 +352,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  			break;
>  	}
>  
> -	computeExposure(context.frameContext, yGain, iqMeanGain);
> +	computeExposure(context, yGain, iqMeanGain);
>  	frameCount_++;
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 84bfe045..d9f17e6f 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -34,8 +34,7 @@ private:
>  	double measureBrightness(const ipu3_uapi_stats_3a *stats,
>  				 const ipu3_uapi_grid_config &grid) const;
>  	utils::Duration filterExposure(utils::Duration currentExposure);
> -	void computeExposure(IPAFrameContext &frameContext, double yGain,
> -			     double iqMeanGain);
> +	void computeExposure(IPAContext &context, double yGain, double iqMeanGain);
>  	double estimateLuminance(IPAFrameContext &frameContext,
>  				 const ipu3_uapi_grid_config &grid,
>  				 const ipu3_uapi_stats_3a *stats,
> @@ -44,11 +43,6 @@ private:
>  	uint64_t frameCount_;
>  
>  	utils::Duration lineDuration_;
> -	utils::Duration minShutterSpeed_;
> -	utils::Duration maxShutterSpeed_;
> -
> -	double minAnalogueGain_;
> -	double maxAnalogueGain_;
>  
>  	utils::Duration filteredExposure_;
>  

-- 
Regards,

Laurent Pinchart