[v5,45/47] ipa: libipa: agc: Work without `CameraSensorHelper`

27824 diff mbox series

`AgcMeanLuminance` can operate without a `CameraSensorHelper`, but in that
case it assumes an "ideal" gain model, where gains are truly continuous,
and a gain of x will apply a gain of exactly x to the result. This is not
a good fit when only gain codes are available.

So now that the agc algorithm from the simple ipa has been extracted (AgcMSV),
let's use that in `AgcAlgorithm` to be able to always provide some level of
automatic exposure/gain control. Even though the main use case for operating
without a known gain model is empirically determining the gain model using
the manual controls.

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

Hi Barnabás

On Mon, Aug 17, 2026 at 01:43:46PM +0200, Barnabás Pőcze wrote:
> `AgcMeanLuminance` can operate without a `CameraSensorHelper`, but in that
> case it assumes an "ideal" gain model, where gains are truly continuous,
> and a gain of x will apply a gain of exactly x to the result. This is not
> a good fit when only gain codes are available.
>
> So now that the agc algorithm from the simple ipa has been extracted (AgcMSV),
> let's use that in `AgcAlgorithm` to be able to always provide some level of
> automatic exposure/gain control. Even though the main use case for operating
> without a known gain model is empirically determining the gain model using
> the manual controls.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
>  src/ipa/libipa/agc.cpp              | 202 +++++++++++++++++++---------
>  src/ipa/libipa/agc.h                |   9 +-
>  src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
>  src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
>  5 files changed, 145 insertions(+), 78 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index c9ea02ed00..975f82eaf3 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 ff1e8d5a84..d327548979 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>
> @@ -79,6 +81,9 @@ namespace 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
>   *
> @@ -217,9 +222,6 @@ namespace 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
>   *
> @@ -264,11 +266,18 @@ namespace 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;
>  }
> @@ -303,10 +312,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 << ']'
> @@ -345,28 +358,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;
>
> @@ -417,25 +423,57 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	add(controls::AnalogueGainMode,
>  	    controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>
> -	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);
> -		}
> -	}
> +	std::visit(utils::overloaded{
> +		[&](AgcMSV&) {

nit: a space before & ?

> +			/* no constraint/exposure mode support */
> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;

I still feel it would be trivial to implement impl.constraintModes()
and impl.exposureModeHelpers() for MSV to return Normal and move this
part out of the vist() overload. Anyway, maybe on top

> +
> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */

You might want to explain what the hack is

> +
> +			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) {

		[&](AgcMeanLuminance &impl) {

?

> +			state.constraintMode =
> +				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);

			state.constraintMode =
				static_cast<controls::AeConstraintModeEnum>
				(impl.constraintModes().begin()->first);

Is awful to read, but the line above is veery long

> +			state.exposureMode =
> +				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> +
> +			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> +
> +			ASSERT(sensor_);

I don't see how can this not be true

> +			impl.configure(session.lineDuration, sensor_);
> +			impl.resetFrameCount();
> +
> +			if (session.autoAllowed) {

Also, not sure if this would work as intended but:

		if (!session.autoAllowed)
                        break;

might save you some tabs.

> +				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);

5 indentation levels not to declare two std::vector<ControlValue> ?

> +				}
> +
> +				{
> +					std::vector<ControlValue> options;
> +					for (const auto &[id, _] : impl.exposureModeHelpers())
> +						options.emplace_back(id);
> +
> +					config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> +				}
> +			}
> +		},
> +	}, impl_);
>
>  	return 0;
>  }
> @@ -625,36 +663,68 @@ 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_);
> +
> +	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>
>  	LOG(Agc, Debug)
> -		<< "exposure-time:" << newEv.exposureTime
> +		<< "exposure-time:" << newExposureTime
>  		<< " analogue-gain:" << state.automatic.gain
>  		<< " quantization-gain:" << state.automatic.quantizationGain
>  		<< " digital-gain:" << state.automatic.digitalGain;
> @@ -664,7 +734,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  	 * the minimum frame duration when we have short exposures.
>  	 */
>  	processFrameDuration(session, frameContext,
> -			     std::max(frameContext.minFrameDuration, newEv.exposureTime));
> +			     std::max(frameContext.minFrameDuration, newExposureTime));
>
>  	fillMetadata(session, frameContext, metadata);
>  }
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 612ac539da..b31f7de8fa 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"
>
> @@ -32,6 +34,7 @@ struct Session {
>  	utils::Duration maxExposureTime;
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
> +	double defAnalogueGain;
>  	utils::Duration minFrameDuration;
>  	utils::Duration maxFrameDuration;
>  	utils::Duration lineDuration;
> @@ -113,7 +116,6 @@ class AgcAlgorithm
>  {
>  public:
>  	struct ConfigurationParams {
> -		const CameraSensorHelper *sensor;
>  		const IPACameraSensorInfo &sensorInfo;
>  		const ControlInfoMap &sensorControls;
>  		ControlInfoMap::Map &ctrlMap;
> @@ -129,7 +131,7 @@ public:
>  		double lux = 0;
>  	};
>
> -	int init(const ValueNode &tuningData);
> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
>
>  	int configure(agc::Session &session, agc::ActiveState &state,
>  		      const ConfigurationParams &config);
> @@ -151,7 +153,8 @@ private:
>  			  const agc::FrameContext &frameContext,
>  			  ControlList &metadata);
>
> -	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 4b675eb7e5..140f34ad50 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 4c2a066e86..41a8cd581f 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(),

One more argument to pass all parameters required by configure() to
init() and call it internally.

Anyway, the only thing that really matters to me is a longer
explanation on the hack, as this is really the only thing that is
obscure to me.

The rest, as said, is mostly about tastes. So with the hack explained:

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

>  		.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,
> --
> 2.55.0
>

2026. 08. 17. 17:45 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Aug 17, 2026 at 01:43:46PM +0200, Barnabás Pőcze wrote:
>> `AgcMeanLuminance` can operate without a `CameraSensorHelper`, but in that
>> case it assumes an "ideal" gain model, where gains are truly continuous,
>> and a gain of x will apply a gain of exactly x to the result. This is not
>> a good fit when only gain codes are available.
>>
>> So now that the agc algorithm from the simple ipa has been extracted (AgcMSV),
>> let's use that in `AgcAlgorithm` to be able to always provide some level of
>> automatic exposure/gain control. Even though the main use case for operating
>> without a known gain model is empirically determining the gain model using
>> the manual controls.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
>>   src/ipa/libipa/agc.cpp              | 202 +++++++++++++++++++---------
>>   src/ipa/libipa/agc.h                |   9 +-
>>   src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
>>   src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
>>   5 files changed, 145 insertions(+), 78 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index c9ea02ed00..975f82eaf3 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 ff1e8d5a84..d327548979 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>
>> @@ -79,6 +81,9 @@ namespace 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
>>    *
>> @@ -217,9 +222,6 @@ namespace 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
>>    *
>> @@ -264,11 +266,18 @@ namespace 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;
>>   }
>> @@ -303,10 +312,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 << ']'
>> @@ -345,28 +358,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;
>>
>> @@ -417,25 +423,57 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   	add(controls::AnalogueGainMode,
>>   	    controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>>
>> -	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);
>> -		}
>> -	}
>> +	std::visit(utils::overloaded{
>> +		[&](AgcMSV&) {
> 
> nit: a space before & ?

Done.


> 
>> +			/* no constraint/exposure mode support */
>> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
>> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> 
> I still feel it would be trivial to implement impl.constraintModes()
> and impl.exposureModeHelpers() for MSV to return Normal and move this
> part out of the vist() overload. Anyway, maybe on top

Possibly, yes, but I feel like creating `std::map`s and all that seems like a big hassle.


> 
>> +
>> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
> 
> You might want to explain what the hack is

It's not necessary, and probably incorrect, so I'll remove this. This comes from
the `kExposureOptimal` constant in that algorithm.


> 
>> +
>> +			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) {
> 
> 		[&](AgcMeanLuminance &impl) {
> 
> ?

Done.


> 
>> +			state.constraintMode =
>> +				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> 
> 			state.constraintMode =
> 				static_cast<controls::AeConstraintModeEnum>
> 				(impl.constraintModes().begin()->first);
> 
> Is awful to read, but the line above is veery long

state.constraintMode = static_cast<...>(
   ...);

?



> 
>> +			state.exposureMode =
>> +				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
>> +
>> +			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
>> +
>> +			ASSERT(sensor_);
> 
> I don't see how can this not be true

Isn't that the point of an assertion? To validate the underlying
assumptions?


> 
>> +			impl.configure(session.lineDuration, sensor_);
>> +			impl.resetFrameCount();
>> +
>> +			if (session.autoAllowed) {
> 
> Also, not sure if this would work as intended but:
> 
> 		if (!session.autoAllowed)
>                          break;

`return` would work.


> 
> might save you some tabs.
> 
>> +				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);
> 
> 5 indentation levels not to declare two std::vector<ControlValue> ?

I have removed the indentation.


> 
>> +				}
>> +
>> +				{
>> +					std::vector<ControlValue> options;
>> +					for (const auto &[id, _] : impl.exposureModeHelpers())
>> +						options.emplace_back(id);
>> +
>> +					config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
>> +				}
>> +			}
>> +		},
>> +	}, impl_);
>>
>>   	return 0;
>>   }
>> @@ -625,36 +663,68 @@ 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_);
>> +
>> +	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>>
>>   	LOG(Agc, Debug)
>> -		<< "exposure-time:" << newEv.exposureTime
>> +		<< "exposure-time:" << newExposureTime
>>   		<< " analogue-gain:" << state.automatic.gain
>>   		<< " quantization-gain:" << state.automatic.quantizationGain
>>   		<< " digital-gain:" << state.automatic.digitalGain;
>> @@ -664,7 +734,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>   	 * the minimum frame duration when we have short exposures.
>>   	 */
>>   	processFrameDuration(session, frameContext,
>> -			     std::max(frameContext.minFrameDuration, newEv.exposureTime));
>> +			     std::max(frameContext.minFrameDuration, newExposureTime));
>>
>>   	fillMetadata(session, frameContext, metadata);
>>   }
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> index 612ac539da..b31f7de8fa 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"
>>
>> @@ -32,6 +34,7 @@ struct Session {
>>   	utils::Duration maxExposureTime;
>>   	double minAnalogueGain;
>>   	double maxAnalogueGain;
>> +	double defAnalogueGain;
>>   	utils::Duration minFrameDuration;
>>   	utils::Duration maxFrameDuration;
>>   	utils::Duration lineDuration;
>> @@ -113,7 +116,6 @@ class AgcAlgorithm
>>   {
>>   public:
>>   	struct ConfigurationParams {
>> -		const CameraSensorHelper *sensor;
>>   		const IPACameraSensorInfo &sensorInfo;
>>   		const ControlInfoMap &sensorControls;
>>   		ControlInfoMap::Map &ctrlMap;
>> @@ -129,7 +131,7 @@ public:
>>   		double lux = 0;
>>   	};
>>
>> -	int init(const ValueNode &tuningData);
>> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
>>
>>   	int configure(agc::Session &session, agc::ActiveState &state,
>>   		      const ConfigurationParams &config);
>> @@ -151,7 +153,8 @@ private:
>>   			  const agc::FrameContext &frameContext,
>>   			  ControlList &metadata);
>>
>> -	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 4b675eb7e5..140f34ad50 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 4c2a066e86..41a8cd581f 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(),
> 
> One more argument to pass all parameters required by configure() to
> init() and call it internally.

Possibly. But I would like to wait for some discussion on the handling
of controls in ipa modules before making that change.


> 
> Anyway, the only thing that really matters to me is a longer
> explanation on the hack, as this is really the only thing that is
> obscure to me.
> 
> The rest, as said, is mostly about tastes. So with the hack explained:
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>>   		.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,
>> --
>> 2.55.0
>>

Hi Barnabás

On Tue, Aug 18, 2026 at 03:34:03PM +0200, Barnabás Pőcze wrote:
> 2026. 08. 17. 17:45 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Mon, Aug 17, 2026 at 01:43:46PM +0200, Barnabás Pőcze wrote:
> > > `AgcMeanLuminance` can operate without a `CameraSensorHelper`, but in that
> > > case it assumes an "ideal" gain model, where gains are truly continuous,
> > > and a gain of x will apply a gain of exactly x to the result. This is not
> > > a good fit when only gain codes are available.
> > >
> > > So now that the agc algorithm from the simple ipa has been extracted (AgcMSV),
> > > let's use that in `AgcAlgorithm` to be able to always provide some level of
> > > automatic exposure/gain control. Even though the main use case for operating
> > > without a known gain model is empirically determining the gain model using
> > > the manual controls.
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
> > >   src/ipa/libipa/agc.cpp              | 202 +++++++++++++++++++---------
> > >   src/ipa/libipa/agc.h                |   9 +-
> > >   src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
> > >   src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
> > >   5 files changed, 145 insertions(+), 78 deletions(-)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index c9ea02ed00..975f82eaf3 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 ff1e8d5a84..d327548979 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>
> > > @@ -79,6 +81,9 @@ namespace 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
> > >    *
> > > @@ -217,9 +222,6 @@ namespace 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
> > >    *
> > > @@ -264,11 +266,18 @@ namespace 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;
> > >   }
> > > @@ -303,10 +312,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 << ']'
> > > @@ -345,28 +358,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;
> > >
> > > @@ -417,25 +423,57 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> > >   	add(controls::AnalogueGainMode,
> > >   	    controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
> > >
> > > -	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);
> > > -		}
> > > -	}
> > > +	std::visit(utils::overloaded{
> > > +		[&](AgcMSV&) {
> >
> > nit: a space before & ?
>
> Done.
>
>
> >
> > > +			/* no constraint/exposure mode support */
> > > +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > > +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> >
> > I still feel it would be trivial to implement impl.constraintModes()
> > and impl.exposureModeHelpers() for MSV to return Normal and move this
> > part out of the vist() overload. Anyway, maybe on top
>
> Possibly, yes, but I feel like creating `std::map`s and all that seems like a big hassle.
>
>
> >
> > > +
> > > +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
> >
> > You might want to explain what the hack is
>
> It's not necessary, and probably incorrect, so I'll remove this. This comes from
> the `kExposureOptimal` constant in that algorithm.
>
>
> >
> > > +
> > > +			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) {
> >
> > 		[&](AgcMeanLuminance &impl) {
> >
> > ?
>
> Done.
>
>
> >
> > > +			state.constraintMode =
> > > +				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
> >
> > 			state.constraintMode =
> > 				static_cast<controls::AeConstraintModeEnum>
> > 				(impl.constraintModes().begin()->first);
> >
> > Is awful to read, but the line above is veery long
>
> state.constraintMode = static_cast<...>(
>   ...);
>
> ?
>
>
>
> >
> > > +			state.exposureMode =
> > > +				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
> > > +
> > > +			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
> > > +
> > > +			ASSERT(sensor_);
> >
> > I don't see how can this not be true
>
> Isn't that the point of an assertion? To validate the underlying
> assumptions?
>

As I use them, assertions should validate conditions that should be
respected between different interacting components. It's an API
contract enforcement if you will. They should be used to catch
developments error as early as possible, if a user mis-uses an
interface, in example.

Here, this very class does in init()

	if (sensor) {
		auto &impl = impl_.emplace<AgcMeanLuminance>();
		int ret = impl.parseTuningData(tuningData);
		if (ret)
			return ret;
	} else {
		impl_.emplace<AgcMSV>();
	}

as we're in the:

	[&](AgcMeanLuminance& impl) {

        }

overload path, I don't see how this can't be true.

Anyway, it's a detail, and more safety is probably better (let me
argue that's not always the case, as if I see an assertion here I will
start wondering "what can make sensor_ nullptr between init() and
here?)

I'm ok with all the other comments

>
> >
> > > +			impl.configure(session.lineDuration, sensor_);
> > > +			impl.resetFrameCount();
> > > +
> > > +			if (session.autoAllowed) {
> >
> > Also, not sure if this would work as intended but:
> >
> > 		if (!session.autoAllowed)
> >                          break;
>
> `return` would work.
>
>
> >
> > might save you some tabs.
> >
> > > +				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);
> >
> > 5 indentation levels not to declare two std::vector<ControlValue> ?
>
> I have removed the indentation.
>
>
> >
> > > +				}
> > > +
> > > +				{
> > > +					std::vector<ControlValue> options;
> > > +					for (const auto &[id, _] : impl.exposureModeHelpers())
> > > +						options.emplace_back(id);
> > > +
> > > +					config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> > > +				}
> > > +			}
> > > +		},
> > > +	}, impl_);
> > >
> > >   	return 0;
> > >   }
> > > @@ -625,36 +663,68 @@ 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_);
> > > +
> > > +	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > >
> > >   	LOG(Agc, Debug)
> > > -		<< "exposure-time:" << newEv.exposureTime
> > > +		<< "exposure-time:" << newExposureTime
> > >   		<< " analogue-gain:" << state.automatic.gain
> > >   		<< " quantization-gain:" << state.automatic.quantizationGain
> > >   		<< " digital-gain:" << state.automatic.digitalGain;
> > > @@ -664,7 +734,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >   	 * the minimum frame duration when we have short exposures.
> > >   	 */
> > >   	processFrameDuration(session, frameContext,
> > > -			     std::max(frameContext.minFrameDuration, newEv.exposureTime));
> > > +			     std::max(frameContext.minFrameDuration, newExposureTime));
> > >
> > >   	fillMetadata(session, frameContext, metadata);
> > >   }
> > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > index 612ac539da..b31f7de8fa 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"
> > >
> > > @@ -32,6 +34,7 @@ struct Session {
> > >   	utils::Duration maxExposureTime;
> > >   	double minAnalogueGain;
> > >   	double maxAnalogueGain;
> > > +	double defAnalogueGain;
> > >   	utils::Duration minFrameDuration;
> > >   	utils::Duration maxFrameDuration;
> > >   	utils::Duration lineDuration;
> > > @@ -113,7 +116,6 @@ class AgcAlgorithm
> > >   {
> > >   public:
> > >   	struct ConfigurationParams {
> > > -		const CameraSensorHelper *sensor;
> > >   		const IPACameraSensorInfo &sensorInfo;
> > >   		const ControlInfoMap &sensorControls;
> > >   		ControlInfoMap::Map &ctrlMap;
> > > @@ -129,7 +131,7 @@ public:
> > >   		double lux = 0;
> > >   	};
> > >
> > > -	int init(const ValueNode &tuningData);
> > > +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
> > >
> > >   	int configure(agc::Session &session, agc::ActiveState &state,
> > >   		      const ConfigurationParams &config);
> > > @@ -151,7 +153,8 @@ private:
> > >   			  const agc::FrameContext &frameContext,
> > >   			  ControlList &metadata);
> > >
> > > -	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 4b675eb7e5..140f34ad50 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 4c2a066e86..41a8cd581f 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(),
> >
> > One more argument to pass all parameters required by configure() to
> > init() and call it internally.
>
> Possibly. But I would like to wait for some discussion on the handling
> of controls in ipa modules before making that change.
>
>
> >
> > Anyway, the only thing that really matters to me is a longer
> > explanation on the hack, as this is really the only thing that is
> > obscure to me.
> >
> > The rest, as said, is mostly about tastes. So with the hack explained:
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > >   		.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,
> > > --
> > > 2.55.0
> > >
>

2026. 08. 19. 9:17 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Tue, Aug 18, 2026 at 03:34:03PM +0200, Barnabás Pőcze wrote:
>> 2026. 08. 17. 17:45 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Mon, Aug 17, 2026 at 01:43:46PM +0200, Barnabás Pőcze wrote:
>>>> `AgcMeanLuminance` can operate without a `CameraSensorHelper`, but in that
>>>> case it assumes an "ideal" gain model, where gains are truly continuous,
>>>> and a gain of x will apply a gain of exactly x to the result. This is not
>>>> a good fit when only gain codes are available.
>>>>
>>>> So now that the agc algorithm from the simple ipa has been extracted (AgcMSV),
>>>> let's use that in `AgcAlgorithm` to be able to always provide some level of
>>>> automatic exposure/gain control. Even though the main use case for operating
>>>> without a known gain model is empirically determining the gain model using
>>>> the manual controls.
>>>>
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    src/ipa/ipu3/algorithms/agc.cpp     |   4 +-
>>>>    src/ipa/libipa/agc.cpp              | 202 +++++++++++++++++++---------
>>>>    src/ipa/libipa/agc.h                |   9 +-
>>>>    src/ipa/mali-c55/algorithms/agc.cpp |   4 +-
>>>>    src/ipa/rkisp1/algorithms/agc.cpp   |   4 +-
>>>>    5 files changed, 145 insertions(+), 78 deletions(-)
>>>>
>>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>>>> index c9ea02ed00..975f82eaf3 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 ff1e8d5a84..d327548979 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>
>>>> @@ -79,6 +81,9 @@ namespace 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
>>>>     *
>>>> @@ -217,9 +222,6 @@ namespace 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
>>>>     *
>>>> @@ -264,11 +266,18 @@ namespace 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;
>>>>    }
>>>> @@ -303,10 +312,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 << ']'
>>>> @@ -345,28 +358,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;
>>>>
>>>> @@ -417,25 +423,57 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>>>    	add(controls::AnalogueGainMode,
>>>>    	    controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>>>>
>>>> -	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);
>>>> -		}
>>>> -	}
>>>> +	std::visit(utils::overloaded{
>>>> +		[&](AgcMSV&) {
>>>
>>> nit: a space before & ?
>>
>> Done.
>>
>>
>>>
>>>> +			/* no constraint/exposure mode support */
>>>> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
>>>> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>>>
>>> I still feel it would be trivial to implement impl.constraintModes()
>>> and impl.exposureModeHelpers() for MSV to return Normal and move this
>>> part out of the vist() overload. Anyway, maybe on top
>>
>> Possibly, yes, but I feel like creating `std::map`s and all that seems like a big hassle.
>>
>>
>>>
>>>> +
>>>> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
>>>
>>> You might want to explain what the hack is
>>
>> It's not necessary, and probably incorrect, so I'll remove this. This comes from
>> the `kExposureOptimal` constant in that algorithm.
>>
>>
>>>
>>>> +
>>>> +			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) {
>>>
>>> 		[&](AgcMeanLuminance &impl) {
>>>
>>> ?
>>
>> Done.
>>
>>
>>>
>>>> +			state.constraintMode =
>>>> +				static_cast<controls::AeConstraintModeEnum>(impl.constraintModes().begin()->first);
>>>
>>> 			state.constraintMode =
>>> 				static_cast<controls::AeConstraintModeEnum>
>>> 				(impl.constraintModes().begin()->first);
>>>
>>> Is awful to read, but the line above is veery long
>>
>> state.constraintMode = static_cast<...>(
>>    ...);
>>
>> ?
>>
>>
>>
>>>
>>>> +			state.exposureMode =
>>>> +				static_cast<controls::AeExposureModeEnum>(impl.exposureModeHelpers().begin()->first);
>>>> +
>>>> +			state.automatic.yTarget = impl.effectiveYTarget(0, 1);
>>>> +
>>>> +			ASSERT(sensor_);
>>>
>>> I don't see how can this not be true
>>
>> Isn't that the point of an assertion? To validate the underlying
>> assumptions?
>>
> 
> As I use them, assertions should validate conditions that should be
> respected between different interacting components. It's an API
> contract enforcement if you will. They should be used to catch
> developments error as early as possible, if a user mis-uses an
> interface, in example.
> 
> Here, this very class does in init()
> 
> 	if (sensor) {
> 		auto &impl = impl_.emplace<AgcMeanLuminance>();
> 		int ret = impl.parseTuningData(tuningData);
> 		if (ret)
> 			return ret;
> 	} else {
> 		impl_.emplace<AgcMSV>();
> 	}
> 
> as we're in the:
> 
> 	[&](AgcMeanLuminance& impl) {
> 
>          }
> 
> overload path, I don't see how this can't be true.
> 
> Anyway, it's a detail, and more safety is probably better (let me
> argue that's not always the case, as if I see an assertion here I will
> start wondering "what can make sensor_ nullptr between init() and
> here?)

Ok, removed.


> 
> I'm ok with all the other comments
> 
>>
>>>
>>>> +			impl.configure(session.lineDuration, sensor_);
>>>> +			impl.resetFrameCount();
>>>> +
>>>> +			if (session.autoAllowed) {
>>>
>>> Also, not sure if this would work as intended but:
>>>
>>> 		if (!session.autoAllowed)
>>>                           break;
>>
>> `return` would work.
>>
>>
>>>
>>> might save you some tabs.
>>>
>>>> +				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);
>>>
>>> 5 indentation levels not to declare two std::vector<ControlValue> ?
>>
>> I have removed the indentation.
>>
>>
>>>
>>>> +				}
>>>> +
>>>> +				{
>>>> +					std::vector<ControlValue> options;
>>>> +					for (const auto &[id, _] : impl.exposureModeHelpers())
>>>> +						options.emplace_back(id);
>>>> +
>>>> +					config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
>>>> +				}
>>>> +			}
>>>> +		},
>>>> +	}, impl_);
>>>>
>>>>    	return 0;
>>>>    }
>>>> @@ -625,36 +663,68 @@ 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_);
>>>> +
>>>> +	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>>>>
>>>>    	LOG(Agc, Debug)
>>>> -		<< "exposure-time:" << newEv.exposureTime
>>>> +		<< "exposure-time:" << newExposureTime
>>>>    		<< " analogue-gain:" << state.automatic.gain
>>>>    		<< " quantization-gain:" << state.automatic.quantizationGain
>>>>    		<< " digital-gain:" << state.automatic.digitalGain;
>>>> @@ -664,7 +734,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>>>    	 * the minimum frame duration when we have short exposures.
>>>>    	 */
>>>>    	processFrameDuration(session, frameContext,
>>>> -			     std::max(frameContext.minFrameDuration, newEv.exposureTime));
>>>> +			     std::max(frameContext.minFrameDuration, newExposureTime));
>>>>
>>>>    	fillMetadata(session, frameContext, metadata);
>>>>    }
>>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>>>> index 612ac539da..b31f7de8fa 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"
>>>>
>>>> @@ -32,6 +34,7 @@ struct Session {
>>>>    	utils::Duration maxExposureTime;
>>>>    	double minAnalogueGain;
>>>>    	double maxAnalogueGain;
>>>> +	double defAnalogueGain;
>>>>    	utils::Duration minFrameDuration;
>>>>    	utils::Duration maxFrameDuration;
>>>>    	utils::Duration lineDuration;
>>>> @@ -113,7 +116,6 @@ class AgcAlgorithm
>>>>    {
>>>>    public:
>>>>    	struct ConfigurationParams {
>>>> -		const CameraSensorHelper *sensor;
>>>>    		const IPACameraSensorInfo &sensorInfo;
>>>>    		const ControlInfoMap &sensorControls;
>>>>    		ControlInfoMap::Map &ctrlMap;
>>>> @@ -129,7 +131,7 @@ public:
>>>>    		double lux = 0;
>>>>    	};
>>>>
>>>> -	int init(const ValueNode &tuningData);
>>>> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor);
>>>>
>>>>    	int configure(agc::Session &session, agc::ActiveState &state,
>>>>    		      const ConfigurationParams &config);
>>>> @@ -151,7 +153,8 @@ private:
>>>>    			  const agc::FrameContext &frameContext,
>>>>    			  ControlList &metadata);
>>>>
>>>> -	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 4b675eb7e5..140f34ad50 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 4c2a066e86..41a8cd581f 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(),
>>>
>>> One more argument to pass all parameters required by configure() to
>>> init() and call it internally.
>>
>> Possibly. But I would like to wait for some discussion on the handling
>> of controls in ipa modules before making that change.
>>
>>
>>>
>>> Anyway, the only thing that really matters to me is a longer
>>> explanation on the hack, as this is really the only thing that is
>>> obscure to me.
>>>
>>> The rest, as said, is mostly about tastes. So with the hack explained:
>>>
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>
>>>>    		.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,
>>>> --
>>>> 2.55.0
>>>>
>>