[RFC,v2,29/43] ipa: libipa: agc_mean_luminance: Remove control handling
diff mbox series

Message ID 20260723154327.1357866-30-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: agc rework
Related show

Commit Message

Barnabás Pőcze July 23, 2026, 3:43 p.m. UTC 
The `AgcMeanLuminance` type does not do any actual control handling,
so let's remove the `ControlInfoMap::Map` member, and let the user
handle that using the `constraintModes()` / `exposureModeHelpers()`
getters.

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

Comments

Jacopo Mondi July 27, 2026, 2:59 p.m. UTC | #1 
Hi Barnabás

On Thu, Jul 23, 2026 at 05:43:12PM +0200, Barnabás Pőcze wrote:
> The `AgcMeanLuminance` type does not do any actual control handling,
> so let's remove the `ControlInfoMap::Map` member, and let the user
> handle that using the `constraintModes()` / `exposureModeHelpers()`
> getters.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp                | 23 +++++++++++++++++------
>  src/ipa/libipa/agc_mean_luminance.cpp | 20 +-------------------
>  src/ipa/libipa/agc_mean_luminance.h   |  8 --------
>  3 files changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 54c5c6c349..27edc96fc6 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -400,16 +400,27 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>  	if (session.autoAllowed) {
>  		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>
> -		for (const auto &[id, info] : impl_.controls())
> -			config.ctrlMap[id] = info;
> +		{
> +			std::vector<ControlValue> options;
> +			for (const auto &[id, _] : impl_.constraintModes())
> +				options.emplace_back(id);
> +
> +			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> +		}
> +
> +		{
> +			std::vector<ControlValue> options;
> +			for (const auto &[id, _] : impl_.exposureModeHelpers())
> +				options.emplace_back(id);
> +
> +			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> +		}
>  	} else {
>  		config.ctrlMap.erase(&controls::ExposureValue);
> -
> -		for (const auto &[id, info] : impl_.controls())
> -			config.ctrlMap.erase(id);
> +		config.ctrlMap.erase(&controls::AeConstraintMode);
> +		config.ctrlMap.erase(&controls::AeExposureMode);
>  	}

I'm still not at peace with this, but it need a global solution, not
for this patch.

Can we record with a todo item the reason why we need to erase
controls ?

>
> -
>  	return 0;
>  }
>
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index a40b34b725..2c9c0b352c 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -237,8 +237,6 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
>
>  int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>  {
> -	std::vector<ControlValue> availableConstraintModes;
> -
>  	const ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];
>  	if (constraintModes.isDictionary()) {
>  		for (const auto &[modeName, modeDict] : constraintModes.asDict()) {
> @@ -258,8 +256,6 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>  			int ret = parseConstraint(modeDict, it->second);
>  			if (ret)
>  				return ret;
> -
> -			availableConstraintModes.push_back(it->second);
>  		}
>  	}
>
> @@ -278,18 +274,13 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>  		};
>
>  		constraintModes_[controls::ConstraintNormal].push_back(std::move(constraint));
> -		availableConstraintModes.push_back(controls::ConstraintNormal);
>  	}
>
> -	controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
> -
>  	return 0;
>  }
>
>  int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>  {
> -	std::vector<ControlValue> availableExposureModes;
> -
>  	const ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];
>  	if (exposureModes.isDictionary()) {
>  		for (const auto &[modeName, modeValues] : exposureModes.asDict()) {
> @@ -332,7 +323,6 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>  			}
>
>  			exposureModeHelpers_.try_emplace(it->second, stages);
> -			availableExposureModes.push_back(it->second);
>  		}
>  	}
>
> @@ -342,14 +332,11 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>  	 * in the ExposureModeHelper simply driving the exposure time as high as
>  	 * possible before touching gain.
>  	 */
> -	if (availableExposureModes.empty()) {
> +	if (exposureModeHelpers_.empty()) {

You can now drop the {}

>  		exposureModeHelpers_.try_emplace(controls::ExposureNormal,
>  						 Span<std::pair<utils::Duration, double>>{});
> -		availableExposureModes.push_back(controls::ExposureNormal);
>  	}
>
> -	controls_[&controls::AeExposureMode] = ControlInfo(availableExposureModes);
> -
>  	return 0;
>  }
>
> @@ -473,11 +460,6 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>   * \brief Get the ExposureModeHelpers that have been parsed from tuning data
>   */
>
> -/**
> - * \fn AgcMeanLuminance::controls()
> - * \brief Get the controls that have been generated after parsing tuning data
> - */
> -
>  /**
>   * \brief Estimate the initial gain needed to achieve a relative luminance
>   * target
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index 5e986be008..46a4d3e3f3 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -13,8 +13,6 @@
>
>  #include <libcamera/base/utils.h>
>
> -#include <libcamera/controls.h>
> -
>  #include "libcamera/internal/value_node.h"
>
>  #include "exposure_mode_helper.h"
> @@ -63,11 +61,6 @@ public:
>  		return exposureModeHelpers_;
>  	}
>
> -	ControlInfoMap::Map controls()
> -	{
> -		return controls_;
> -	}
> -
>  	struct Params {
>  		const Traits &traits;
>  		const Histogram &yHist;
> @@ -108,7 +101,6 @@ private:
>  	std::vector<AgcConstraint> additionalConstraints_;
>  	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
>  	std::map<int32_t, ExposureModeHelper> exposureModeHelpers_;
> -	ControlInfoMap::Map controls_;

The general direction makes sense!

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

>  };
>
>  } /* namespace ipa */
> --
> 2.55.0
>
Barnabás Pőcze July 27, 2026, 3:16 p.m. UTC | #2 
2026. 07. 27. 16:59 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Jul 23, 2026 at 05:43:12PM +0200, Barnabás Pőcze wrote:
>> The `AgcMeanLuminance` type does not do any actual control handling,
>> so let's remove the `ControlInfoMap::Map` member, and let the user
>> handle that using the `constraintModes()` / `exposureModeHelpers()`
>> getters.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp                | 23 +++++++++++++++++------
>>   src/ipa/libipa/agc_mean_luminance.cpp | 20 +-------------------
>>   src/ipa/libipa/agc_mean_luminance.h   |  8 --------
>>   3 files changed, 18 insertions(+), 33 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index 54c5c6c349..27edc96fc6 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -400,16 +400,27 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>>   	if (session.autoAllowed) {
>>   		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>>
>> -		for (const auto &[id, info] : impl_.controls())
>> -			config.ctrlMap[id] = info;
>> +		{
>> +			std::vector<ControlValue> options;
>> +			for (const auto &[id, _] : impl_.constraintModes())
>> +				options.emplace_back(id);
>> +
>> +			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
>> +		}
>> +
>> +		{
>> +			std::vector<ControlValue> options;
>> +			for (const auto &[id, _] : impl_.exposureModeHelpers())
>> +				options.emplace_back(id);
>> +
>> +			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
>> +		}
>>   	} else {
>>   		config.ctrlMap.erase(&controls::ExposureValue);
>> -
>> -		for (const auto &[id, info] : impl_.controls())
>> -			config.ctrlMap.erase(id);
>> +		config.ctrlMap.erase(&controls::AeConstraintMode);
>> +		config.ctrlMap.erase(&controls::AeExposureMode);
>>   	}
> 
> I'm still not at peace with this, but it need a global solution, not
> for this patch.

Me neither. I added an internal discussion point about this.


> 
> Can we record with a todo item the reason why we need to erase
> controls ?
> 
>>
>> -
>>   	return 0;
>>   }
>>
>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>> index a40b34b725..2c9c0b352c 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>> @@ -237,8 +237,6 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
>>
>>   int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>>   {
>> -	std::vector<ControlValue> availableConstraintModes;
>> -
>>   	const ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];
>>   	if (constraintModes.isDictionary()) {
>>   		for (const auto &[modeName, modeDict] : constraintModes.asDict()) {
>> @@ -258,8 +256,6 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>>   			int ret = parseConstraint(modeDict, it->second);
>>   			if (ret)
>>   				return ret;
>> -
>> -			availableConstraintModes.push_back(it->second);
>>   		}
>>   	}
>>
>> @@ -278,18 +274,13 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>>   		};
>>
>>   		constraintModes_[controls::ConstraintNormal].push_back(std::move(constraint));
>> -		availableConstraintModes.push_back(controls::ConstraintNormal);
>>   	}
>>
>> -	controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
>> -
>>   	return 0;
>>   }
>>
>>   int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>>   {
>> -	std::vector<ControlValue> availableExposureModes;
>> -
>>   	const ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];
>>   	if (exposureModes.isDictionary()) {
>>   		for (const auto &[modeName, modeValues] : exposureModes.asDict()) {
>> @@ -332,7 +323,6 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>>   			}
>>
>>   			exposureModeHelpers_.try_emplace(it->second, stages);
>> -			availableExposureModes.push_back(it->second);
>>   		}
>>   	}
>>
>> @@ -342,14 +332,11 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>>   	 * in the ExposureModeHelper simply driving the exposure time as high as
>>   	 * possible before touching gain.
>>   	 */
>> -	if (availableExposureModes.empty()) {
>> +	if (exposureModeHelpers_.empty()) {
> 
> You can now drop the {}
> 
>>   		exposureModeHelpers_.try_emplace(controls::ExposureNormal,
>>   						 Span<std::pair<utils::Duration, double>>{});
>> -		availableExposureModes.push_back(controls::ExposureNormal);
>>   	}
>>
>> -	controls_[&controls::AeExposureMode] = ControlInfo(availableExposureModes);
>> -
>>   	return 0;
>>   }
>>
>> @@ -473,11 +460,6 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>>    * \brief Get the ExposureModeHelpers that have been parsed from tuning data
>>    */
>>
>> -/**
>> - * \fn AgcMeanLuminance::controls()
>> - * \brief Get the controls that have been generated after parsing tuning data
>> - */
>> -
>>   /**
>>    * \brief Estimate the initial gain needed to achieve a relative luminance
>>    * target
>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
>> index 5e986be008..46a4d3e3f3 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.h
>> +++ b/src/ipa/libipa/agc_mean_luminance.h
>> @@ -13,8 +13,6 @@
>>
>>   #include <libcamera/base/utils.h>
>>
>> -#include <libcamera/controls.h>
>> -
>>   #include "libcamera/internal/value_node.h"
>>
>>   #include "exposure_mode_helper.h"
>> @@ -63,11 +61,6 @@ public:
>>   		return exposureModeHelpers_;
>>   	}
>>
>> -	ControlInfoMap::Map controls()
>> -	{
>> -		return controls_;
>> -	}
>> -
>>   	struct Params {
>>   		const Traits &traits;
>>   		const Histogram &yHist;
>> @@ -108,7 +101,6 @@ private:
>>   	std::vector<AgcConstraint> additionalConstraints_;
>>   	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
>>   	std::map<int32_t, ExposureModeHelper> exposureModeHelpers_;
>> -	ControlInfoMap::Map controls_;
> 
> The general direction makes sense!
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>>   };
>>
>>   } /* namespace ipa */
>> --
>> 2.55.0
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
index 54c5c6c349..27edc96fc6 100644
--- a/src/ipa/libipa/agc.cpp
+++ b/src/ipa/libipa/agc.cpp
@@ -400,16 +400,27 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
 	if (session.autoAllowed) {
 		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
 
-		for (const auto &[id, info] : impl_.controls())
-			config.ctrlMap[id] = info;
+		{
+			std::vector<ControlValue> options;
+			for (const auto &[id, _] : impl_.constraintModes())
+				options.emplace_back(id);
+
+			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
+		}
+
+		{
+			std::vector<ControlValue> options;
+			for (const auto &[id, _] : impl_.exposureModeHelpers())
+				options.emplace_back(id);
+
+			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
+		}
 	} else {
 		config.ctrlMap.erase(&controls::ExposureValue);
-
-		for (const auto &[id, info] : impl_.controls())
-			config.ctrlMap.erase(id);
+		config.ctrlMap.erase(&controls::AeConstraintMode);
+		config.ctrlMap.erase(&controls::AeExposureMode);
 	}
 
-
 	return 0;
 }
 
diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index a40b34b725..2c9c0b352c 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -237,8 +237,6 @@  int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
 
 int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
 {
-	std::vector<ControlValue> availableConstraintModes;
-
 	const ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];
 	if (constraintModes.isDictionary()) {
 		for (const auto &[modeName, modeDict] : constraintModes.asDict()) {
@@ -258,8 +256,6 @@  int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
 			int ret = parseConstraint(modeDict, it->second);
 			if (ret)
 				return ret;
-
-			availableConstraintModes.push_back(it->second);
 		}
 	}
 
@@ -278,18 +274,13 @@  int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
 		};
 
 		constraintModes_[controls::ConstraintNormal].push_back(std::move(constraint));
-		availableConstraintModes.push_back(controls::ConstraintNormal);
 	}
 
-	controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
-
 	return 0;
 }
 
 int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
 {
-	std::vector<ControlValue> availableExposureModes;
-
 	const ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];
 	if (exposureModes.isDictionary()) {
 		for (const auto &[modeName, modeValues] : exposureModes.asDict()) {
@@ -332,7 +323,6 @@  int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
 			}
 
 			exposureModeHelpers_.try_emplace(it->second, stages);
-			availableExposureModes.push_back(it->second);
 		}
 	}
 
@@ -342,14 +332,11 @@  int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
 	 * in the ExposureModeHelper simply driving the exposure time as high as
 	 * possible before touching gain.
 	 */
-	if (availableExposureModes.empty()) {
+	if (exposureModeHelpers_.empty()) {
 		exposureModeHelpers_.try_emplace(controls::ExposureNormal,
 						 Span<std::pair<utils::Duration, double>>{});
-		availableExposureModes.push_back(controls::ExposureNormal);
 	}
 
-	controls_[&controls::AeExposureMode] = ControlInfo(availableExposureModes);
-
 	return 0;
 }
 
@@ -473,11 +460,6 @@  void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
  * \brief Get the ExposureModeHelpers that have been parsed from tuning data
  */
 
-/**
- * \fn AgcMeanLuminance::controls()
- * \brief Get the controls that have been generated after parsing tuning data
- */
-
 /**
  * \brief Estimate the initial gain needed to achieve a relative luminance
  * target
diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
index 5e986be008..46a4d3e3f3 100644
--- a/src/ipa/libipa/agc_mean_luminance.h
+++ b/src/ipa/libipa/agc_mean_luminance.h
@@ -13,8 +13,6 @@ 
 
 #include <libcamera/base/utils.h>
 
-#include <libcamera/controls.h>
-
 #include "libcamera/internal/value_node.h"
 
 #include "exposure_mode_helper.h"
@@ -63,11 +61,6 @@  public:
 		return exposureModeHelpers_;
 	}
 
-	ControlInfoMap::Map controls()
-	{
-		return controls_;
-	}
-
 	struct Params {
 		const Traits &traits;
 		const Histogram &yHist;
@@ -108,7 +101,6 @@  private:
 	std::vector<AgcConstraint> additionalConstraints_;
 	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
 	std::map<int32_t, ExposureModeHelper> exposureModeHelpers_;
-	ControlInfoMap::Map controls_;
 };
 
 } /* namespace ipa */