[v4,46/49] ipa: libipa: agc: Work without `CameraSensorHelper`

27718 diff mbox series

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(-)

Hi Barnabás

On Mon, Aug 10, 2026 at 12:38:42PM +0200, Barnabás Pőcze wrote:
> Use the agc algorithm extracted from the simple ipa module (AgcMSV)
> to provide some kind of operation when a `CameraSensorHelper` is

is "some kind" intentional or did you mean "the same kind" ?

> 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 117559eb76..bc57179d26 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>();
> +	}
> +

I know this is something we disagreed on when discussing the design.

I would have preferred a pure virtual base class where a common
interface would have been defined. And if MSV would have to implement
an empty parseTuningData() and configure() being able to write:

        if (sensor)
                impl = mean;
        else
                impl = msv;

        ret = impl->parseTuningData();
        if (ret)
                return ret;

and reduce the amount of conditional code in this cassl was worth it imho.

This would require unifying:

msv:
	struct Limits {
		std::array<uint32_t, 2> exposure;
		std::array<double, 2> gain;
		double gainMinStep;
		double gain1;
	};

mean_luminance:
	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
		       double minGain, double maxGain, std::vector<AgcConstraint> constraints);

msv:
	struct Params {
		const Histogram &yHist;
		uint32_t exposure;
		double gain;
	};

mean_luminance:
	struct Params {
		const Traits &traits;
		const Histogram &yHist;
		utils::Duration effectiveExposureValue;
		uint32_t constraintModeIndex;
		uint32_t exposureModeIndex;
		double lux = 0;
		double exposureCompensation = 1;
	};

which I know you don't like.

You're too far in the design and implementation already, and I'm
certainly not asking to reconsider this, but I felt like mentioning
it in case someone else feels the same.

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

As I presume you know I consider std::visit()'s three levels
indentation worse in practice compared to a simpler if (sensor_) or
similar.

> +			/* no constraint/exposure mode support */

I would dare to say that one can say that AgcMsv only implements
support for ConstraintNormal and ExposureNormal in its implementation
of constaintModes() and exposureModeHelpers(), so you could move this
to the common part of the code.

> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> +
> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */

does this come from existing code or is this new ?

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

How can this possible be not true ?

> +			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) {
>  		/* The IPA control maps keep their states, so the removal is necessary. */
>
>  		config.ctrlMap.erase(&controls::ExposureValue);
> @@ -601,33 +641,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,

Do you expect this to change ? If it stays 1, why would you need a
parameter for it ?

> +					/* 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.

please reflow to shorter lines

> +				 */
> +				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 d02dbff562..1babf0762d 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"
>
> @@ -56,6 +58,7 @@ struct Session {
>  	utils::Duration maxExposureTime;
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
> +	double defAnalogueGain;
>  	utils::Duration minFrameDuration;
>  	utils::Duration maxFrameDuration;
>
> @@ -114,14 +117,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);
> @@ -146,7 +148,8 @@ public:
>  		     ControlList &metadata);
>
>  private:
> -	AgcMeanLuminance impl_;
> +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
> +	CameraSensorHelper *sensor_ = nullptr;

Would an optional<> help or is it an overkill ?

>  };
>
>  } /* 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,
> --
> 2.55.0
>

2026. 08. 10. 16:57 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Aug 10, 2026 at 12:38:42PM +0200, Barnabás Pőcze wrote:
>> Use the agc algorithm extracted from the simple ipa module (AgcMSV)
>> to provide some kind of operation when a `CameraSensorHelper` is
> 
> is "some kind" intentional or did you mean "the same kind" ?

It is intentional. The intent is to express that even in the absence of
a `CameraSensorHelper`, a rudimentary auto exposure/gain should work.

Maybe it's enough to say "[...] to provide auto exposure/gain
even in the absence of a `CameraSensorHelper` (that is, when
`AgcMeanLuminance` cannot be used)."


> 
>> 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 117559eb76..bc57179d26 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>();
>> +	}
>> +
> 
> I know this is something we disagreed on when discussing the design.
> 
> I would have preferred a pure virtual base class where a common
> interface would have been defined. And if MSV would have to implement
> an empty parseTuningData() and configure() being able to write:
> 
>          if (sensor)
>                  impl = mean;
>          else
>                  impl = msv;
> 
>          ret = impl->parseTuningData();
>          if (ret)
>                  return ret;
> 
> and reduce the amount of conditional code in this cassl was worth it imho.
> 
> This would require unifying:
> 
> msv:
> 	struct Limits {
> 		std::array<uint32_t, 2> exposure;
> 		std::array<double, 2> gain;
> 		double gainMinStep;
> 		double gain1;
> 	};
> 
> mean_luminance:
> 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> 		       double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> 
> msv:
> 	struct Params {
> 		const Histogram &yHist;
> 		uint32_t exposure;
> 		double gain;
> 	};
> 
> mean_luminance:
> 	struct Params {
> 		const Traits &traits;
> 		const Histogram &yHist;
> 		utils::Duration effectiveExposureValue;
> 		uint32_t constraintModeIndex;
> 		uint32_t exposureModeIndex;
> 		double lux = 0;
> 		double exposureCompensation = 1;
> 	};
> 
> which I know you don't like.
> 
> You're too far in the design and implementation already, and I'm
> certainly not asking to reconsider this, but I felt like mentioning
> it in case someone else feels the same.

That's a fair point, my reasoning is as follows. I would very much postpone
the design of an appropriate interface until there are 3 or more implementations.
With these two, in my opinion, it's easier to handle the differences in the "user",
and all that without having to design an interface, the various data structures,
and having to fit the implementations to the interface.


> 
>> +	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&) {
> 
> As I presume you know I consider std::visit()'s three levels
> indentation worse in practice compared to a simpler if (sensor_) or
> similar.

The alternative would be an `std::get_if` chain, which would save 1 level.


> 
>> +			/* no constraint/exposure mode support */
> 
> I would dare to say that one can say that AgcMsv only implements
> support for ConstraintNormal and ExposureNormal in its implementation
> of constaintModes() and exposureModeHelpers(), so you could move this
> to the common part of the code.
> 
>> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
>> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>> +
>> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
> 
> does this come from existing code or is this new ?

Well, it's more of my interpretation of the algorithm, it's probably not too useful to set it.
The only user is the rkisp1 wdr algorithm, but that sets `AgcMeanLuminance::AgcConstraint`s,
so it wouldn't work the AgcMSV in any case. I'm not sure that functionality can be reasonably
implemented with just gain codes. So the idea there was to reject wdr is a camera sensor helper
is not found.


> 
>> +
>> +			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_);
> 
> How can this possible be not true ?

It should be non-nullptr, hence the assertion.


> 
>> +			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) {
>>   		/* The IPA control maps keep their states, so the removal is necessary. */
>>
>>   		config.ctrlMap.erase(&controls::ExposureValue);
>> @@ -601,33 +641,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,
> 
> Do you expect this to change ? If it stays 1, why would you need a
> parameter for it ?

At this patch, the simple pipeline handler uses `AgcMSV` directly, even
with a `CameraSensorHelper`, and in that case it sets `gainMinStep` to
something based on the true gain values.

Arguably, after the next patch, this parameter will only be 1, so it
could be removed.


> 
>> +					/* 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.
> 
> please reflow to shorter lines

How short are you thinking? Enough to move the "was" to the next line?


> 
>> +				 */
>> +				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 d02dbff562..1babf0762d 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"
>>
>> @@ -56,6 +58,7 @@ struct Session {
>>   	utils::Duration maxExposureTime;
>>   	double minAnalogueGain;
>>   	double maxAnalogueGain;
>> +	double defAnalogueGain;
>>   	utils::Duration minFrameDuration;
>>   	utils::Duration maxFrameDuration;
>>
>> @@ -114,14 +117,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);
>> @@ -146,7 +148,8 @@ public:
>>   		     ControlList &metadata);
>>
>>   private:
>> -	AgcMeanLuminance impl_;
>> +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
>> +	CameraSensorHelper *sensor_ = nullptr;
> 
> Would an optional<> help or is it an overkill ?

Can you clarify what you mean exactly? For `sensor_` or `impl_` or?


> 
>>   };
>>
>>   } /* 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,
>> --
>> 2.55.0
>>

Hi Barnabás

On Mon, Aug 10, 2026 at 05:44:40PM +0200, Barnabás Pőcze wrote:
> 2026. 08. 10. 16:57 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Mon, Aug 10, 2026 at 12:38:42PM +0200, Barnabás Pőcze wrote:
> > > Use the agc algorithm extracted from the simple ipa module (AgcMSV)
> > > to provide some kind of operation when a `CameraSensorHelper` is
> >
> > is "some kind" intentional or did you mean "the same kind" ?
>
> It is intentional. The intent is to express that even in the absence of
> a `CameraSensorHelper`, a rudimentary auto exposure/gain should work.
>
> Maybe it's enough to say "[...] to provide auto exposure/gain
> even in the absence of a `CameraSensorHelper` (that is, when
> `AgcMeanLuminance` cannot be used)."

Ah ok, thanks for clarifying. yeah I like the wording better

>
>
> >
> > > 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 117559eb76..bc57179d26 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>();
> > > +	}
> > > +
> >
> > I know this is something we disagreed on when discussing the design.
> >
> > I would have preferred a pure virtual base class where a common
> > interface would have been defined. And if MSV would have to implement
> > an empty parseTuningData() and configure() being able to write:
> >
> >          if (sensor)
> >                  impl = mean;
> >          else
> >                  impl = msv;
> >
> >          ret = impl->parseTuningData();
> >          if (ret)
> >                  return ret;
> >
> > and reduce the amount of conditional code in this cassl was worth it imho.
> >
> > This would require unifying:
> >
> > msv:
> > 	struct Limits {
> > 		std::array<uint32_t, 2> exposure;
> > 		std::array<double, 2> gain;
> > 		double gainMinStep;
> > 		double gain1;
> > 	};
> >
> > mean_luminance:
> > 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> > 		       double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> >
> > msv:
> > 	struct Params {
> > 		const Histogram &yHist;
> > 		uint32_t exposure;
> > 		double gain;
> > 	};
> >
> > mean_luminance:
> > 	struct Params {
> > 		const Traits &traits;
> > 		const Histogram &yHist;
> > 		utils::Duration effectiveExposureValue;
> > 		uint32_t constraintModeIndex;
> > 		uint32_t exposureModeIndex;
> > 		double lux = 0;
> > 		double exposureCompensation = 1;
> > 	};
> >
> > which I know you don't like.
> >
> > You're too far in the design and implementation already, and I'm
> > certainly not asking to reconsider this, but I felt like mentioning
> > it in case someone else feels the same.
>
> That's a fair point, my reasoning is as follows. I would very much postpone
> the design of an appropriate interface until there are 3 or more implementations.
> With these two, in my opinion, it's easier to handle the differences in the "user",
> and all that without having to design an interface, the various data structures,
> and having to fit the implementations to the interface.
>

I know, and I guess your arguments are valid.

As said, let's leave it there for the time being, what matters to me
is that we can do the changes on top and we don't corner ourselves.

I wonder if we will ever have a third implementation, but who knows

>
> >
> > > +	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&) {
> >
> > As I presume you know I consider std::visit()'s three levels
> > indentation worse in practice compared to a simpler if (sensor_) or
> > similar.
>
> The alternative would be an `std::get_if` chain, which would save 1 level.
>

I wonder if we sholdn't add rule to do what we do with switch() that
suffers from the same exact indentation issue

	std::visit(utils::overloaded{
	[&](AgcMSV&) {

        },
        [&](AgcMeanLuminance& impl) {

        },


>
> >
> > > +			/* no constraint/exposure mode support */
> >
> > I would dare to say that one can say that AgcMsv only implements
> > support for ConstraintNormal and ExposureNormal in its implementation
> > of constaintModes() and exposureModeHelpers(), so you could move this
> > to the common part of the code.
> >
> > > +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> > > +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
> > > +
> > > +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
> >
> > does this come from existing code or is this new ?
>
> Well, it's more of my interpretation of the algorithm, it's probably not too useful to set it.
> The only user is the rkisp1 wdr algorithm, but that sets `AgcMeanLuminance::AgcConstraint`s,
> so it wouldn't work the AgcMSV in any case. I'm not sure that functionality can be reasonably
> implemented with just gain codes. So the idea there was to reject wdr is a camera sensor helper
> is not found.
>

I will defer this to Stefan which knows Wdr

>
> >
> > > +
> > > +			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_);
> >
> > How can this possible be not true ?
>
> It should be non-nullptr, hence the assertion.

Yeah my point is that you instantiate AgcMeanLuminance only
if (sensor_), don't you ? How can it become a nullptr ?

>
>
> >
> > > +			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) {
> > >   		/* The IPA control maps keep their states, so the removal is necessary. */
> > >
> > >   		config.ctrlMap.erase(&controls::ExposureValue);
> > > @@ -601,33 +641,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,
> >
> > Do you expect this to change ? If it stays 1, why would you need a
> > parameter for it ?
>
> At this patch, the simple pipeline handler uses `AgcMSV` directly, even
> with a `CameraSensorHelper`, and in that case it sets `gainMinStep` to
> something based on the true gain values.
>
> Arguably, after the next patch, this parameter will only be 1, so it
> could be removed.
>

ack

>
> >
> > > +					/* 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.
> >
> > please reflow to shorter lines
>
> How short are you thinking? Enough to move the "was" to the next line?

For comments, I think 80-cols is still the rule

				/*
				 * 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 d02dbff562..1babf0762d 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"
> > >
> > > @@ -56,6 +58,7 @@ struct Session {
> > >   	utils::Duration maxExposureTime;
> > >   	double minAnalogueGain;
> > >   	double maxAnalogueGain;
> > > +	double defAnalogueGain;
> > >   	utils::Duration minFrameDuration;
> > >   	utils::Duration maxFrameDuration;
> > >
> > > @@ -114,14 +117,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);
> > > @@ -146,7 +148,8 @@ public:
> > >   		     ControlList &metadata);
> > >
> > >   private:
> > > -	AgcMeanLuminance impl_;
> > > +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
> > > +	CameraSensorHelper *sensor_ = nullptr;
> >
> > Would an optional<> help or is it an overkill ?
>
> Can you clarify what you mean exactly? For `sensor_` or `impl_` or?
>

I meant for the sensor_, but it doesn't change much and is passed in
as a pointer, so don't bother.

>
> >
> > >   };
> > >
> > >   } /* 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,
> > > --
> > > 2.55.0
> > >
>

2026. 08. 10. 17:57 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Aug 10, 2026 at 05:44:40PM +0200, Barnabás Pőcze wrote:
>> 2026. 08. 10. 16:57 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Mon, Aug 10, 2026 at 12:38:42PM +0200, Barnabás Pőcze wrote:
>>>> Use the agc algorithm extracted from the simple ipa module (AgcMSV)
>>>> to provide some kind of operation when a `CameraSensorHelper` is
>>>
>>> is "some kind" intentional or did you mean "the same kind" ?
>>
>> It is intentional. The intent is to express that even in the absence of
>> a `CameraSensorHelper`, a rudimentary auto exposure/gain should work.
>>
>> Maybe it's enough to say "[...] to provide auto exposure/gain
>> even in the absence of a `CameraSensorHelper` (that is, when
>> `AgcMeanLuminance` cannot be used)."
> 
> Ah ok, thanks for clarifying. yeah I like the wording better
> 
>>
>>
>>>
>>>> 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 117559eb76..bc57179d26 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>();
>>>> +	}
>>>> +
>>>
>>> I know this is something we disagreed on when discussing the design.
>>>
>>> I would have preferred a pure virtual base class where a common
>>> interface would have been defined. And if MSV would have to implement
>>> an empty parseTuningData() and configure() being able to write:
>>>
>>>           if (sensor)
>>>                   impl = mean;
>>>           else
>>>                   impl = msv;
>>>
>>>           ret = impl->parseTuningData();
>>>           if (ret)
>>>                   return ret;
>>>
>>> and reduce the amount of conditional code in this cassl was worth it imho.
>>>
>>> This would require unifying:
>>>
>>> msv:
>>> 	struct Limits {
>>> 		std::array<uint32_t, 2> exposure;
>>> 		std::array<double, 2> gain;
>>> 		double gainMinStep;
>>> 		double gain1;
>>> 	};
>>>
>>> mean_luminance:
>>> 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>>> 		       double minGain, double maxGain, std::vector<AgcConstraint> constraints);
>>>
>>> msv:
>>> 	struct Params {
>>> 		const Histogram &yHist;
>>> 		uint32_t exposure;
>>> 		double gain;
>>> 	};
>>>
>>> mean_luminance:
>>> 	struct Params {
>>> 		const Traits &traits;
>>> 		const Histogram &yHist;
>>> 		utils::Duration effectiveExposureValue;
>>> 		uint32_t constraintModeIndex;
>>> 		uint32_t exposureModeIndex;
>>> 		double lux = 0;
>>> 		double exposureCompensation = 1;
>>> 	};
>>>
>>> which I know you don't like.
>>>
>>> You're too far in the design and implementation already, and I'm
>>> certainly not asking to reconsider this, but I felt like mentioning
>>> it in case someone else feels the same.
>>
>> That's a fair point, my reasoning is as follows. I would very much postpone
>> the design of an appropriate interface until there are 3 or more implementations.
>> With these two, in my opinion, it's easier to handle the differences in the "user",
>> and all that without having to design an interface, the various data structures,
>> and having to fit the implementations to the interface.
>>
> 
> I know, and I guess your arguments are valid.
> 
> As said, let's leave it there for the time being, what matters to me
> is that we can do the changes on top and we don't corner ourselves.
> 
> I wonder if we will ever have a third implementation, but who knows
> 
>>
>>>
>>>> +	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&) {
>>>
>>> As I presume you know I consider std::visit()'s three levels
>>> indentation worse in practice compared to a simpler if (sensor_) or
>>> similar.
>>
>> The alternative would be an `std::get_if` chain, which would save 1 level.
>>
> 
> I wonder if we sholdn't add rule to do what we do with switch() that
> suffers from the same exact indentation issue
> 
> 	std::visit(utils::overloaded{
> 	[&](AgcMSV&) {
> 
>          },
>          [&](AgcMeanLuminance& impl) {
> 
>          },
> 

I don't like it, but possibly. Nonetheless it should probably
be clang-format compatible otherwise the CI checker will
complain a lot.


> 
>>
>>>
>>>> +			/* no constraint/exposure mode support */
>>>
>>> I would dare to say that one can say that AgcMsv only implements
>>> support for ConstraintNormal and ExposureNormal in its implementation
>>> of constaintModes() and exposureModeHelpers(), so you could move this
>>> to the common part of the code.
>>>
>>>> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
>>>> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>>>> +
>>>> +			state.automatic.yTarget = (2.5 - 1) / (5 - 1); /* \todo hack? */
>>>
>>> does this come from existing code or is this new ?
>>
>> Well, it's more of my interpretation of the algorithm, it's probably not too useful to set it.
>> The only user is the rkisp1 wdr algorithm, but that sets `AgcMeanLuminance::AgcConstraint`s,
>> so it wouldn't work the AgcMSV in any case. I'm not sure that functionality can be reasonably
>> implemented with just gain codes. So the idea there was to reject wdr is a camera sensor helper
>> is not found.
>>
> 
> I will defer this to Stefan which knows Wdr
> 
>>
>>>
>>>> +
>>>> +			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_);
>>>
>>> How can this possible be not true ?
>>
>> It should be non-nullptr, hence the assertion.
> 
> Yeah my point is that you instantiate AgcMeanLuminance only
> if (sensor_), don't you ? How can it become a nullptr ?

Well, it shouldn't ever be. This assertion encodes that expectation.
But I guess the real question is where one draws the line wrt. what
to check.


> 
>>
>>
>>>
>>>> +			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) {
>>>>    		/* The IPA control maps keep their states, so the removal is necessary. */
>>>>
>>>>    		config.ctrlMap.erase(&controls::ExposureValue);
>>>> @@ -601,33 +641,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,
>>>
>>> Do you expect this to change ? If it stays 1, why would you need a
>>> parameter for it ?
>>
>> At this patch, the simple pipeline handler uses `AgcMSV` directly, even
>> with a `CameraSensorHelper`, and in that case it sets `gainMinStep` to
>> something based on the true gain values.
>>
>> Arguably, after the next patch, this parameter will only be 1, so it
>> could be removed.
>>
> 
> ack
> 
>>
>>>
>>>> +					/* 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.
>>>
>>> please reflow to shorter lines
>>
>> How short are you thinking? Enough to move the "was" to the next line?
> 
> For comments, I think 80-cols is still the rule
> 
> 				/*
> 				 * The Agc algorithm needs to know the effective
> 				 * exposure value that was applied to the sensor
> 				 * when the statistics were collected.
> 				 */
> 

Ok


>>
>>
>>>
>>>> +				 */
>>>> +				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 d02dbff562..1babf0762d 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"
>>>>
>>>> @@ -56,6 +58,7 @@ struct Session {
>>>>    	utils::Duration maxExposureTime;
>>>>    	double minAnalogueGain;
>>>>    	double maxAnalogueGain;
>>>> +	double defAnalogueGain;
>>>>    	utils::Duration minFrameDuration;
>>>>    	utils::Duration maxFrameDuration;
>>>>
>>>> @@ -114,14 +117,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);
>>>> @@ -146,7 +148,8 @@ public:
>>>>    		     ControlList &metadata);
>>>>
>>>>    private:
>>>> -	AgcMeanLuminance impl_;
>>>> +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
>>>> +	CameraSensorHelper *sensor_ = nullptr;
>>>
>>> Would an optional<> help or is it an overkill ?
>>
>> Can you clarify what you mean exactly? For `sensor_` or `impl_` or?
>>
> 
> I meant for the sensor_, but it doesn't change much and is passed in
> as a pointer, so don't bother.
> 
>>
>>>
>>>>    };
>>>>
>>>>    } /* 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,
>>>> --
>>>> 2.55.0
>>>>
>>