[v5,04/24] ipa: libipa: exposure_mode_helper: Keep stage params together
diff mbox series

Message ID 20261029125844.170324-5-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • ipa: libipa: agc: Take exposure margin into account
Related show

Commit Message

Barnabás Pőcze Oct. 29, 2026, 12:58 p.m. UTC 
Add a new type `Stage` that contains both the exposure time and gain
in one place, and have a single vector of `Stage`s instead of the
two same size exposure time / gain vectors.

Also clarify in the documentation that the gain of a stage only
constrains the analogue gain at the moment.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc_mean_luminance.cpp   | 12 ++++-----
 src/ipa/libipa/exposure_mode_helper.cpp | 33 +++++++++++++++----------
 src/ipa/libipa/exposure_mode_helper.h   | 12 +++++----
 3 files changed, 33 insertions(+), 24 deletions(-)

Comments

Jacopo Mondi Oct. 29, 2026, 3:37 p.m. UTC | #1 
Hi Barnabás

On Thu, Oct 29, 2026 at 01:58:24PM +0100, Barnabás Pőcze wrote:
> Add a new type `Stage` that contains both the exposure time and gain
> in one place, and have a single vector of `Stage`s instead of the
> two same size exposure time / gain vectors.
>
> Also clarify in the documentation that the gain of a stage only
> constrains the analogue gain at the moment.

Thanks, do you mean "clarify" as removing the existing comment ?

>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  src/ipa/libipa/agc_mean_luminance.cpp   | 12 ++++-----
>  src/ipa/libipa/exposure_mode_helper.cpp | 33 +++++++++++++++----------
>  src/ipa/libipa/exposure_mode_helper.h   | 12 +++++----
>  3 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 486c09c076..4c47770424 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -314,15 +314,15 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>  				return -EINVAL;
>  			}
>
> -			std::vector<std::pair<utils::Duration, double>> stages;
> -			for (unsigned int i = 0; i < exposureTimes.size(); i++) {
> +			std::vector<ExposureModeHelper::Stage> stages;
> +			for (size_t i = 0; i < exposureTimes.size(); i++) {
>  				stages.push_back({
> -					std::chrono::microseconds(exposureTimes[i]),
> -					gains[i]
> +					.exposureTime = std::chrono::microseconds(exposureTimes[i]),
> +					.gain = gains[i],
>  				});
>  			}
>
> -			exposureModeHelpers_.try_emplace(it->second, stages);
> +			exposureModeHelpers_.try_emplace(it->second, std::move(stages));
>  		}
>  	}
>
> @@ -334,7 +334,7 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>  	 */
>  	if (exposureModeHelpers_.empty())
>  		exposureModeHelpers_.try_emplace(controls::AeExposureModeNormal,
> -						 std::span<std::pair<utils::Duration, double>>{});
> +						 std::vector<ExposureModeHelper::Stage>{});
>
>  	return 0;
>  }
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index b3e75f6358..059792af28 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -59,24 +59,31 @@ namespace ipa {
>   * used.
>   */
>
> +/**
> +  * \class ExposureModeHelper::Stage
> +  * \brief Details of an exposure stage
> +  *
> +  * This type contains the parameters of a single exposure stage as used by the
> +  * algorithm.
> +  *
> +  * \var ExposureModeHelper::Stage::exposureTime
> +  * \brief The maximum exposure time of the stage
> +  *
> +  * \var ExposureModeHelper::Stage::gain
> +  * \brief The maximum analogue gain of the stage
> +  */
> +
>  /**
>   * \brief Construct an ExposureModeHelper instance
>   * \param[in] stages The vector of paired exposure time and gain limits
>   *
> - * The input stages are exposure time and _total_ gain pairs; the gain
> - * encompasses both analogue and digital gain.
> - *
>   * The vector of stages may be empty. In that case, the helper will simply use
>   * the runtime limits set through setLimits() instead.
>   */
> -ExposureModeHelper::ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages)
> -	: lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
> -	  minGain_(0), maxGain_(0), sensorHelper_(nullptr)
> +ExposureModeHelper::ExposureModeHelper(std::vector<Stage> &&stages)
> +	: stages_(std::move(stages)), lineDuration_(1us), minExposureTime_(0us),
> +	  maxExposureTime_(0us), minGain_(0), maxGain_(0), sensorHelper_(nullptr)
>  {
> -	for (const auto &[s, g] : stages) {
> -		exposureTimes_.push_back(s);
> -		gains_.push_back(g);
> -	}
>  }
>
>  /**
> @@ -236,10 +243,10 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>  	double stageGain = clampGain(1.0);
>  	double lastStageGain = stageGain;
>
> -	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
> -		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
> +	for (const auto &stage : stages_) {
> +		utils::Duration stageExposureTime = clampExposureTime(stage.exposureTime,
>  								      &quantGain);
> -		stageGain = clampGain(gains_[stage]);
> +		stageGain = clampGain(stage.gain);
>
>  		/*
>  		 * We perform the clamping on both exposure time and gain in
> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> index d0d468a6a2..1a77bf4da8 100644
> --- a/src/ipa/libipa/exposure_mode_helper.h
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -7,8 +7,6 @@
>
>  #pragma once
>
> -#include <span>
> -#include <utility>
>  #include <vector>
>
>  #include <libcamera/base/utils.h>
> @@ -22,7 +20,12 @@ namespace ipa {
>  class ExposureModeHelper
>  {
>  public:
> -	ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages);
> +	struct Stage {
> +		utils::Duration exposureTime;
> +		double gain;
> +	};
> +
> +	ExposureModeHelper(std::vector<Stage> &&stages);
>
>  	void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
>  	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> @@ -47,8 +50,7 @@ private:
>  					  double *quantizationGain = nullptr) const;
>  	double clampGain(double gain, double *quantizationGain = nullptr) const;
>
> -	std::vector<utils::Duration> exposureTimes_;
> -	std::vector<double> gains_;
> +	std::vector<Stage> stages_;
>
>  	utils::Duration lineDuration_;
>  	utils::Duration minExposureTime_;
> --
> 2.56.0
>
Barnabás Pőcze Oct. 29, 2026, 4:18 p.m. UTC | #2 
2026. 10. 29. 16:37 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Oct 29, 2026 at 01:58:24PM +0100, Barnabás Pőcze wrote:
>> Add a new type `Stage` that contains both the exposure time and gain
>> in one place, and have a single vector of `Stage`s instead of the
>> two same size exposure time / gain vectors.
>>
>> Also clarify in the documentation that the gain of a stage only
>> constrains the analogue gain at the moment.
> 
> Thanks, do you mean "clarify" as removing the existing comment ?

That and saying "analogue gain" in the documentation of `Stage::gain`.


> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>> ---
>>   src/ipa/libipa/agc_mean_luminance.cpp   | 12 ++++-----
>>   src/ipa/libipa/exposure_mode_helper.cpp | 33 +++++++++++++++----------
>>   src/ipa/libipa/exposure_mode_helper.h   | 12 +++++----
>>   3 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>> index 486c09c076..4c47770424 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>> @@ -314,15 +314,15 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>>   				return -EINVAL;
>>   			}
>>
>> -			std::vector<std::pair<utils::Duration, double>> stages;
>> -			for (unsigned int i = 0; i < exposureTimes.size(); i++) {
>> +			std::vector<ExposureModeHelper::Stage> stages;
>> +			for (size_t i = 0; i < exposureTimes.size(); i++) {
>>   				stages.push_back({
>> -					std::chrono::microseconds(exposureTimes[i]),
>> -					gains[i]
>> +					.exposureTime = std::chrono::microseconds(exposureTimes[i]),
>> +					.gain = gains[i],
>>   				});
>>   			}
>>
>> -			exposureModeHelpers_.try_emplace(it->second, stages);
>> +			exposureModeHelpers_.try_emplace(it->second, std::move(stages));
>>   		}
>>   	}
>>
>> @@ -334,7 +334,7 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>>   	 */
>>   	if (exposureModeHelpers_.empty())
>>   		exposureModeHelpers_.try_emplace(controls::AeExposureModeNormal,
>> -						 std::span<std::pair<utils::Duration, double>>{});
>> +						 std::vector<ExposureModeHelper::Stage>{});
>>
>>   	return 0;
>>   }
>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
>> index b3e75f6358..059792af28 100644
>> --- a/src/ipa/libipa/exposure_mode_helper.cpp
>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
>> @@ -59,24 +59,31 @@ namespace ipa {
>>    * used.
>>    */
>>
>> +/**
>> +  * \class ExposureModeHelper::Stage
>> +  * \brief Details of an exposure stage
>> +  *
>> +  * This type contains the parameters of a single exposure stage as used by the
>> +  * algorithm.
>> +  *
>> +  * \var ExposureModeHelper::Stage::exposureTime
>> +  * \brief The maximum exposure time of the stage
>> +  *
>> +  * \var ExposureModeHelper::Stage::gain
>> +  * \brief The maximum analogue gain of the stage
>> +  */
>> +
>>   /**
>>    * \brief Construct an ExposureModeHelper instance
>>    * \param[in] stages The vector of paired exposure time and gain limits
>>    *
>> - * The input stages are exposure time and _total_ gain pairs; the gain
>> - * encompasses both analogue and digital gain.
>> - *
>>    * The vector of stages may be empty. In that case, the helper will simply use
>>    * the runtime limits set through setLimits() instead.
>>    */
>> -ExposureModeHelper::ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages)
>> -	: lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
>> -	  minGain_(0), maxGain_(0), sensorHelper_(nullptr)
>> +ExposureModeHelper::ExposureModeHelper(std::vector<Stage> &&stages)
>> +	: stages_(std::move(stages)), lineDuration_(1us), minExposureTime_(0us),
>> +	  maxExposureTime_(0us), minGain_(0), maxGain_(0), sensorHelper_(nullptr)
>>   {
>> -	for (const auto &[s, g] : stages) {
>> -		exposureTimes_.push_back(s);
>> -		gains_.push_back(g);
>> -	}
>>   }
>>
>>   /**
>> @@ -236,10 +243,10 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>   	double stageGain = clampGain(1.0);
>>   	double lastStageGain = stageGain;
>>
>> -	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
>> -		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
>> +	for (const auto &stage : stages_) {
>> +		utils::Duration stageExposureTime = clampExposureTime(stage.exposureTime,
>>   								      &quantGain);
>> -		stageGain = clampGain(gains_[stage]);
>> +		stageGain = clampGain(stage.gain);
>>
>>   		/*
>>   		 * We perform the clamping on both exposure time and gain in
>> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
>> index d0d468a6a2..1a77bf4da8 100644
>> --- a/src/ipa/libipa/exposure_mode_helper.h
>> +++ b/src/ipa/libipa/exposure_mode_helper.h
>> @@ -7,8 +7,6 @@
>>
>>   #pragma once
>>
>> -#include <span>
>> -#include <utility>
>>   #include <vector>
>>
>>   #include <libcamera/base/utils.h>
>> @@ -22,7 +20,12 @@ namespace ipa {
>>   class ExposureModeHelper
>>   {
>>   public:
>> -	ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages);
>> +	struct Stage {
>> +		utils::Duration exposureTime;
>> +		double gain;
>> +	};
>> +
>> +	ExposureModeHelper(std::vector<Stage> &&stages);
>>
>>   	void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
>>   	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>> @@ -47,8 +50,7 @@ private:
>>   					  double *quantizationGain = nullptr) const;
>>   	double clampGain(double gain, double *quantizationGain = nullptr) const;
>>
>> -	std::vector<utils::Duration> exposureTimes_;
>> -	std::vector<double> gains_;
>> +	std::vector<Stage> stages_;
>>
>>   	utils::Duration lineDuration_;
>>   	utils::Duration minExposureTime_;
>> --
>> 2.56.0
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 486c09c076..4c47770424 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -314,15 +314,15 @@  int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
 				return -EINVAL;
 			}
 
-			std::vector<std::pair<utils::Duration, double>> stages;
-			for (unsigned int i = 0; i < exposureTimes.size(); i++) {
+			std::vector<ExposureModeHelper::Stage> stages;
+			for (size_t i = 0; i < exposureTimes.size(); i++) {
 				stages.push_back({
-					std::chrono::microseconds(exposureTimes[i]),
-					gains[i]
+					.exposureTime = std::chrono::microseconds(exposureTimes[i]),
+					.gain = gains[i],
 				});
 			}
 
-			exposureModeHelpers_.try_emplace(it->second, stages);
+			exposureModeHelpers_.try_emplace(it->second, std::move(stages));
 		}
 	}
 
@@ -334,7 +334,7 @@  int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
 	 */
 	if (exposureModeHelpers_.empty())
 		exposureModeHelpers_.try_emplace(controls::AeExposureModeNormal,
-						 std::span<std::pair<utils::Duration, double>>{});
+						 std::vector<ExposureModeHelper::Stage>{});
 
 	return 0;
 }
diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
index b3e75f6358..059792af28 100644
--- a/src/ipa/libipa/exposure_mode_helper.cpp
+++ b/src/ipa/libipa/exposure_mode_helper.cpp
@@ -59,24 +59,31 @@  namespace ipa {
  * used.
  */
 
+/**
+  * \class ExposureModeHelper::Stage
+  * \brief Details of an exposure stage
+  *
+  * This type contains the parameters of a single exposure stage as used by the
+  * algorithm.
+  *
+  * \var ExposureModeHelper::Stage::exposureTime
+  * \brief The maximum exposure time of the stage
+  *
+  * \var ExposureModeHelper::Stage::gain
+  * \brief The maximum analogue gain of the stage
+  */
+
 /**
  * \brief Construct an ExposureModeHelper instance
  * \param[in] stages The vector of paired exposure time and gain limits
  *
- * The input stages are exposure time and _total_ gain pairs; the gain
- * encompasses both analogue and digital gain.
- *
  * The vector of stages may be empty. In that case, the helper will simply use
  * the runtime limits set through setLimits() instead.
  */
-ExposureModeHelper::ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages)
-	: lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
-	  minGain_(0), maxGain_(0), sensorHelper_(nullptr)
+ExposureModeHelper::ExposureModeHelper(std::vector<Stage> &&stages)
+	: stages_(std::move(stages)), lineDuration_(1us), minExposureTime_(0us),
+	  maxExposureTime_(0us), minGain_(0), maxGain_(0), sensorHelper_(nullptr)
 {
-	for (const auto &[s, g] : stages) {
-		exposureTimes_.push_back(s);
-		gains_.push_back(g);
-	}
 }
 
 /**
@@ -236,10 +243,10 @@  ExposureModeHelper::splitExposure(utils::Duration exposure) const
 	double stageGain = clampGain(1.0);
 	double lastStageGain = stageGain;
 
-	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
-		utils::Duration stageExposureTime = clampExposureTime(exposureTimes_[stage],
+	for (const auto &stage : stages_) {
+		utils::Duration stageExposureTime = clampExposureTime(stage.exposureTime,
 								      &quantGain);
-		stageGain = clampGain(gains_[stage]);
+		stageGain = clampGain(stage.gain);
 
 		/*
 		 * We perform the clamping on both exposure time and gain in
diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
index d0d468a6a2..1a77bf4da8 100644
--- a/src/ipa/libipa/exposure_mode_helper.h
+++ b/src/ipa/libipa/exposure_mode_helper.h
@@ -7,8 +7,6 @@ 
 
 #pragma once
 
-#include <span>
-#include <utility>
 #include <vector>
 
 #include <libcamera/base/utils.h>
@@ -22,7 +20,12 @@  namespace ipa {
 class ExposureModeHelper
 {
 public:
-	ExposureModeHelper(std::span<std::pair<utils::Duration, double>> stages);
+	struct Stage {
+		utils::Duration exposureTime;
+		double gain;
+	};
+
+	ExposureModeHelper(std::vector<Stage> &&stages);
 
 	void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
@@ -47,8 +50,7 @@  private:
 					  double *quantizationGain = nullptr) const;
 	double clampGain(double gain, double *quantizationGain = nullptr) const;
 
-	std::vector<utils::Duration> exposureTimes_;
-	std::vector<double> gains_;
+	std::vector<Stage> stages_;
 
 	utils::Duration lineDuration_;
 	utils::Duration minExposureTime_;