[PATCH 1/3] ipa: ipu3: Refer to integration time instead of shutter speed

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Thu, Apr 18, 2024 at 01:46:30PM +0100, Daniel Scally wrote:
> Within the IPU3 IPA module there are multiple references to min and
> max shutter speeds. In calculation and usage however those variables
> reflect integration time rather than shutter speed - this difference
> in terminology is particularly problematic because the minimum
> shutter speed is equivalent to the maximum integration time.
> 
> Replace references to shutter speed with integration time.

I like this (I may be tempted to reply with U+2661), and I'd like the
change to be applied globally through libcamera. "Integration time" even
deserves a place in a glossary in my opinion.

> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 12 ++++++------
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>  src/ipa/ipu3/ipa_context.cpp    |  8 ++++----
>  src/ipa/ipu3/ipa_context.h      |  4 ++--
>  src/ipa/ipu3/ipu3.cpp           |  8 ++++----
>  5 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 46fc3b33..a59b73fb 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -51,7 +51,7 @@ LOG_DEFINE_CATEGORY(IPU3Agc)
>  static constexpr double kMinAnalogueGain = 1.0;
>  
>  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> -static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> +static constexpr utils::Duration kMaxIntegrationTime = 60ms;
>  
>  /* Histogram constants */
>  static constexpr uint32_t knumHistogramBins = 256;
> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10;
>  static constexpr double kRelativeLuminanceTarget = 0.16;
>  
>  Agc::Agc()
> -	: minShutterSpeed_(0s), maxShutterSpeed_(0s)
> +	: minIntegrationTime_(0s), maxIntegrationTime_(0s)
>  {
>  }
>  
> @@ -114,9 +114,9 @@ int Agc::configure(IPAContext &context,
>  	stride_ = configuration.grid.stride;
>  	bdsGrid_ = configuration.grid.bdsGrid;
>  
> -	minShutterSpeed_ = configuration.agc.minShutterSpeed;
> -	maxShutterSpeed_ = std::min(configuration.agc.maxShutterSpeed,
> -				    kMaxShutterSpeed);
> +	minIntegrationTime_ = configuration.agc.minIntegrationTime;
> +	maxIntegrationTime_ = std::min(configuration.agc.maxIntegrationTime,
> +				       kMaxIntegrationTime);
>  
>  	minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain);
>  	maxAnalogueGain_ = configuration.agc.maxAnalogueGain;
> @@ -129,7 +129,7 @@ int Agc::configure(IPAContext &context,
>  	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>  
>  	/* \todo Run this again when FrameDurationLimits is passed in */
> -	configureExposureModeHelpers(minShutterSpeed_, maxShutterSpeed_,
> +	configureExposureModeHelpers(minIntegrationTime_, maxIntegrationTime_,
>  				     minAnalogueGain_, maxAnalogueGain_);
>  
>  	resetFrameCount();
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 945d1846..631dea52 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -42,8 +42,8 @@ private:
>  	Histogram parseStatistics(const ipu3_uapi_stats_3a *stats,
>  				  const ipu3_uapi_grid_config &grid);
>  
> -	utils::Duration minShutterSpeed_;
> -	utils::Duration maxShutterSpeed_;
> +	utils::Duration minIntegrationTime_;
> +	utils::Duration maxIntegrationTime_;
>  
>  	double minAnalogueGain_;
>  	double maxAnalogueGain_;
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index c4fb5642..72f7cec9 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -92,11 +92,11 @@ namespace libcamera::ipa::ipu3 {
>   * \var IPASessionConfiguration::agc
>   * \brief AGC parameters configuration of the IPA
>   *
> - * \var IPASessionConfiguration::agc.minShutterSpeed
> - * \brief Minimum shutter speed supported with the configured sensor
> + * \var IPASessionConfiguration::agc.minIntegrationTime
> + * \brief Minimum integration time supported with the configured sensor
>   *
> - * \var IPASessionConfiguration::agc.maxShutterSpeed
> - * \brief Maximum shutter speed supported with the configured sensor
> + * \var IPASessionConfiguration::agc.maxIntegrationTime
> + * \brief Maximum integration time supported with the configured sensor
>   *
>   * \var IPASessionConfiguration::agc.minAnalogueGain
>   * \brief Minimum analogue gain supported with the configured sensor
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index a92cb6ce..6b1ffdd1 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -33,8 +33,8 @@ struct IPASessionConfiguration {
>  	} af;
>  
>  	struct {
> -		utils::Duration minShutterSpeed;
> -		utils::Duration maxShutterSpeed;
> +		utils::Duration minIntegrationTime;
> +		utils::Duration maxIntegrationTime;
>  		double minAnalogueGain;
>  		double maxAnalogueGain;
>  	} agc;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 4809eb60..f13cc394 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -217,13 +217,13 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
> -	 * to know the limits for shutter speed and analogue gain.
> +	 * to know the limits for integration time and analogue gain.
>  	 * As it depends on the sensor, update it with the controls.
>  	 *
> -	 * \todo take VBLANK into account for maximum shutter speed
> +	 * \todo take VBLANK into account for maximum integration time
>  	 */
> -	context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.agc.minIntegrationTime = minExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.agc.maxIntegrationTime = maxExposure * context_.configuration.sensor.lineDuration;
>  	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
>  	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>  }

-- 
Regards,

Laurent Pinchart