[RFC,v3,46/50] ipa: libipa: agc: Work without `CameraSensorHelper`
diff mbox series

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

Commit Message

Barnabás Pőcze Aug. 3, 2026, 1:14 p.m. UTC 
Use the agc algorithm extracted from the simple ipa module (AgcMSV)
to provide some kind of operation when a `CameraSensorHelper` is
not available.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
 src/ipa/libipa/agc.cpp              | 197 +++++++++++++++++++---------
 src/ipa/libipa/agc.h                |   9 +-
 src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
 src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
 5 files changed, 142 insertions(+), 76 deletions(-)

Comments

Milan Zamazal Aug. 4, 2026, 3:18 p.m. UTC | #1 
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Use the agc algorithm extracted from the simple ipa module (AgcMSV)
> to provide some kind of operation when a `CameraSensorHelper` is
> not available.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
>  src/ipa/libipa/agc.cpp              | 197 +++++++++++++++++++---------
>  src/ipa/libipa/agc.h                |   9 +-
>  src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
>  src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
>  5 files changed, 142 insertions(+), 76 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 91923c7f70..a79898520c 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -68,12 +68,11 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
>  	int ret;
>  
> -	ret = agc_.init(tuningData);
> +	ret = agc_.init(tuningData, context.camHelper.get());
>  	if (ret)
>  		return ret;
>  
>  	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
> @@ -98,7 +97,6 @@ int Agc::configure(IPAContext &context,
>  	bdsGrid_ = context.configuration.grid.bdsGrid;
>  
>  	return agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 3efa4a89e1..b8b4cc9e83 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -11,10 +11,12 @@
>  #include <array>
>  #include <chrono>
>  #include <optional>
> +#include <variant>
>  
>  #include <linux/v4l2-controls.h>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
> @@ -50,6 +52,9 @@ LOG_DEFINE_CATEGORY(Agc)
>   * \var agc::Session::maxAnalogueGain
>   * \brief Maximum analogue gain for the streaming session
>   *
> + * \var agc::Session::defAnalogueGain
> + * \brief Default analogue gain of the configured sensor
> + *
>   * \var agc::Session::minFrameDuration
>   * \brief Minimum frame duration for the streaming session
>   *
> @@ -188,9 +193,6 @@ LOG_DEFINE_CATEGORY(Agc)
>   * \struct AgcAlgorithm::ConfigurationParams
>   * \brief Parameters for AgcAlgorithm::configure()
>   *
> - * \var AgcAlgorithm::ConfigurationParams::sensor
> - * \brief CameraSensorHelper for the sensor
> - *
>   * \var AgcAlgorithm::ConfigurationParams::sensorInfo
>   * \brief Current configuration of the sensor
>   *
> @@ -235,11 +237,18 @@ LOG_DEFINE_CATEGORY(Agc)
>  /**
>   * \brief Load tuning data
>   */
> -int AgcAlgorithm::init(const ValueNode &tuningData)
> +int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor)
>  {
> -	int ret = impl_.parseTuningData(tuningData);
> -	if (ret)
> -		return ret;
> +	if (sensor) {
> +		auto &impl = impl_.emplace<AgcMeanLuminance>();
> +		int ret = impl.parseTuningData(tuningData);
> +		if (ret)
> +			return ret;
> +	} else {
> +		impl_.emplace<AgcMSV>();
> +	}

This looks OK here but I'm not sure it's completely fine for software
ISP where AgcMSV is currently used, once it switches to AgcAlgorithm.
What if a sensor helper is available but doesn't contain the desired
tuning data?  Is it still better to use AgcMeanLuminance?  This deserves
some explanation in the commit message.

> +	sensor_ = sensor;
>  
>  	return 0;
>  }
> @@ -270,10 +279,14 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
>  
>  	/* Compute the analogue gain limits. */
> +	const auto extractGain = [&](const ControlValue &v) {
> +		auto gainCode = v.get<int32_t>();
> +		return sensor_ ? sensor_->gain(gainCode) : gainCode;
> +	};
>  	const ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
> -	float minGain = config.sensor->gain(v4l2Gain.min().get<int32_t>());
> -	float maxGain = config.sensor->gain(v4l2Gain.max().get<int32_t>());
> -	float defGain = config.sensor->gain(v4l2Gain.def().get<int32_t>());
> +	float minGain = extractGain(v4l2Gain.min());
> +	float maxGain = extractGain(v4l2Gain.max());
> +	float defGain = extractGain(v4l2Gain.def());
>  
>  	LOG(Agc, Debug)
>  		<< "exposure:[" << minExposure << ',' << maxExposure << ']'
> @@ -312,28 +325,21 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	session.maxExposureTime = maxExposure * session.lineDuration;
>  	session.minAnalogueGain = minGain;
>  	session.maxAnalogueGain = maxGain;
> +	session.defAnalogueGain = defGain;
>  	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
>  	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
>  
> -	impl_.configure(session.lineDuration, config.sensor);
> -	impl_.resetFrameCount();
> -
>  	/* Configure the default exposure and gain. */
>  	state = {};
>  	state.automatic.gain = session.minAnalogueGain;
>  	state.automatic.exposure = defExposure;
>  	state.automatic.quantizationGain = 1;
>  	state.automatic.digitalGain = 1;
> -	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
>  	state.manual.gain = state.automatic.gain;
>  	state.manual.exposure = state.automatic.exposure;
>  	state.autoExposureEnabled = session.autoAllowed;
>  	state.autoGainEnabled = session.autoAllowed;
>  	state.exposureValue = 0;
> -	state.constraintMode =
> -		static_cast<controls::AeConstraintModeEnum>(impl_.constraintModes().begin()->first);
> -	state.exposureMode =
> -		static_cast<controls::AeExposureModeEnum>(impl_.exposureModeHelpers().begin()->first);
>  	state.minFrameDuration = session.minFrameDuration;
>  	state.maxFrameDuration = session.maxFrameDuration;
>  
> @@ -379,25 +385,59 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  		Span<const int64_t, 2>{ { frameDurations[0], frameDurations[1] } },
>  	};
>  
> -	if (session.autoAllowed) {
> -		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> -
> -		{
> -			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 {
> +	std::visit(utils::overloaded{
> +		[&](AgcMSV&) {
> +			/* no constraint/exposure mode support */
> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> +
> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
> +
> +			if (session.autoAllowed) {
> +				config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
> +					std::array{ ControlValue(state.constraintMode) }
> +				);
> +
> +				config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
> +					std::array{ ControlValue(state.exposureMode) }
> +				);
> +			}
> +		},
> +		[&](AgcMeanLuminance& impl) {
> +			state.constraintMode =
> +				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> +			state.exposureMode =
> +				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> +
> +			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> +
> +			ASSERT(sensor_);
> +			impl.configure(session.lineDuration, sensor_);
> +			impl.resetFrameCount();
> +
> +			if (session.autoAllowed) {
> +				config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> +
> +				{
> +					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);
> +				}
> +			}
> +		},
> +	}, impl_);
> +
> +	if (!session.autoAllowed) {
>  		config.ctrlMap.erase(&controls::ExposureValue);
>  		config.ctrlMap.erase(&controls::AeConstraintMode);
>  		config.ctrlMap.erase(&controls::AeExposureMode);
> @@ -599,33 +639,62 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  			maxAnalogueGain = frameContext.gain;
>  		}
>  
> -		/*
> -		 * The Agc algorithm needs to know the effective exposure value that was
> -		 * applied to the sensor when the statistics were collected.
> -		 */
> -		utils::Duration effectiveExposureValue =
> -			lineDuration * params->exposure * params->gain;
> -
> -		impl_.setLimits(minExposureTime, maxExposureTime,
> -				minAnalogueGain, maxAnalogueGain,
> -				std::move(params->additionalConstraints));
> -
> -		const auto &newEv = impl_.calculateNewEv({
> -			.traits = params->traits,
> -			.yHist = params->yHist,
> -			.effectiveExposureValue = effectiveExposureValue,
> -			.constraintModeIndex = frameContext.constraintMode,
> -			.exposureModeIndex = frameContext.exposureMode,
> -			.lux = params->lux,
> -			.exposureCompensation = pow(2.0, frameContext.exposureValue),
> -		});
> -
> -		/* Update the estimated exposure and gain. */
> -		state.automatic.exposure = newEv.exposureTime / lineDuration;
> -		state.automatic.gain = newEv.analogueGain;
> -		state.automatic.quantizationGain = newEv.quantizationGain;
> -		state.automatic.digitalGain = newEv.digitalGain;
> -		state.automatic.yTarget = newEv.yTarget;
> +		std::visit(utils::overloaded{
> +			[&](AgcMSV& impl) {
> +				impl.setLimits({
> +					.exposure = {
> +						uint32_t(minExposureTime / lineDuration),
> +						uint32_t(maxExposureTime / lineDuration),
> +					},
> +					.gain = {
> +						minAnalogueGain,
> +						maxAnalogueGain,
> +					},
> +					/* gain codes -> step size of 1 */
> +					.gainMinStep = 1,
> +					/* assume default gain is close to 1.0 */
> +					.gain1 = session.defAnalogueGain,
> +				});
> +
> +				const auto& newEv = impl.calculateNewEv({
> +					.yHist = params->yHist,
> +					.exposure = params->exposure,
> +					.gain = params->gain,
> +				});
> +
> +				state.automatic.exposure = newEv.exposure;
> +				state.automatic.gain = newEv.analogueGain;
> +			},
> +			[&](AgcMeanLuminance& impl) {
> +				/*
> +				 * The Agc algorithm needs to know the effective exposure value that was
> +				 * applied to the sensor when the statistics were collected.
> +				 */
> +				utils::Duration effectiveExposureValue =
> +					lineDuration * params->exposure * params->gain;
> +
> +				impl.setLimits(minExposureTime, maxExposureTime,
> +					       minAnalogueGain, maxAnalogueGain,
> +					       std::move(params->additionalConstraints));
> +
> +				const auto &newEv = impl.calculateNewEv({
> +					.traits = params->traits,
> +					.yHist = params->yHist,
> +					.effectiveExposureValue = effectiveExposureValue,
> +					.constraintModeIndex = frameContext.constraintMode,
> +					.exposureModeIndex = frameContext.exposureMode,
> +					.lux = params->lux,
> +					.exposureCompensation = pow(2.0, frameContext.exposureValue),
> +				});
> +
> +				/* Update the estimated exposure and gain. */
> +				state.automatic.exposure = newEv.exposureTime / lineDuration;
> +				state.automatic.gain = newEv.analogueGain;
> +				state.automatic.quantizationGain = newEv.quantizationGain;
> +				state.automatic.digitalGain = newEv.digitalGain;
> +				state.automatic.yTarget = newEv.yTarget;
> +			},
> +		}, impl_);
>  
>  		LOG(Agc, Debug)
>  			<< "exposure-time:" << utils::Duration(state.automatic.exposure * lineDuration)
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index e96c6926b2..4f8038ae41 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -9,6 +9,7 @@
>  
>  #include <optional>
>  #include <utility>
> +#include <variant>
>  
>  #include <linux/v4l2-controls.h>
>  
> @@ -18,6 +19,7 @@
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
>  #include "agc_mean_luminance.h"
> +#include "agc_msv.h"
>  #include "camera_sensor_helper.h"
>  #include "histogram.h"
>  
> @@ -53,6 +55,7 @@ struct Session {
>  	utils::Duration maxExposureTime;
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
> +	double defAnalogueGain;
>  	utils::Duration minFrameDuration;
>  	utils::Duration maxFrameDuration;
>  
> @@ -111,14 +114,13 @@ class AgcAlgorithm
>  {
>  public:
>  	struct ConfigurationParams {
> -		const CameraSensorHelper *sensor;
>  		const IPACameraSensorInfo &sensorInfo;
>  		const ControlInfoMap &sensorControls;
>  		ControlInfoMap::Map &ctrlMap;
>  		bool autoAllowed = true;
>  	};
>  
> -	int init(const ValueNode &tuningData);
> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
>  
>  	int configure(agc::Session &session, agc::ActiveState &state,
>  		      const ConfigurationParams &config);
> @@ -143,7 +145,8 @@ public:
>  		     ControlList &metadata);
>  
>  private:
> -	AgcMeanLuminance impl_;
> +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
> +	CameraSensorHelper *sensor_ = nullptr;
>  };
>  
>  } /* namespace ipa */
> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> index 87586e8a82..c869cc0235 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -122,12 +122,11 @@ Agc::Agc()
>  
>  int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
> -	int ret = agc_.init(tuningData);
> +	int ret = agc_.init(tuningData, context.camHelper.get());
>  	if (ret)
>  		return ret;
>  
>  	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
> @@ -147,7 +146,6 @@ int Agc::configure(IPAContext &context,
>  		return ret;
>  
>  	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 06e0646981..c4de8d4bab 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -136,12 +136,11 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
>  	int ret;
>  
> -	ret = agc_.init(tuningData);
> +	ret = agc_.init(tuningData, context.camHelper.get());
>  	if (ret)
>  		return ret;
>  
>  	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
> @@ -167,7 +166,6 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
>  	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
>  		.sensorInfo = context.sensorInfo,
>  		.sensorControls = context.sensorControls,
>  		.ctrlMap = context.ctrlMap,
Barnabás Pőcze Aug. 4, 2026, 3:28 p.m. UTC | #2 
2026. 08. 04. 17:18 keltezéssel, Milan Zamazal írta:
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> Use the agc algorithm extracted from the simple ipa module (AgcMSV)
>> to provide some kind of operation when a `CameraSensorHelper` is
>> not available.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
>>   src/ipa/libipa/agc.cpp              | 197 +++++++++++++++++++---------
>>   src/ipa/libipa/agc.h                |   9 +-
>>   src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
>>   src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
>>   5 files changed, 142 insertions(+), 76 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 91923c7f70..a79898520c 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -68,12 +68,11 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>>   {
>>   	int ret;
>>   
>> -	ret = agc_.init(tuningData);
>> +	ret = agc_.init(tuningData, context.camHelper.get());
>>   	if (ret)
>>   		return ret;
>>   
>>   	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
>> -		.sensor = context.camHelper.get(),
>>   		.sensorInfo = context.sensorInfo,
>>   		.sensorControls = context.sensorControls,
>>   		.ctrlMap = context.ctrlMap,
>> @@ -98,7 +97,6 @@ int Agc::configure(IPAContext &context,
>>   	bdsGrid_ = context.configuration.grid.bdsGrid;
>>   
>>   	return agc_.configure(context.configuration.agc, context.activeState.agc, {
>> -		.sensor = context.camHelper.get(),
>>   		.sensorInfo = context.sensorInfo,
>>   		.sensorControls = context.sensorControls,
>>   		.ctrlMap = context.ctrlMap,
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index 3efa4a89e1..b8b4cc9e83 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -11,10 +11,12 @@
>>   #include <array>
>>   #include <chrono>
>>   #include <optional>
>> +#include <variant>
>>   
>>   #include <linux/v4l2-controls.h>
>>   
>>   #include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>>   
>>   #include <libcamera/control_ids.h>
>>   #include <libcamera/controls.h>
>> @@ -50,6 +52,9 @@ LOG_DEFINE_CATEGORY(Agc)
>>    * \var agc::Session::maxAnalogueGain
>>    * \brief Maximum analogue gain for the streaming session
>>    *
>> + * \var agc::Session::defAnalogueGain
>> + * \brief Default analogue gain of the configured sensor
>> + *
>>    * \var agc::Session::minFrameDuration
>>    * \brief Minimum frame duration for the streaming session
>>    *
>> @@ -188,9 +193,6 @@ LOG_DEFINE_CATEGORY(Agc)
>>    * \struct AgcAlgorithm::ConfigurationParams
>>    * \brief Parameters for AgcAlgorithm::configure()
>>    *
>> - * \var AgcAlgorithm::ConfigurationParams::sensor
>> - * \brief CameraSensorHelper for the sensor
>> - *
>>    * \var AgcAlgorithm::ConfigurationParams::sensorInfo
>>    * \brief Current configuration of the sensor
>>    *
>> @@ -235,11 +237,18 @@ LOG_DEFINE_CATEGORY(Agc)
>>   /**
>>    * \brief Load tuning data
>>    */
>> -int AgcAlgorithm::init(const ValueNode &tuningData)
>> +int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor)
>>   {
>> -	int ret = impl_.parseTuningData(tuningData);
>> -	if (ret)
>> -		return ret;
>> +	if (sensor) {
>> +		auto &impl = impl_.emplace<AgcMeanLuminance>();
>> +		int ret = impl.parseTuningData(tuningData);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		impl_.emplace<AgcMSV>();
>> +	}
> 
> This looks OK here but I'm not sure it's completely fine for software
> ISP where AgcMSV is currently used, once it switches to AgcAlgorithm.
> What if a sensor helper is available but doesn't contain the desired
> tuning data?  Is it still better to use AgcMeanLuminance?  This deserves
> some explanation in the commit message.

I can only speak to the current state, which is that only the gain model is
needed, and that is a mandatory part of `CameraSensorHelper`. And if that is
available, I think AgcMeanLuminance provides a better experience (at least in
my experiments), so it makes sense to use it. Now if that changes, and there
potentially optional parameters, then this can (should be) revisited.

Testing AgcMeanLuminance on various hardware would also be welcome.


> 
>> +	sensor_ = sensor;
>>   
>>   	return 0;
>>   }
>> [...]

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 91923c7f70..a79898520c 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -68,12 +68,11 @@  int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
 	int ret;
 
-	ret = agc_.init(tuningData);
+	ret = agc_.init(tuningData, context.camHelper.get());
 	if (ret)
 		return ret;
 
 	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,
@@ -98,7 +97,6 @@  int Agc::configure(IPAContext &context,
 	bdsGrid_ = context.configuration.grid.bdsGrid;
 
 	return agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,
diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
index 3efa4a89e1..b8b4cc9e83 100644
--- a/src/ipa/libipa/agc.cpp
+++ b/src/ipa/libipa/agc.cpp
@@ -11,10 +11,12 @@ 
 #include <array>
 #include <chrono>
 #include <optional>
+#include <variant>
 
 #include <linux/v4l2-controls.h>
 
 #include <libcamera/base/log.h>
+#include <libcamera/base/utils.h>
 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
@@ -50,6 +52,9 @@  LOG_DEFINE_CATEGORY(Agc)
  * \var agc::Session::maxAnalogueGain
  * \brief Maximum analogue gain for the streaming session
  *
+ * \var agc::Session::defAnalogueGain
+ * \brief Default analogue gain of the configured sensor
+ *
  * \var agc::Session::minFrameDuration
  * \brief Minimum frame duration for the streaming session
  *
@@ -188,9 +193,6 @@  LOG_DEFINE_CATEGORY(Agc)
  * \struct AgcAlgorithm::ConfigurationParams
  * \brief Parameters for AgcAlgorithm::configure()
  *
- * \var AgcAlgorithm::ConfigurationParams::sensor
- * \brief CameraSensorHelper for the sensor
- *
  * \var AgcAlgorithm::ConfigurationParams::sensorInfo
  * \brief Current configuration of the sensor
  *
@@ -235,11 +237,18 @@  LOG_DEFINE_CATEGORY(Agc)
 /**
  * \brief Load tuning data
  */
-int AgcAlgorithm::init(const ValueNode &tuningData)
+int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor)
 {
-	int ret = impl_.parseTuningData(tuningData);
-	if (ret)
-		return ret;
+	if (sensor) {
+		auto &impl = impl_.emplace<AgcMeanLuminance>();
+		int ret = impl.parseTuningData(tuningData);
+		if (ret)
+			return ret;
+	} else {
+		impl_.emplace<AgcMSV>();
+	}
+
+	sensor_ = sensor;
 
 	return 0;
 }
@@ -270,10 +279,14 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
 	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
 
 	/* Compute the analogue gain limits. */
+	const auto extractGain = [&](const ControlValue &v) {
+		auto gainCode = v.get<int32_t>();
+		return sensor_ ? sensor_->gain(gainCode) : gainCode;
+	};
 	const ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
-	float minGain = config.sensor->gain(v4l2Gain.min().get<int32_t>());
-	float maxGain = config.sensor->gain(v4l2Gain.max().get<int32_t>());
-	float defGain = config.sensor->gain(v4l2Gain.def().get<int32_t>());
+	float minGain = extractGain(v4l2Gain.min());
+	float maxGain = extractGain(v4l2Gain.max());
+	float defGain = extractGain(v4l2Gain.def());
 
 	LOG(Agc, Debug)
 		<< "exposure:[" << minExposure << ',' << maxExposure << ']'
@@ -312,28 +325,21 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
 	session.maxExposureTime = maxExposure * session.lineDuration;
 	session.minAnalogueGain = minGain;
 	session.maxAnalogueGain = maxGain;
+	session.defAnalogueGain = defGain;
 	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
 	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
 
-	impl_.configure(session.lineDuration, config.sensor);
-	impl_.resetFrameCount();
-
 	/* Configure the default exposure and gain. */
 	state = {};
 	state.automatic.gain = session.minAnalogueGain;
 	state.automatic.exposure = defExposure;
 	state.automatic.quantizationGain = 1;
 	state.automatic.digitalGain = 1;
-	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
 	state.manual.gain = state.automatic.gain;
 	state.manual.exposure = state.automatic.exposure;
 	state.autoExposureEnabled = session.autoAllowed;
 	state.autoGainEnabled = session.autoAllowed;
 	state.exposureValue = 0;
-	state.constraintMode =
-		static_cast<controls::AeConstraintModeEnum>(impl_.constraintModes().begin()->first);
-	state.exposureMode =
-		static_cast<controls::AeExposureModeEnum>(impl_.exposureModeHelpers().begin()->first);
 	state.minFrameDuration = session.minFrameDuration;
 	state.maxFrameDuration = session.maxFrameDuration;
 
@@ -379,25 +385,59 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
 		Span<const int64_t, 2>{ { frameDurations[0], frameDurations[1] } },
 	};
 
-	if (session.autoAllowed) {
-		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
-
-		{
-			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 {
+	std::visit(utils::overloaded{
+		[&](AgcMSV&) {
+			/* no constraint/exposure mode support */
+			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
+			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
+
+			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
+
+			if (session.autoAllowed) {
+				config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(
+					std::array{ ControlValue(state.constraintMode) }
+				);
+
+				config.ctrlMap[&controls::AeExposureMode] = ControlInfo(
+					std::array{ ControlValue(state.exposureMode) }
+				);
+			}
+		},
+		[&](AgcMeanLuminance& impl) {
+			state.constraintMode =
+				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
+			state.exposureMode =
+				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
+
+			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
+
+			ASSERT(sensor_);
+			impl.configure(session.lineDuration, sensor_);
+			impl.resetFrameCount();
+
+			if (session.autoAllowed) {
+				config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
+
+				{
+					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);
+				}
+			}
+		},
+	}, impl_);
+
+	if (!session.autoAllowed) {
 		config.ctrlMap.erase(&controls::ExposureValue);
 		config.ctrlMap.erase(&controls::AeConstraintMode);
 		config.ctrlMap.erase(&controls::AeExposureMode);
@@ -599,33 +639,62 @@  void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
 			maxAnalogueGain = frameContext.gain;
 		}
 
-		/*
-		 * The Agc algorithm needs to know the effective exposure value that was
-		 * applied to the sensor when the statistics were collected.
-		 */
-		utils::Duration effectiveExposureValue =
-			lineDuration * params->exposure * params->gain;
-
-		impl_.setLimits(minExposureTime, maxExposureTime,
-				minAnalogueGain, maxAnalogueGain,
-				std::move(params->additionalConstraints));
-
-		const auto &newEv = impl_.calculateNewEv({
-			.traits = params->traits,
-			.yHist = params->yHist,
-			.effectiveExposureValue = effectiveExposureValue,
-			.constraintModeIndex = frameContext.constraintMode,
-			.exposureModeIndex = frameContext.exposureMode,
-			.lux = params->lux,
-			.exposureCompensation = pow(2.0, frameContext.exposureValue),
-		});
-
-		/* Update the estimated exposure and gain. */
-		state.automatic.exposure = newEv.exposureTime / lineDuration;
-		state.automatic.gain = newEv.analogueGain;
-		state.automatic.quantizationGain = newEv.quantizationGain;
-		state.automatic.digitalGain = newEv.digitalGain;
-		state.automatic.yTarget = newEv.yTarget;
+		std::visit(utils::overloaded{
+			[&](AgcMSV& impl) {
+				impl.setLimits({
+					.exposure = {
+						uint32_t(minExposureTime / lineDuration),
+						uint32_t(maxExposureTime / lineDuration),
+					},
+					.gain = {
+						minAnalogueGain,
+						maxAnalogueGain,
+					},
+					/* gain codes -> step size of 1 */
+					.gainMinStep = 1,
+					/* assume default gain is close to 1.0 */
+					.gain1 = session.defAnalogueGain,
+				});
+
+				const auto& newEv = impl.calculateNewEv({
+					.yHist = params->yHist,
+					.exposure = params->exposure,
+					.gain = params->gain,
+				});
+
+				state.automatic.exposure = newEv.exposure;
+				state.automatic.gain = newEv.analogueGain;
+			},
+			[&](AgcMeanLuminance& impl) {
+				/*
+				 * The Agc algorithm needs to know the effective exposure value that was
+				 * applied to the sensor when the statistics were collected.
+				 */
+				utils::Duration effectiveExposureValue =
+					lineDuration * params->exposure * params->gain;
+
+				impl.setLimits(minExposureTime, maxExposureTime,
+					       minAnalogueGain, maxAnalogueGain,
+					       std::move(params->additionalConstraints));
+
+				const auto &newEv = impl.calculateNewEv({
+					.traits = params->traits,
+					.yHist = params->yHist,
+					.effectiveExposureValue = effectiveExposureValue,
+					.constraintModeIndex = frameContext.constraintMode,
+					.exposureModeIndex = frameContext.exposureMode,
+					.lux = params->lux,
+					.exposureCompensation = pow(2.0, frameContext.exposureValue),
+				});
+
+				/* Update the estimated exposure and gain. */
+				state.automatic.exposure = newEv.exposureTime / lineDuration;
+				state.automatic.gain = newEv.analogueGain;
+				state.automatic.quantizationGain = newEv.quantizationGain;
+				state.automatic.digitalGain = newEv.digitalGain;
+				state.automatic.yTarget = newEv.yTarget;
+			},
+		}, impl_);
 
 		LOG(Agc, Debug)
 			<< "exposure-time:" << utils::Duration(state.automatic.exposure * lineDuration)
diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
index e96c6926b2..4f8038ae41 100644
--- a/src/ipa/libipa/agc.h
+++ b/src/ipa/libipa/agc.h
@@ -9,6 +9,7 @@ 
 
 #include <optional>
 #include <utility>
+#include <variant>
 
 #include <linux/v4l2-controls.h>
 
@@ -18,6 +19,7 @@ 
 #include <libcamera/ipa/core_ipa_interface.h>
 
 #include "agc_mean_luminance.h"
+#include "agc_msv.h"
 #include "camera_sensor_helper.h"
 #include "histogram.h"
 
@@ -53,6 +55,7 @@  struct Session {
 	utils::Duration maxExposureTime;
 	double minAnalogueGain;
 	double maxAnalogueGain;
+	double defAnalogueGain;
 	utils::Duration minFrameDuration;
 	utils::Duration maxFrameDuration;
 
@@ -111,14 +114,13 @@  class AgcAlgorithm
 {
 public:
 	struct ConfigurationParams {
-		const CameraSensorHelper *sensor;
 		const IPACameraSensorInfo &sensorInfo;
 		const ControlInfoMap &sensorControls;
 		ControlInfoMap::Map &ctrlMap;
 		bool autoAllowed = true;
 	};
 
-	int init(const ValueNode &tuningData);
+	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
 
 	int configure(agc::Session &session, agc::ActiveState &state,
 		      const ConfigurationParams &config);
@@ -143,7 +145,8 @@  public:
 		     ControlList &metadata);
 
 private:
-	AgcMeanLuminance impl_;
+	std::variant<AgcMSV, AgcMeanLuminance> impl_;
+	CameraSensorHelper *sensor_ = nullptr;
 };
 
 } /* namespace ipa */
diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
index 87586e8a82..c869cc0235 100644
--- a/src/ipa/mali-c55/algorithms/agc.cpp
+++ b/src/ipa/mali-c55/algorithms/agc.cpp
@@ -122,12 +122,11 @@  Agc::Agc()
 
 int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
-	int ret = agc_.init(tuningData);
+	int ret = agc_.init(tuningData, context.camHelper.get());
 	if (ret)
 		return ret;
 
 	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,
@@ -147,7 +146,6 @@  int Agc::configure(IPAContext &context,
 		return ret;
 
 	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 06e0646981..c4de8d4bab 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -136,12 +136,11 @@  int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
 	int ret;
 
-	ret = agc_.init(tuningData);
+	ret = agc_.init(tuningData, context.camHelper.get());
 	if (ret)
 		return ret;
 
 	ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,
@@ -167,7 +166,6 @@  int Agc::init(IPAContext &context, const ValueNode &tuningData)
 int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 {
 	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
 		.sensorInfo = context.sensorInfo,
 		.sensorControls = context.sensorControls,
 		.ctrlMap = context.ctrlMap,