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

Message ID 20211125102143.52556-4-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: ipu3: Misc clean up
Related show

Commit Message

Jean-Michel Hautbois Nov. 25, 2021, 10:21 a.m. UTC 
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@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 42 ++++++++++++++++++---------------
 src/ipa/ipu3/algorithms/agc.h   |  8 +------
 2 files changed, 24 insertions(+), 26 deletions(-)

Comments

Kieran Bingham Nov. 25, 2021, 12:04 p.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-11-25 10:21:42)
> 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.
> 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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_;
>  
>         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;
> +       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);
> +
>         /* 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_;
>  
> -- 
> 2.32.0
>
Laurent Pinchart Nov. 25, 2021, 12:10 p.m. UTC | #2 
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@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_;
>

Patch
diff mbox series 

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_;
 
 	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;
+	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);
+
 	/* 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_;