[v6,45/47] ipa: libipa: agc: Work without `CameraSensorHelper`
diff mbox series

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

Commit Message

Barnabás Pőcze Aug. 20, 2026, 3:19 p.m. UTC
`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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp     |  13 +-
 src/ipa/libipa/agc.cpp              | 183 +++++++++++++++++++---------
 src/ipa/libipa/agc.h                |   9 +-
 src/ipa/mali-c55/algorithms/agc.cpp |  13 +-
 src/ipa/rkisp1/algorithms/agc.cpp   |  13 +-
 5 files changed, 148 insertions(+), 83 deletions(-)

--
2.55.0

Comments

Jacopo Mondi Aug. 21, 2026, 8:48 a.m. UTC | #1
Hi Barnabás

On Thu, Aug 20, 2026 at 05:19:15PM +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>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp     |  13 +-
>  src/ipa/libipa/agc.cpp              | 183 +++++++++++++++++++---------
>  src/ipa/libipa/agc.h                |   9 +-
>  src/ipa/mali-c55/algorithms/agc.cpp |  13 +-
>  src/ipa/rkisp1/algorithms/agc.cpp   |  13 +-
>  5 files changed, 148 insertions(+), 83 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 4b2336b660..81f1d1e03c 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -66,12 +66,12 @@ Agc::Agc()
>   */
>  int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
> -	return agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.init(tuningData, context.camHelper.get(),
> +			 context.configuration.agc, context.activeState.agc, {

If I read it right, you're only removing

		.sensor = context.camHelper.get(),

There shouldn't be a need to re-indent right ?

(I'm not saying the new indentation is worse, I actually think it's
better, but it should maybe be done like this from the beginnig ?)

> +				.sensorInfo = context.sensorInfo,
> +				.sensorControls = context.sensorControls,
> +				.ctrlMap = context.ctrlMap,
> +			 });
>  }
>
>  /**
> @@ -88,7 +88,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 8d571f43e0..f614d52ef4 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>

The class documentation

/**
 * \class AgcAlgorithm
 * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
 *
 * \todo DigitalGain, DigitalGainMode
 */

Should now be updated

> @@ -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,13 +266,20 @@ namespace agc {
>  /**
>   * \brief Load tuning data and configure
>   */
> -int AgcAlgorithm::init(const ValueNode &tuningData,
> +int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>  		       agc::Session &session, agc::ActiveState &state,
>  		       const ConfigurationParams &config)
>  {
> -	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 configure(session, state, config);
>  }
> @@ -301,10 +310,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 << "], "
> @@ -343,28 +356,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;
>
> @@ -415,25 +421,52 @@ 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::visit(utils::overloaded{
> +		[&](AgcMSV &) {
> +			/* No constraint/exposure mode support. */
> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>
> -		{
> -			std::vector<ControlValue> options;
> -			for (const auto &[id, _] : impl_.constraintModes())
> -				options.emplace_back(id);
> +			state.automatic.yTarget = 0; /* Not supported. */
>
> -			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
> -		}
> +			if (!session.autoAllowed)
> +				return;
> +
> +			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);
> +
> +			impl.configure(session.lineDuration, sensor_);
> +			impl.resetFrameCount();
> +
> +			if (!session.autoAllowed)
> +				return;
> +
> +			config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>
> -		{
>  			std::vector<ControlValue> options;
> -			for (const auto &[id, _] : impl_.exposureModeHelpers())
> +			for (const auto &[id, _] : impl.constraintModes())
>  				options.emplace_back(id);
> +			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
>
> +			options.clear();
> +			for (const auto &[id, _] : impl.exposureModeHelpers())
> +				options.emplace_back(id);
>  			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
> -		}
> -	}
> +		},
> +	}, impl_);
>
>  	return 0;
>  }
> @@ -623,36 +656,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),

prefer static_cast<> over plain C casts.

I keep requesting this, but only because it's something we enforced
since the very beginnig. I actually think C casts for integers are
more compact

> +				},
> +				.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;
> @@ -662,7 +727,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 60ca00caa2..05db84548a 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -10,6 +10,7 @@
>  #include <optional>
>  #include <stdint.h>
>  #include <utility>
> +#include <variant>
>
>  #include <linux/v4l2-controls.h>
>
> @@ -19,6 +20,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"
>
> @@ -33,6 +35,7 @@ struct Session {
>  	utils::Duration maxExposureTime;
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
> +	double defAnalogueGain;
>  	utils::Duration minFrameDuration;
>  	utils::Duration maxFrameDuration;
>  	utils::Duration lineDuration;
> @@ -114,7 +117,6 @@ class AgcAlgorithm
>  {
>  public:
>  	struct ConfigurationParams {
> -		const CameraSensorHelper *sensor;
>  		const IPACameraSensorInfo &sensorInfo;
>  		const ControlInfoMap &sensorControls;
>  		ControlInfoMap::Map &ctrlMap;
> @@ -130,7 +132,7 @@ public:
>  		double lux = 0;
>  	};
>
> -	int init(const ValueNode &tuningData,
> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>  		 agc::Session &session, agc::ActiveState &state,
>  		 const ConfigurationParams &config);
>
> @@ -154,7 +156,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 b0df3269d6..9cb6233b3e 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -122,12 +122,12 @@ Agc::Agc()
>
>  int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
> -	return agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	return agc_.init(tuningData, context.camHelper.get(),
> +			 context.configuration.agc, context.activeState.agc, {
> +				.sensorInfo = context.sensorInfo,
> +				.sensorControls = context.sensorControls,
> +				.ctrlMap = context.ctrlMap,
> +			 });

same comment as per IPU3 regarding the indentation

>  }
>
>  int Agc::configure(IPAContext &context,
> @@ -138,7 +138,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 a83ee33b20..e5eac03943 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -136,12 +136,12 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>  {
>  	int ret;
>
> -	ret = agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
> -		.sensor = context.camHelper.get(),
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -	});
> +	ret = agc_.init(tuningData, context.camHelper.get(),
> +			context.configuration.agc, context.activeState.agc, {
> +				.sensorInfo = context.sensorInfo,
> +				.sensorControls = context.sensorControls,
> +				.ctrlMap = context.ctrlMap,
> +			});

Same here.
I'm starting to wonder if it's intentional

Only minors, my tag still stands


>  	if (ret)
>  		return ret;
>
> @@ -163,7 +163,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
Barnabás Pőcze Aug. 21, 2026, 9:51 a.m. UTC | #2
2026. 08. 21. 10:48 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Aug 20, 2026 at 05:19:15PM +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>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp     |  13 +-
>>   src/ipa/libipa/agc.cpp              | 183 +++++++++++++++++++---------
>>   src/ipa/libipa/agc.h                |   9 +-
>>   src/ipa/mali-c55/algorithms/agc.cpp |  13 +-
>>   src/ipa/rkisp1/algorithms/agc.cpp   |  13 +-
>>   5 files changed, 148 insertions(+), 83 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 4b2336b660..81f1d1e03c 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -66,12 +66,12 @@ Agc::Agc()
>>    */
>>   int Agc::init(IPAContext &context, const ValueNode &tuningData)
>>   {
>> -	return agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
>> -		.sensor = context.camHelper.get(),
>> -		.sensorInfo = context.sensorInfo,
>> -		.sensorControls = context.sensorControls,
>> -		.ctrlMap = context.ctrlMap,
>> -	});
>> +	return agc_.init(tuningData, context.camHelper.get(),
>> +			 context.configuration.agc, context.activeState.agc, {
> 
> If I read it right, you're only removing
> 
> 		.sensor = context.camHelper.get(),

No, it is also passed as a separate argument. I would have done

   agc_.init(tuningData, context.camHelper.get(), context.configuration.agc, context.activeState.agc, {

but then I imagine it would be too long.


> 
> There shouldn't be a need to re-indent right ?
> 
> (I'm not saying the new indentation is worse, I actually think it's
> better, but it should maybe be done like this from the beginnig ?)
> 
>> +				.sensorInfo = context.sensorInfo,
>> +				.sensorControls = context.sensorControls,
>> +				.ctrlMap = context.ctrlMap,
>> +			 });
>>   }
>>
>>   /**
>> @@ -88,7 +88,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 8d571f43e0..f614d52ef4 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>
> 
> The class documentation
> 
> /**
>   * \class AgcAlgorithm
>   * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
>   *
>   * \todo DigitalGain, DigitalGainMode
>   */
> 
> Should now be updated

Done.


> 
>> @@ -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,13 +266,20 @@ namespace agc {
>>   /**
>>    * \brief Load tuning data and configure
>>    */
>> -int AgcAlgorithm::init(const ValueNode &tuningData,
>> +int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>>   		       agc::Session &session, agc::ActiveState &state,
>>   		       const ConfigurationParams &config)
>>   {
>> -	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 configure(session, state, config);
>>   }
>> @@ -301,10 +310,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 << "], "
>> @@ -343,28 +356,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;
>>
>> @@ -415,25 +421,52 @@ 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::visit(utils::overloaded{
>> +		[&](AgcMSV &) {
>> +			/* No constraint/exposure mode support. */
>> +			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
>> +			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;
>>
>> -		{
>> -			std::vector<ControlValue> options;
>> -			for (const auto &[id, _] : impl_.constraintModes())
>> -				options.emplace_back(id);
>> +			state.automatic.yTarget = 0; /* Not supported. */
>>
>> -			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
>> -		}
>> +			if (!session.autoAllowed)
>> +				return;
>> +
>> +			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);
>> +
>> +			impl.configure(session.lineDuration, sensor_);
>> +			impl.resetFrameCount();
>> +
>> +			if (!session.autoAllowed)
>> +				return;
>> +
>> +			config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>>
>> -		{
>>   			std::vector<ControlValue> options;
>> -			for (const auto &[id, _] : impl_.exposureModeHelpers())
>> +			for (const auto &[id, _] : impl.constraintModes())
>>   				options.emplace_back(id);
>> +			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
>>
>> +			options.clear();
>> +			for (const auto &[id, _] : impl.exposureModeHelpers())
>> +				options.emplace_back(id);
>>   			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
>> -		}
>> -	}
>> +		},
>> +	}, impl_);
>>
>>   	return 0;
>>   }
>> @@ -623,36 +656,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),
> 
> prefer static_cast<> over plain C casts.
> 
> I keep requesting this, but only because it's something we enforced
> since the very beginnig. I actually think C casts for integers are
> more compact

Ok, I'll replace them. But it is not a C-style cast, it's a "functional-style cast",
and it has different behaviour. (https://en.cppreference.com/cpp/language/explicit_cast)
Especially for integral types, I find them to be quite compact.


> 
>> +				},
>> +				.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;
>> @@ -662,7 +727,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 60ca00caa2..05db84548a 100644
>> --- a/src/ipa/libipa/agc.h
>> +++ b/src/ipa/libipa/agc.h
>> @@ -10,6 +10,7 @@
>>   #include <optional>
>>   #include <stdint.h>
>>   #include <utility>
>> +#include <variant>
>>
>>   #include <linux/v4l2-controls.h>
>>
>> @@ -19,6 +20,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"
>>
>> @@ -33,6 +35,7 @@ struct Session {
>>   	utils::Duration maxExposureTime;
>>   	double minAnalogueGain;
>>   	double maxAnalogueGain;
>> +	double defAnalogueGain;
>>   	utils::Duration minFrameDuration;
>>   	utils::Duration maxFrameDuration;
>>   	utils::Duration lineDuration;
>> @@ -114,7 +117,6 @@ class AgcAlgorithm
>>   {
>>   public:
>>   	struct ConfigurationParams {
>> -		const CameraSensorHelper *sensor;
>>   		const IPACameraSensorInfo &sensorInfo;
>>   		const ControlInfoMap &sensorControls;
>>   		ControlInfoMap::Map &ctrlMap;
>> @@ -130,7 +132,7 @@ public:
>>   		double lux = 0;
>>   	};
>>
>> -	int init(const ValueNode &tuningData,
>> +	int init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>>   		 agc::Session &session, agc::ActiveState &state,
>>   		 const ConfigurationParams &config);
>>
>> @@ -154,7 +156,8 @@ private:
>>   			  const agc::FrameContext &frameContext,
>>   			  ControlList &metadata);
>>
>> -	AgcMeanLuminance impl_;
>> +	std::variant<AgcMSV, AgcMeanLuminance> impl_;
>> +	CameraSensorHelper *sensor_ = nullptr;
>>   };
>>
>>   } /* namespace ipa */
> [...]

Patch
diff mbox series

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 4b2336b660..81f1d1e03c 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -66,12 +66,12 @@  Agc::Agc()
  */
 int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
-	return agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
-		.sensorInfo = context.sensorInfo,
-		.sensorControls = context.sensorControls,
-		.ctrlMap = context.ctrlMap,
-	});
+	return agc_.init(tuningData, context.camHelper.get(),
+			 context.configuration.agc, context.activeState.agc, {
+				.sensorInfo = context.sensorInfo,
+				.sensorControls = context.sensorControls,
+				.ctrlMap = context.ctrlMap,
+			 });
 }

 /**
@@ -88,7 +88,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 8d571f43e0..f614d52ef4 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,13 +266,20 @@  namespace agc {
 /**
  * \brief Load tuning data and configure
  */
-int AgcAlgorithm::init(const ValueNode &tuningData,
+int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor,
 		       agc::Session &session, agc::ActiveState &state,
 		       const ConfigurationParams &config)
 {
-	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 configure(session, state, config);
 }
@@ -301,10 +310,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 << "], "
@@ -343,28 +356,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;

@@ -415,25 +421,52 @@  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::visit(utils::overloaded{
+		[&](AgcMSV &) {
+			/* No constraint/exposure mode support. */
+			state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal;
+			state.exposureMode = controls::AeExposureModeEnum::ExposureNormal;

-		{
-			std::vector<ControlValue> options;
-			for (const auto &[id, _] : impl_.constraintModes())
-				options.emplace_back(id);
+			state.automatic.yTarget = 0; /* Not supported. */

-			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);
-		}
+			if (!session.autoAllowed)
+				return;
+
+			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);
+
+			impl.configure(session.lineDuration, sensor_);
+			impl.resetFrameCount();
+
+			if (!session.autoAllowed)
+				return;
+
+			config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);

-		{
 			std::vector<ControlValue> options;
-			for (const auto &[id, _] : impl_.exposureModeHelpers())
+			for (const auto &[id, _] : impl.constraintModes())
 				options.emplace_back(id);
+			config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);

+			options.clear();
+			for (const auto &[id, _] : impl.exposureModeHelpers())
+				options.emplace_back(id);
 			config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);
-		}
-	}
+		},
+	}, impl_);

 	return 0;
 }
@@ -623,36 +656,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;
@@ -662,7 +727,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 60ca00caa2..05db84548a 100644
--- a/src/ipa/libipa/agc.h
+++ b/src/ipa/libipa/agc.h
@@ -10,6 +10,7 @@ 
 #include <optional>
 #include <stdint.h>
 #include <utility>
+#include <variant>

 #include <linux/v4l2-controls.h>

@@ -19,6 +20,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"

@@ -33,6 +35,7 @@  struct Session {
 	utils::Duration maxExposureTime;
 	double minAnalogueGain;
 	double maxAnalogueGain;
+	double defAnalogueGain;
 	utils::Duration minFrameDuration;
 	utils::Duration maxFrameDuration;
 	utils::Duration lineDuration;
@@ -114,7 +117,6 @@  class AgcAlgorithm
 {
 public:
 	struct ConfigurationParams {
-		const CameraSensorHelper *sensor;
 		const IPACameraSensorInfo &sensorInfo;
 		const ControlInfoMap &sensorControls;
 		ControlInfoMap::Map &ctrlMap;
@@ -130,7 +132,7 @@  public:
 		double lux = 0;
 	};

-	int init(const ValueNode &tuningData,
+	int init(const ValueNode &tuningData, CameraSensorHelper *sensor,
 		 agc::Session &session, agc::ActiveState &state,
 		 const ConfigurationParams &config);

@@ -154,7 +156,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 b0df3269d6..9cb6233b3e 100644
--- a/src/ipa/mali-c55/algorithms/agc.cpp
+++ b/src/ipa/mali-c55/algorithms/agc.cpp
@@ -122,12 +122,12 @@  Agc::Agc()

 int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
-	return agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
-		.sensorInfo = context.sensorInfo,
-		.sensorControls = context.sensorControls,
-		.ctrlMap = context.ctrlMap,
-	});
+	return agc_.init(tuningData, context.camHelper.get(),
+			 context.configuration.agc, context.activeState.agc, {
+				.sensorInfo = context.sensorInfo,
+				.sensorControls = context.sensorControls,
+				.ctrlMap = context.ctrlMap,
+			 });
 }

 int Agc::configure(IPAContext &context,
@@ -138,7 +138,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 a83ee33b20..e5eac03943 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -136,12 +136,12 @@  int Agc::init(IPAContext &context, const ValueNode &tuningData)
 {
 	int ret;

-	ret = agc_.init(tuningData, context.configuration.agc, context.activeState.agc, {
-		.sensor = context.camHelper.get(),
-		.sensorInfo = context.sensorInfo,
-		.sensorControls = context.sensorControls,
-		.ctrlMap = context.ctrlMap,
-	});
+	ret = agc_.init(tuningData, context.camHelper.get(),
+			context.configuration.agc, context.activeState.agc, {
+				.sensorInfo = context.sensorInfo,
+				.sensorControls = context.sensorControls,
+				.ctrlMap = context.ctrlMap,
+			});
 	if (ret)
 		return ret;

@@ -163,7 +163,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,