[RFC,v2,21/43] ipa: libipa: Add `AgcAlgorithm`

27469 diff mbox series

Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
based on the rkisp1 `Agc` algorithm, with slight adjustments.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/agc.h       |  98 ++++++
 src/ipa/libipa/meson.build |   1 +
 3 files changed, 738 insertions(+)
 create mode 100644 src/ipa/libipa/agc.cpp

Hi Barnabás

On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
> based on the rkisp1 `Agc` algorithm, with slight adjustments.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/agc.h       |  98 ++++++
>  src/ipa/libipa/meson.build |   1 +
>  3 files changed, 738 insertions(+)
>  create mode 100644 src/ipa/libipa/agc.cpp
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> new file mode 100644
> index 0000000000..e16a02fdde
> --- /dev/null
> +++ b/src/ipa/libipa/agc.cpp
> @@ -0,0 +1,639 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021-2022, 2026 Ideas On Board
> + *
> + * Generic AGC algorithm
> + */
> +
> +#include "agc.h"
> +
> +#include <algorithm>
> +#include <array>
> +#include <chrono>
> +#include <optional>
> +
> +#include <linux/v4l2-controls.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +using namespace std::chrono_literals;
> +
> +LOG_DEFINE_CATEGORY(Agc)
> +
> +/**
> + * \class AgcAlgorithm
> + * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
> + *
> + * \todo DigitalGain, DigitalGainMode
> + */
> +
> +/**
> + * \struct agc::Session
> + * \brief Session configuration for AgcAlgorithm
> + *
> + * \var agc::Session::minExposureTime
> + * \brief Minimum exposure time supported with the configured sensor
> + *
> + * \var agc::Session::maxExposureTime
> + * \brief Maximum exposure time supported with the configured sensor
> + *
> + * \var agc::Session::minAnalogueGain
> + * \brief Minimum analogue gain supported with the configured sensor
> + *
> + * \var agc::Session::maxAnalogueGain
> + * \brief Maximum analogue gain supported with the configured sensor
> + *
> + * \var agc::Session::minFrameDuration
> + * \brief Minimum frame duration supported with the configured sensor
> + *
> + * \var agc::Session::maxFrameDuration
> + * \brief Maximum frame duration supported with the configured sensor

I would use "for the streaming session" in place of "with the
configured sensor" in all previous entries.

yes, those settings depend on the sensor configuration but I found the
wording a bit confusing

Or maybe "for the current sensor configuration" ?

> + *
> + * \var agc::Session::lineDuration
> + * \brief Line duration with the configured sensor and output size
> + *
> + * \var agc::Session::sensor
> + * \brief Details of the sensor configuration
> + *
> + * \var agc::Session::sensor.outputSize
> + * \brief Configured output size of the sensor
> + *
> + * \var agc::Session::autoAllowed
> + * \brief Whether automatic controls are allowed

As I was really confused by this, is it worth adding one line saying
that auto is not allowed when capturing raw frames ?

> + */
> +
> +/**
> + * \struct agc::ActiveState
> + * \brief Active state for AgcAlgorithm
> + *
> + * The \a automatic variables track the latest values computed by algorithm
> + * based on the latest processed statistics. All other variables track the
> + * consolidated controls requested in queued requests.
> + *
> + * \var agc::ActiveState::manual
> + * \brief Manual exposure time and analog gain (set through requests)
> + *
> + * \var agc::ActiveState::manual.exposure
> + * \brief Manual exposure time expressed as a number of lines as set by the
> + * ExposureTime control
> + *
> + * \var agc::ActiveState::manual.gain
> + * \brief Manual analogue gain as set by the AnalogueGain control
> + *
> + * \var agc::ActiveState::automatic
> + * \brief Automatic exposure time and analog gain (computed by the algorithm)
> + *
> + * \var agc::ActiveState::automatic.exposure
> + * \brief Automatic exposure time expressed as a number of lines
> + *
> + * \var agc::ActiveState::automatic.gain
> + * \brief Automatic analogue gain multiplier
> + *
> + * \var agc::ActiveState::automatic.quantizationGain
> + * \brief Automatic quantization gain multiplier
> + *
> + * \var agc::ActiveState::automatic.yTarget
> + * \brief Automatically determined luminance target
> + *
> + * \var agc::ActiveState::autoExposureEnabled
> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control

s/AGC state (exposure)/exposure mode

> + *
> + * \var agc::ActiveState::autoGainEnabled
> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control

same here ?

> + *
> + * \var agc::ActiveState::exposureValue
> + * \brief Exposure value as set by the ExposureValue control
> + *
> + * \var agc::ActiveState::constraintMode
> + * \brief Constraint mode as set by the AeConstraintMode control
> + *
> + * \var agc::ActiveState::exposureMode
> + * \brief Exposure mode as set by the AeExposureMode control
> + *
> + * \var agc::ActiveState::minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
> + * \var agc::ActiveState::maxFrameDuration
> + * \brief Maximum frame duration as set by the FrameDurationLimits control
> + */
> +
> +/**
> + * \struct agc::FrameContext
> + * \brief Per-frame context for AgcAlgorithm
> + *
> + * \var agc::FrameContext::exposure
> + * \brief Exposure time expressed as a number of lines computed by the algorithm
> + *
> + * \var agc::FrameContext::gain
> + * \brief Analogue gain multiplier computed by the algorithm

Are these computed by the algorithms or can these be the manually
programmed values when running in manual mode ?

I would replace "computed by the algorithm" with "for the frame" or
similar

> + *
> + * The gain should be adapted to the sensor specific gain code before applying.

s/adapted/translated ?

> + *
> + * \var agc::FrameContext::quantizationGain
> + * \brief Quantization gain multiplier computed by the algorithm
> + *
> + * \var agc::FrameContext::exposureValue
> + * \brief Exposure value as set by the ExposureValue control
> + *
> + * \var agc::FrameContext::yTarget
> + * \brief Luminance target computed by the algorithm
> + *
> + * \var agc::FrameContext::vblank
> + * \brief Vertical blanking parameter computed by the algorithm
> + *
> + * \var agc::FrameContext::autoExposureEnabled
> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> + *
> + * \var agc::FrameContext::autoGainEnabled
> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> + *
> + * \var agc::FrameContext::constraintMode
> + * \brief Constraint mode as set by the AeConstraintMode control
> + *
> + * \var agc::FrameContext::exposureMode
> + * \brief Exposure mode as set by the AeExposureMode control
> + *
> + * \var agc::FrameContext::minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
> + * \var agc::FrameContext::maxFrameDuration
> + * \brief Maximum frame duration as set by the FrameDurationLimits control
> + *
> + * \var agc::FrameContext::frameDuration
> + * \brief The actual FrameDuration used by the algorithm for the frame
> + *
> + * \var agc::FrameContext::autoExposureModeChange
> + * \brief Indicate if autoExposureEnabled has changed from true in the previous
> + * frame to false in the current frame, and no manual exposure value has been
> + * supplied in the current frame.

does this only track auto->manual transaction or does it track the
other way around.

Kind of a long brief (and no '.' at the end).

I would

     \brief Exposure mode change flag

     Indicate if the exposure mode has changed compared to the
     previous frame.

> + *
> + * \var agc::FrameContext::autoGainModeChange
> + * \brief Indicate if autoGainEnabled has changed from true in the previous
> + * frame to false in the current frame, and no manual gain value has been
> + * supplied in the current frame.
> + */
> +
> +/**
> + * \struct AgcAlgorithm::ConfigurationParams
> + * \brief Parameters for AgcAlgorithm::configure()
> + *
> + * \var AgcAlgorithm::ConfigurationParams::sensor
> + * \brief CameraSensorHelper for the sensor
> + *
> + * \var AgcAlgorithm::ConfigurationParams::sensorInfo
> + * \brief Details of the sensor

The sensor configuration description

> + *
> + * \var AgcAlgorithm::ConfigurationParams::sensorControls
> + * \brief ControlInfoMap of the sensor
> + *
> + * \var AgcAlgorithm::ConfigurationParams::ctrlMap
> + * \brief ControlMap to update with controls
> + *
> + * \var AgcAlgorithm::ConfigurationParams::autoAllowed
> + * \brief Whether to enable auto controls
> + */
> +
> +/**
> + * \struct AgcAlgorithm::ProcessParams
> + * \brief Parameters for AgcAlgorithm::process()
> + *
> + * \var AgcAlgorithm::ProcessParams::traits
> + * \brief Implementation of AgcMeanLuminance::Traits
> + *
> + * \var AgcAlgorithm::ProcessParams::yHist
> + * \brief Luminance histogram of the frame
> + *
> + * \var AgcAlgorithm::ProcessParams::exposure
> + * \brief Effective exposure of the frame
> + *
> + * \var AgcAlgorithm::ProcessParams::gain
> + * \brief Effective gain of the frame
> + *
> + * \var AgcAlgorithm::ProcessParams::additionalConstraints
> + * \brief Additional AgcMeanLuminance::AgcConstraints to apply
> + *
> + * \var AgcAlgorithm::ProcessParams::lux
> + * \brief Effective lux value of the frame
> + */
> +
> +/**
> + * \brief Load tuning data
> + */
> +int AgcAlgorithm::init(const ValueNode &tuningData)
> +{
> +	int ret = impl_.parseTuningData(tuningData);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Initialize the session configuration and active state
> + */
> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)

Easy to shorten to

int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
			    const ConfigurationParams &config)


Seems like session is an out parameter.

We used to have a rule: const & for input params, * for output ones.
Not sure anymore how much we enforce it these days..

> +{
> +	session = {};
> +	session.lineDuration = config.sensorInfo.minLineLength * 1.0s
> +		/ config.sensorInfo.pixelRate;
> +	session.sensor.outputSize = config.sensorInfo.outputSize;
> +	session.autoAllowed = config.autoAllowed;
> +
> +	const double lineDurationUs = session.lineDuration.get<std::micro>();
> +
> +	/*
> +	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
> +	 * limits and the line duration.
> +	 */
> +
> +	const ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;
> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> +	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
> +	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
> +		static_cast<int32_t>(minExposure * lineDurationUs),
> +		static_cast<int32_t>(maxExposure * lineDurationUs),
> +		static_cast<int32_t>(defExposure * lineDurationUs),
> +	};
> +
> +	/* Compute the analogue gain limits. */
> +	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>());
> +	config.ctrlMap[&controls::AnalogueGain] = ControlInfo{
> +		minGain,
> +		maxGain,
> +		defGain,
> +	};
> +
> +	LOG(Agc, Debug)
> +		<< "Exposure: [" << minExposure << ", " << maxExposure
> +		<< "], gain: [" << minGain << ", " << maxGain << "]";
> +
> +	/*
> +	 * Compute the frame duration limits.
> +	 *
> +	 * The frame length is computed assuming a fixed line length combined
> +	 * with the vertical frame sizes.
> +	 */
> +	const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> +	uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
> +
> +	const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
> +	std::array<uint32_t, 3> frameHeights{
> +		v4l2VBlank.min().get<int32_t>() + config.sensorInfo.outputSize.height,
> +		v4l2VBlank.max().get<int32_t>() + config.sensorInfo.outputSize.height,
> +		v4l2VBlank.def().get<int32_t>() + config.sensorInfo.outputSize.height,
> +	};
> +
> +	std::array<int64_t, 3> frameDurations;
> +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> +		uint64_t frameSize = lineLength * frameHeights[i];
> +		frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);
> +	}
> +
> +	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
> +		frameDurations[0],
> +		frameDurations[1],
> +		Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } },
> +	};
> +
> +	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
> +	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
> +
> +	/*
> +	 * When the AGC computes the new exposure values for a frame, it needs
> +	 * to know the limits for exposure time and analogue gain. As it depends
> +	 * on the sensor, update it with the controls.
> +	 *
> +	 * \todo take VBLANK into account for maximum exposure time
> +	 */
> +	session.minExposureTime = minExposure * session.lineDuration;
> +	session.maxExposureTime = maxExposure * session.lineDuration;
> +	session.minAnalogueGain = minGain;
> +	session.maxAnalogueGain = maxGain;

The above matches the current RkISP1Agc::reconfigure() content

> +
> +	impl_.configure(session.lineDuration, config.sensor);
> +	impl_.setLimits(session.minExposureTime, session.maxExposureTime,
> +			session.minAnalogueGain, session.maxAnalogueGain,
> +			{});
> +	impl_.resetFrameCount();
> +
> +	/* Configure the default exposure and gain. */
> +	state = {};
> +	state.automatic.gain = session.minAnalogueGain;
> +	state.automatic.exposure = 10ms / session.lineDuration;
> +	state.automatic.quantizationGain = 1;
> +	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);

And this matches RkISP1::Agc() apart from this early yTarget
computation which I presume is intentional

> +	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;
> +
> +	const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {
> +		std::array<ControlValue, 2> values;
> +		size_t count = 0;
> +
> +		if (session.autoAllowed)
> +			values[count++] = ControlValue(automatic);
> +
> +		values[count++] = ControlValue(manual);
> +
> +		config.ctrlMap[&cid] = ControlInfo{
> +			{ values.data(), count },
> +			ControlValue(session.autoAllowed ? automatic : manual),
> +		};
> +	};
> +
> +	add(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
> +	add(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);

Could you please break trivially re-adjustable long lines ?

> +
> +	/* \todo Move this to the `Camera` class. */
> +	config.ctrlMap[&controls::AeEnable] = ControlInfo{
> +		false,
> +		session.autoAllowed,
> +		session.autoAllowed,
> +	};

RkISP1Agc register controls in init() but I think what you're doing
here it's better, as some control such as ExposureTimeMode depend on
the session configuration.

Is there any risk of having leftovers from a previous session in the
control list ?

> +
> +	if (session.autoAllowed) {
> +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> +
> +		for (const auto &[id, info] : impl_.controls())
> +			config.ctrlMap[id] = info;
> +	} else {
> +		config.ctrlMap.erase(&controls::ExposureValue);
> +
> +		for (const auto &[id, info] : impl_.controls())
> +			config.ctrlMap.erase(id);
> +	}

Maybe it happens later on, but this is a bit cumbersome. Making
AgcMeanLuminance aware of autoAllowed would avoid this

> +
> +

Double empty line

> +	return 0;
> +}
> +
> +/**
> + * \brief Handle a \a queueRequest operation
> + */
> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
> +				agc::FrameContext &frameContext, const ControlList &controls)
> +{
> +	if (session.autoAllowed) {

Now that you register controls at configure() time and not init()
time, am I correct that if !autoAllowed applications won't be able to select
controls::ExposureTimeModeAuto ?

Which makes me wonder if controls::ExposureTimeMode should be
registered at all if !autoAllowed

> +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> +		if (aeEnable &&
> +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {
> +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
> +
> +			LOG(Agc, Debug)
> +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
> +				<< " AGC (exposure)";
> +
> +			/*
> +			 * If we go from auto -> manual with no manual control
> +			 * set, use the last computed value, which we don't
> +			 * know until prepare() so save this information.
> +			 *
> +			 * \todo Check the previous frame at prepare() time
> +			 * instead of saving a flag here
> +			 */
> +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
> +				frameContext.autoExposureModeChange = true;
> +		}
> +
> +		const auto &agEnable = controls.get(controls::AnalogueGainMode);

Same reasoning for AnalogueGainMode

> +		if (agEnable &&
> +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
> +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> +
> +			LOG(Agc, Debug)
> +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
> +				<< " AGC (gain)";
> +			/*
> +			 * If we go from auto -> manual with no manual control
> +			 * set, use the last computed value, which we don't
> +			 * know until prepare() so save this information.
> +			 */
> +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
> +				frameContext.autoGainModeChange = true;
> +		}
> +	}
> +
> +	const auto &exposure = controls.get(controls::ExposureTime);
> +	if (exposure && !state.autoExposureEnabled) {
> +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
> +
> +		LOG(Agc, Debug)
> +			<< "Set exposure to " << state.manual.exposure;
> +	}
> +
> +	const auto &gain = controls.get(controls::AnalogueGain);
> +	if (gain && !state.autoGainEnabled) {
> +		state.manual.gain = *gain;
> +
> +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
> +	}
> +
> +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
> +	frameContext.autoGainEnabled = state.autoGainEnabled;
> +
> +	if (!frameContext.autoExposureEnabled)
> +		frameContext.exposure = state.manual.exposure;
> +	if (!frameContext.autoGainEnabled)
> +		frameContext.gain = state.manual.gain;
> +
> +	if (!frameContext.autoExposureEnabled &&
> +	    !frameContext.autoGainEnabled)
> +		frameContext.quantizationGain = 1.0;
> +
> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> +	if (exposureMode)
> +		state.exposureMode =
> +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
> +	frameContext.exposureMode = state.exposureMode;
> +
> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> +	if (constraintMode)
> +		state.constraintMode =
> +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> +	frameContext.constraintMode = state.constraintMode;
> +
> +	const auto &exposureValue = controls.get(controls::ExposureValue);
> +	if (exposureValue)
> +		state.exposureValue = *exposureValue;
> +	frameContext.exposureValue = state.exposureValue;
> +
> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> +	if (frameDurationLimits) {
> +		/* Limit the control value to the limits in ControlInfo */
> +		state.minFrameDuration = std::clamp<utils::Duration>(
> +			std::chrono::microseconds((*frameDurationLimits).front()),
> +			session.minFrameDuration,
> +			session.maxFrameDuration
> +		);
> +
> +		state.maxFrameDuration = std::clamp<utils::Duration>(
> +			std::chrono::microseconds((*frameDurationLimits).back()),
> +			session.minFrameDuration,
> +			session.maxFrameDuration
> +		);
> +	}
> +	frameContext.minFrameDuration = state.minFrameDuration;
> +	frameContext.maxFrameDuration = state.maxFrameDuration;
> +}
> +

This matches RkISP1Agc::queueRequest() (metering mode handling apart)

> +/**
> + * \brief Handle a \a prepare operation
> + */
> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
> +{
> +	uint32_t activeAutoExposure = state.automatic.exposure;
> +	double activeAutoGain = state.automatic.gain;
> +	double activeAutoQGain = state.automatic.quantizationGain;
> +
> +	/* Populate exposure and gain in auto mode */
> +	if (frameContext.autoExposureEnabled) {
> +		frameContext.exposure = activeAutoExposure;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +	if (frameContext.autoGainEnabled) {
> +		frameContext.gain = activeAutoGain;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +
> +	/*
> +	 * Populate manual exposure and gain from the active auto values when
> +	 * transitioning from auto to manual
> +	 */

I guess this answers to my question on the auto->manual transition
tracking for the frameContext.autoExposureModeChange flag.

It tracks both state changes, but we use it to propagate auto values
to the manual state if they have not been overriden by an application
provide control

> +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
> +		state.manual.exposure = activeAutoExposure;
> +		frameContext.exposure = activeAutoExposure;
> +	}
> +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
> +		state.manual.gain = activeAutoGain;
> +		frameContext.gain = activeAutoGain;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +
> +	frameContext.yTarget = state.automatic.yTarget;
> +}

Ack, this matches RkISP1Agc::prepare()

> +
> +/**
> + * \brief Handle a \a process operation
> + */
> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
> +			   ControlList &metadata)
> +{
> +	const utils::Duration &lineDuration = session.lineDuration;
> +	utils::Duration newExposureTime = {};
> +
> +	if (params) {

I presume (well, you told me) that this matches the

        if (!stats)

case in RkISP1.

I wonder, should

The RkISP1 IPA does

	if (!stats) {
		processFrameDuration(context, frameContext,
				     frameContext.agc.minFrameDuration);
		fillMetadata(context, frameContext, metadata);
		return;
	}

I guess it's fine for IPA to call AgcAlgorithm::process() even when
!stats, as it is simpler for them to make the call unconditionally, so
I'm not against this.

But maybe we can do like RkISP1 does (split processFrameDuration() and
fillMetadata() to helpers) to save some indentation here.

> +		ASSERT(session.autoAllowed);

Does it mean IPA should not call process if (autoAllowed) ?

Can't we simply bail out here to make IPAs simpler ?

> +
> +		/*
> +		* Set the AGC limits using the fixed exposure time and/or gain in
> +		* manual mode, or the sensor limits in auto mode.
> +		*/

Weird indent

> +		utils::Duration minExposureTime;
> +		utils::Duration maxExposureTime;
> +		double minAnalogueGain;
> +		double maxAnalogueGain;
> +
> +		if (frameContext.autoExposureEnabled) {
> +			minExposureTime = session.minExposureTime;
> +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);

very long line

> +		} else {
> +			minExposureTime = lineDuration * frameContext.exposure;
> +			maxExposureTime = minExposureTime;
> +		}
> +
> +		if (frameContext.autoGainEnabled) {
> +			minAnalogueGain = session.minAnalogueGain;
> +			maxAnalogueGain = session.maxAnalogueGain;
> +		} else {
> +			minAnalogueGain = frameContext.gain;
> +			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;

RkISP1 gets exposure and gain from the frameContext.

I see in the next patches that port RkISP1 to AgcAlgorithm this hunk

        agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,

                        ...

                       .exposure = frameContext.sensor.exposure,
                       /*
                        * Include the quantization gain if it was applied. Do not use
                        * compress.gain because it will include gains that shall not be
                        * reported to the user when HDR is implemented.
                        */
                       .gain = frameContext.sensor.gain
                               * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),

Is the reason you can't take exposure and gain from the FrameContext
the gain adjustment ?

> +
> +		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),
> +		});
> +
> +		LOG(Agc, Debug)
> +			<< "Divided up exposure time, analogue gain, quantization gain"
> +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
> +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
> +
> +		/* Update the estimated exposure and gain. */
> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
> +		state.automatic.gain = newEv.analogueGain;
> +		state.automatic.quantizationGain = newEv.quantizationGain;
> +		state.automatic.yTarget = newEv.yTarget;
> +
> +		newExposureTime = newEv.exposureTime;
> +	}
> +
> +	/*
> +	 * Expand the target frame duration so that we do not run faster than
> +	 * the minimum frame duration when we have short exposures.
> +	 */
> +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
> +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
> +
> +	/* Update frame duration accounting for line length quantization. */
> +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;

Easy to shorten to

	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)
				   * lineDuration;

> +
> +	metadata.set(controls::AnalogueGain, frameContext.gain);
> +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());

I'm not sure I understand the strategy here.. this line is very long
and could be easily made

	metadata.set(controls::ExposureTime,
		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());


> +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
> +	metadata.set(controls::ExposureTimeMode,
> +		     frameContext.autoExposureEnabled
> +		     ? controls::ExposureTimeModeAuto
> +		     : controls::ExposureTimeModeManual);
> +	metadata.set(controls::AnalogueGainMode,
> +		     frameContext.autoGainEnabled
> +		     ? controls::AnalogueGainModeAuto
> +		     : controls::AnalogueGainModeManual);

These ones instead are very short and could easily be made

	metadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?
		     controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);

> +
> +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
> +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
> +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 5247425952..1eace12908 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -7,13 +7,19 @@
>
>  #pragma once
>
> +#include <optional>
>  #include <utility>
>
>  #include <linux/v4l2-controls.h>
>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "agc_mean_luminance.h"
>  #include "camera_sensor_helper.h"
> +#include "histogram.h"
>
>  namespace libcamera {
>
> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>  	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>  }
>
> +struct Session {
> +	utils::Duration minExposureTime;
> +	utils::Duration maxExposureTime;
> +	double minAnalogueGain;
> +	double maxAnalogueGain;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +
> +	utils::Duration lineDuration;
> +
> +	struct {
> +		Size outputSize;
> +	} sensor;
> +
> +	bool autoAllowed;
> +};
> +
> +struct ActiveState {
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +	} manual;
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +		double quantizationGain;
> +		double yTarget;
> +	} automatic;
> +
> +	bool autoExposureEnabled;
> +	bool autoGainEnabled;
> +	double exposureValue;
> +	controls::AeConstraintModeEnum constraintMode;
> +	controls::AeExposureModeEnum exposureMode;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +};
> +
> +struct FrameContext {
> +	uint32_t exposure;
> +	double gain;
> +	double quantizationGain;
> +	double exposureValue;
> +	double yTarget;
> +	uint32_t vblank;
> +	bool autoExposureEnabled;
> +	bool autoGainEnabled;
> +	controls::AeConstraintModeEnum constraintMode;
> +	controls::AeExposureModeEnum exposureMode;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +	utils::Duration frameDuration;
> +	bool autoExposureModeChange;
> +	bool autoGainModeChange;
> +};
> +
>  } /* namespace agc */
>
> +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);

easy to shorten

> +
> +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
> +			  agc::FrameContext &frameContext, const ControlList &controls);
> +
> +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
> +
> +	struct ProcessParams {
> +		const AgcMeanLuminance::Traits &traits;
> +		const Histogram &yHist;
> +		uint32_t exposure;
> +		double gain;
> +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
> +		double lux = 0;
> +	};
> +
> +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,

same here

> +		     std::optional<ProcessParams> &&params, ControlList &metadata);
> +
> +private:
> +	AgcMeanLuminance impl_;
> +};
> +
>  } /* namespace ipa */
>
>  } /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 05f1a8749c..ca681fa5af 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -23,6 +23,7 @@ libipa_headers = files([
>  ])
>
>  libipa_sources = files([
> +    'agc.cpp',
>      'agc_mean_luminance.cpp',
>      'algorithm.cpp',
>      'awb_bayes.cpp',

Enormous work overall!

Agc is the most complex beast we have and I'm looking forwards to
getting this merged already!


> --
> 2.55.0
>

2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
>> based on the rkisp1 `Agc` algorithm, with slight adjustments.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/agc.h       |  98 ++++++
>>   src/ipa/libipa/meson.build |   1 +
>>   3 files changed, 738 insertions(+)
>>   create mode 100644 src/ipa/libipa/agc.cpp
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> new file mode 100644
>> index 0000000000..e16a02fdde
>> --- /dev/null
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -0,0 +1,639 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021-2022, 2026 Ideas On Board
>> + *
>> + * Generic AGC algorithm
>> + */
>> +
>> +#include "agc.h"
>> +
>> +#include <algorithm>
>> +#include <array>
>> +#include <chrono>
>> +#include <optional>
>> +
>> +#include <linux/v4l2-controls.h>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +#include <libcamera/controls.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +using namespace std::chrono_literals;
>> +
>> +LOG_DEFINE_CATEGORY(Agc)
>> +
>> +/**
>> + * \class AgcAlgorithm
>> + * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
>> + *
>> + * \todo DigitalGain, DigitalGainMode
>> + */
>> +
>> +/**
>> + * \struct agc::Session
>> + * \brief Session configuration for AgcAlgorithm
>> + *
>> + * \var agc::Session::minExposureTime
>> + * \brief Minimum exposure time supported with the configured sensor
>> + *
>> + * \var agc::Session::maxExposureTime
>> + * \brief Maximum exposure time supported with the configured sensor
>> + *
>> + * \var agc::Session::minAnalogueGain
>> + * \brief Minimum analogue gain supported with the configured sensor
>> + *
>> + * \var agc::Session::maxAnalogueGain
>> + * \brief Maximum analogue gain supported with the configured sensor
>> + *
>> + * \var agc::Session::minFrameDuration
>> + * \brief Minimum frame duration supported with the configured sensor
>> + *
>> + * \var agc::Session::maxFrameDuration
>> + * \brief Maximum frame duration supported with the configured sensor
> 
> I would use "for the streaming session" in place of "with the
> configured sensor" in all previous entries.
> 
> yes, those settings depend on the sensor configuration but I found the
> wording a bit confusing
> 
> Or maybe "for the current sensor configuration" ?

I'll drop "supported" and use "for the streaming session".


> 
>> + *
>> + * \var agc::Session::lineDuration
>> + * \brief Line duration with the configured sensor and output size
>> + *
>> + * \var agc::Session::sensor
>> + * \brief Details of the sensor configuration
>> + *
>> + * \var agc::Session::sensor.outputSize
>> + * \brief Configured output size of the sensor
>> + *
>> + * \var agc::Session::autoAllowed
>> + * \brief Whether automatic controls are allowed
> 
> As I was really confused by this, is it worth adding one line saying
> that auto is not allowed when capturing raw frames ?

I suppose if statistics are provided, then it can work for raw capture as well.
So my suggestion:

   \brief Whether automatic controls are allowed
   
   Determines whether statistics are generally expected to be available for
   automatic exposure and gain control. If \a false, statistics must not
   be provided to \a AgcAlgorithm::process(), and \a ExposureTimeMode and
   \a AnalogueGainMode will only avertise manual control, and multiple other
   controls will also be omitted.


> 
>> + */
>> +
>> +/**
>> + * \struct agc::ActiveState
>> + * \brief Active state for AgcAlgorithm
>> + *
>> + * The \a automatic variables track the latest values computed by algorithm
>> + * based on the latest processed statistics. All other variables track the
>> + * consolidated controls requested in queued requests.
>> + *
>> + * \var agc::ActiveState::manual
>> + * \brief Manual exposure time and analog gain (set through requests)
>> + *
>> + * \var agc::ActiveState::manual.exposure
>> + * \brief Manual exposure time expressed as a number of lines as set by the
>> + * ExposureTime control
>> + *
>> + * \var agc::ActiveState::manual.gain
>> + * \brief Manual analogue gain as set by the AnalogueGain control
>> + *
>> + * \var agc::ActiveState::automatic
>> + * \brief Automatic exposure time and analog gain (computed by the algorithm)
>> + *
>> + * \var agc::ActiveState::automatic.exposure
>> + * \brief Automatic exposure time expressed as a number of lines
>> + *
>> + * \var agc::ActiveState::automatic.gain
>> + * \brief Automatic analogue gain multiplier
>> + *
>> + * \var agc::ActiveState::automatic.quantizationGain
>> + * \brief Automatic quantization gain multiplier
>> + *
>> + * \var agc::ActiveState::automatic.yTarget
>> + * \brief Automatically determined luminance target
>> + *
>> + * \var agc::ActiveState::autoExposureEnabled
>> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> 
> s/AGC state (exposure)/exposure mode
> 
>> + *
>> + * \var agc::ActiveState::autoGainEnabled
>> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> 
> same here ?

These were copied verbatim.


> 
>> + *
>> + * \var agc::ActiveState::exposureValue
>> + * \brief Exposure value as set by the ExposureValue control
>> + *
>> + * \var agc::ActiveState::constraintMode
>> + * \brief Constraint mode as set by the AeConstraintMode control
>> + *
>> + * \var agc::ActiveState::exposureMode
>> + * \brief Exposure mode as set by the AeExposureMode control
>> + *
>> + * \var agc::ActiveState::minFrameDuration
>> + * \brief Minimum frame duration as set by the FrameDurationLimits control
>> + *
>> + * \var agc::ActiveState::maxFrameDuration
>> + * \brief Maximum frame duration as set by the FrameDurationLimits control
>> + */
>> +
>> +/**
>> + * \struct agc::FrameContext
>> + * \brief Per-frame context for AgcAlgorithm
>> + *
>> + * \var agc::FrameContext::exposure
>> + * \brief Exposure time expressed as a number of lines computed by the algorithm
>> + *
>> + * \var agc::FrameContext::gain
>> + * \brief Analogue gain multiplier computed by the algorithm
> 
> Are these computed by the algorithms or can these be the manually
> programmed values when running in manual mode ?

This is always the gain that needs to be applied to the frame, so
the manual gain in manual mode, the automatic gain in automatic mode.


> 
> I would replace "computed by the algorithm" with "for the frame" or
> similar
> 
>> + *
>> + * The gain should be adapted to the sensor specific gain code before applying.
> 
> s/adapted/translated ?

These were also all copied verbatim, but I'll adjust it.


> 
>> + *
>> + * \var agc::FrameContext::quantizationGain
>> + * \brief Quantization gain multiplier computed by the algorithm
>> + *
>> + * \var agc::FrameContext::exposureValue
>> + * \brief Exposure value as set by the ExposureValue control
>> + *
>> + * \var agc::FrameContext::yTarget
>> + * \brief Luminance target computed by the algorithm
>> + *
>> + * \var agc::FrameContext::vblank
>> + * \brief Vertical blanking parameter computed by the algorithm
>> + *
>> + * \var agc::FrameContext::autoExposureEnabled
>> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
>> + *
>> + * \var agc::FrameContext::autoGainEnabled
>> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
>> + *
>> + * \var agc::FrameContext::constraintMode
>> + * \brief Constraint mode as set by the AeConstraintMode control
>> + *
>> + * \var agc::FrameContext::exposureMode
>> + * \brief Exposure mode as set by the AeExposureMode control
>> + *
>> + * \var agc::FrameContext::minFrameDuration
>> + * \brief Minimum frame duration as set by the FrameDurationLimits control
>> + *
>> + * \var agc::FrameContext::maxFrameDuration
>> + * \brief Maximum frame duration as set by the FrameDurationLimits control
>> + *
>> + * \var agc::FrameContext::frameDuration
>> + * \brief The actual FrameDuration used by the algorithm for the frame
>> + *
>> + * \var agc::FrameContext::autoExposureModeChange
>> + * \brief Indicate if autoExposureEnabled has changed from true in the previous
>> + * frame to false in the current frame, and no manual exposure value has been
>> + * supplied in the current frame.
> 
> does this only track auto->manual transaction or does it track the
> other way around.
> 
> Kind of a long brief (and no '.' at the end).

These were also copied verbatim, but I have removed the dots.


> 
> I would
> 
>       \brief Exposure mode change flag
> 
>       Indicate if the exposure mode has changed compared to the
>       previous frame.

It only tracks auto -> manual transitions without manual `ExposureTime`,
so I think the above wouldn't be entirely correct.


> 
>> + *
>> + * \var agc::FrameContext::autoGainModeChange
>> + * \brief Indicate if autoGainEnabled has changed from true in the previous
>> + * frame to false in the current frame, and no manual gain value has been
>> + * supplied in the current frame.
>> + */
>> +
>> +/**
>> + * \struct AgcAlgorithm::ConfigurationParams
>> + * \brief Parameters for AgcAlgorithm::configure()
>> + *
>> + * \var AgcAlgorithm::ConfigurationParams::sensor
>> + * \brief CameraSensorHelper for the sensor
>> + *
>> + * \var AgcAlgorithm::ConfigurationParams::sensorInfo
>> + * \brief Details of the sensor
> 
> The sensor configuration description

I have adjusted it to

   Current configuration of the sensor

hope it is also acceptable.


> 
>> + *
>> + * \var AgcAlgorithm::ConfigurationParams::sensorControls
>> + * \brief ControlInfoMap of the sensor
>> + *
>> + * \var AgcAlgorithm::ConfigurationParams::ctrlMap
>> + * \brief ControlMap to update with controls
>> + *
>> + * \var AgcAlgorithm::ConfigurationParams::autoAllowed
>> + * \brief Whether to enable auto controls
>> + */
>> +
>> +/**
>> + * \struct AgcAlgorithm::ProcessParams
>> + * \brief Parameters for AgcAlgorithm::process()
>> + *
>> + * \var AgcAlgorithm::ProcessParams::traits
>> + * \brief Implementation of AgcMeanLuminance::Traits
>> + *
>> + * \var AgcAlgorithm::ProcessParams::yHist
>> + * \brief Luminance histogram of the frame
>> + *
>> + * \var AgcAlgorithm::ProcessParams::exposure
>> + * \brief Effective exposure of the frame
>> + *
>> + * \var AgcAlgorithm::ProcessParams::gain
>> + * \brief Effective gain of the frame
>> + *
>> + * \var AgcAlgorithm::ProcessParams::additionalConstraints
>> + * \brief Additional AgcMeanLuminance::AgcConstraints to apply
>> + *
>> + * \var AgcAlgorithm::ProcessParams::lux
>> + * \brief Effective lux value of the frame
>> + */
>> +
>> +/**
>> + * \brief Load tuning data
>> + */
>> +int AgcAlgorithm::init(const ValueNode &tuningData)
>> +{
>> +	int ret = impl_.parseTuningData(tuningData);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Initialize the session configuration and active state
>> + */
>> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)
> 
> Easy to shorten to
> 
> int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> 			    const ConfigurationParams &config)

Done.


> 
> 
> Seems like session is an out parameter.
> 
> We used to have a rule: const & for input params, * for output ones.
> Not sure anymore how much we enforce it these days..

I'm not sure I would consider it an out param in the traditional sense
(i.e. the caller is not really expected to use/inspect the result), and
I really prefer the consistency of always passing by reference to every
function of `AgcAlgorithm`.


> 
>> +{
>> +	session = {};
>> +	session.lineDuration = config.sensorInfo.minLineLength * 1.0s
>> +		/ config.sensorInfo.pixelRate;
>> +	session.sensor.outputSize = config.sensorInfo.outputSize;
>> +	session.autoAllowed = config.autoAllowed;
>> +
>> +	const double lineDurationUs = session.lineDuration.get<std::micro>();
>> +
>> +	/*
>> +	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
>> +	 * limits and the line duration.
>> +	 */
>> +
>> +	const ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;
>> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
>> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
>> +	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
>> +	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
>> +		static_cast<int32_t>(minExposure * lineDurationUs),
>> +		static_cast<int32_t>(maxExposure * lineDurationUs),
>> +		static_cast<int32_t>(defExposure * lineDurationUs),
>> +	};
>> +
>> +	/* Compute the analogue gain limits. */
>> +	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>());
>> +	config.ctrlMap[&controls::AnalogueGain] = ControlInfo{
>> +		minGain,
>> +		maxGain,
>> +		defGain,
>> +	};
>> +
>> +	LOG(Agc, Debug)
>> +		<< "Exposure: [" << minExposure << ", " << maxExposure
>> +		<< "], gain: [" << minGain << ", " << maxGain << "]";
>> +
>> +	/*
>> +	 * Compute the frame duration limits.
>> +	 *
>> +	 * The frame length is computed assuming a fixed line length combined
>> +	 * with the vertical frame sizes.
>> +	 */
>> +	const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
>> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
>> +	uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
>> +
>> +	const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
>> +	std::array<uint32_t, 3> frameHeights{
>> +		v4l2VBlank.min().get<int32_t>() + config.sensorInfo.outputSize.height,
>> +		v4l2VBlank.max().get<int32_t>() + config.sensorInfo.outputSize.height,
>> +		v4l2VBlank.def().get<int32_t>() + config.sensorInfo.outputSize.height,
>> +	};
>> +
>> +	std::array<int64_t, 3> frameDurations;
>> +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>> +		uint64_t frameSize = lineLength * frameHeights[i];
>> +		frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);
>> +	}
>> +
>> +	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
>> +		frameDurations[0],
>> +		frameDurations[1],
>> +		Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } },
>> +	};
>> +
>> +	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
>> +	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
>> +
>> +	/*
>> +	 * When the AGC computes the new exposure values for a frame, it needs
>> +	 * to know the limits for exposure time and analogue gain. As it depends
>> +	 * on the sensor, update it with the controls.
>> +	 *
>> +	 * \todo take VBLANK into account for maximum exposure time
>> +	 */
>> +	session.minExposureTime = minExposure * session.lineDuration;
>> +	session.maxExposureTime = maxExposure * session.lineDuration;
>> +	session.minAnalogueGain = minGain;
>> +	session.maxAnalogueGain = maxGain;
> 
> The above matches the current RkISP1Agc::reconfigure() content
> 
>> +
>> +	impl_.configure(session.lineDuration, config.sensor);
>> +	impl_.setLimits(session.minExposureTime, session.maxExposureTime,
>> +			session.minAnalogueGain, session.maxAnalogueGain,
>> +			{});
>> +	impl_.resetFrameCount();
>> +
>> +	/* Configure the default exposure and gain. */
>> +	state = {};
>> +	state.automatic.gain = session.minAnalogueGain;
>> +	state.automatic.exposure = 10ms / session.lineDuration;
>> +	state.automatic.quantizationGain = 1;
>> +	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
> 
> And this matches RkISP1::Agc() apart from this early yTarget
> computation which I presume is intentional

Hmmm... that should match as well. There is

   context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);

at the end of `rkisp1.cpp:Agc::configure()`.


> 
>> +	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;
>> +
>> +	const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {
>> +		std::array<ControlValue, 2> values;
>> +		size_t count = 0;
>> +
>> +		if (session.autoAllowed)
>> +			values[count++] = ControlValue(automatic);
>> +
>> +		values[count++] = ControlValue(manual);
>> +
>> +		config.ctrlMap[&cid] = ControlInfo{
>> +			{ values.data(), count },
>> +			ControlValue(session.autoAllowed ? automatic : manual),
>> +		};
>> +	};
>> +
>> +	add(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
>> +	add(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
> 
> Could you please break trivially re-adjustable long lines ?

Adjusted to

   add(controls::ExposureTimeMode,
       controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
   add(controls::AnalogueGainMode,
       controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);

Does that look better?


> 
>> +
>> +	/* \todo Move this to the `Camera` class. */
>> +	config.ctrlMap[&controls::AeEnable] = ControlInfo{
>> +		false,
>> +		session.autoAllowed,
>> +		session.autoAllowed,
>> +	};
> 
> RkISP1Agc register controls in init() but I think what you're doing
> here it's better, as some control such as ExposureTimeMode depend on
> the session configuration.
> 
> Is there any risk of having leftovers from a previous session in the
> control list ?

Every control should either be overwritten or removed.


> 
>> +
>> +	if (session.autoAllowed) {
>> +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>> +
>> +		for (const auto &[id, info] : impl_.controls())
>> +			config.ctrlMap[id] = info;
>> +	} else {
>> +		config.ctrlMap.erase(&controls::ExposureValue);
>> +
>> +		for (const auto &[id, info] : impl_.controls())
>> +			config.ctrlMap.erase(id);
>> +	}
> 
> Maybe it happens later on, but this is a bit cumbersome. Making
> AgcMeanLuminance aware of autoAllowed would avoid this

Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.
And in any case, there will be a loop like this or equivalent, and
I'm not sure it matters too much where it is.


> 
>> +
>> +
> 
> Double empty line

Fixed.


> 
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Handle a \a queueRequest operation
>> + */
>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
>> +				agc::FrameContext &frameContext, const ControlList &controls)
>> +{
>> +	if (session.autoAllowed) {
> 
> Now that you register controls at configure() time and not init()
> time, am I correct that if !autoAllowed applications won't be able to select
> controls::ExposureTimeModeAuto ?

Yes.


> 
> Which makes me wonder if controls::ExposureTimeMode should be
> registered at all if !autoAllowed

I think that's more of a philosophical question. My preference is to
register controls even if they only really have one valid value. Maybe
this needs more discussion to arrive at a libcamera-wide policy.


> 
>> +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
>> +		if (aeEnable &&
>> +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {
>> +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
>> +
>> +			LOG(Agc, Debug)
>> +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
>> +				<< " AGC (exposure)";
>> +
>> +			/*
>> +			 * If we go from auto -> manual with no manual control
>> +			 * set, use the last computed value, which we don't
>> +			 * know until prepare() so save this information.
>> +			 *
>> +			 * \todo Check the previous frame at prepare() time
>> +			 * instead of saving a flag here
>> +			 */
>> +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
>> +				frameContext.autoExposureModeChange = true;
>> +		}
>> +
>> +		const auto &agEnable = controls.get(controls::AnalogueGainMode);
> 
> Same reasoning for AnalogueGainMode
> 
>> +		if (agEnable &&
>> +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
>> +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
>> +
>> +			LOG(Agc, Debug)
>> +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
>> +				<< " AGC (gain)";
>> +			/*
>> +			 * If we go from auto -> manual with no manual control
>> +			 * set, use the last computed value, which we don't
>> +			 * know until prepare() so save this information.
>> +			 */
>> +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
>> +				frameContext.autoGainModeChange = true;
>> +		}
>> +	}
>> +
>> +	const auto &exposure = controls.get(controls::ExposureTime);
>> +	if (exposure && !state.autoExposureEnabled) {
>> +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
>> +
>> +		LOG(Agc, Debug)
>> +			<< "Set exposure to " << state.manual.exposure;
>> +	}
>> +
>> +	const auto &gain = controls.get(controls::AnalogueGain);
>> +	if (gain && !state.autoGainEnabled) {
>> +		state.manual.gain = *gain;
>> +
>> +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
>> +	}
>> +
>> +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
>> +	frameContext.autoGainEnabled = state.autoGainEnabled;
>> +
>> +	if (!frameContext.autoExposureEnabled)
>> +		frameContext.exposure = state.manual.exposure;
>> +	if (!frameContext.autoGainEnabled)
>> +		frameContext.gain = state.manual.gain;
>> +
>> +	if (!frameContext.autoExposureEnabled &&
>> +	    !frameContext.autoGainEnabled)
>> +		frameContext.quantizationGain = 1.0;
>> +
>> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
>> +	if (exposureMode)
>> +		state.exposureMode =
>> +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
>> +	frameContext.exposureMode = state.exposureMode;
>> +
>> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
>> +	if (constraintMode)
>> +		state.constraintMode =
>> +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
>> +	frameContext.constraintMode = state.constraintMode;
>> +
>> +	const auto &exposureValue = controls.get(controls::ExposureValue);
>> +	if (exposureValue)
>> +		state.exposureValue = *exposureValue;
>> +	frameContext.exposureValue = state.exposureValue;
>> +
>> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>> +	if (frameDurationLimits) {
>> +		/* Limit the control value to the limits in ControlInfo */
>> +		state.minFrameDuration = std::clamp<utils::Duration>(
>> +			std::chrono::microseconds((*frameDurationLimits).front()),
>> +			session.minFrameDuration,
>> +			session.maxFrameDuration
>> +		);
>> +
>> +		state.maxFrameDuration = std::clamp<utils::Duration>(
>> +			std::chrono::microseconds((*frameDurationLimits).back()),
>> +			session.minFrameDuration,
>> +			session.maxFrameDuration
>> +		);
>> +	}
>> +	frameContext.minFrameDuration = state.minFrameDuration;
>> +	frameContext.maxFrameDuration = state.maxFrameDuration;
>> +}
>> +
> 
> This matches RkISP1Agc::queueRequest() (metering mode handling apart)
> 
>> +/**
>> + * \brief Handle a \a prepare operation
>> + */
>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
>> +{
>> +	uint32_t activeAutoExposure = state.automatic.exposure;
>> +	double activeAutoGain = state.automatic.gain;
>> +	double activeAutoQGain = state.automatic.quantizationGain;
>> +
>> +	/* Populate exposure and gain in auto mode */
>> +	if (frameContext.autoExposureEnabled) {
>> +		frameContext.exposure = activeAutoExposure;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +	if (frameContext.autoGainEnabled) {
>> +		frameContext.gain = activeAutoGain;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +
>> +	/*
>> +	 * Populate manual exposure and gain from the active auto values when
>> +	 * transitioning from auto to manual
>> +	 */
> 
> I guess this answers to my question on the auto->manual transition
> tracking for the frameContext.autoExposureModeChange flag.
> 
> It tracks both state changes, but we use it to propagate auto values
> to the manual state if they have not been overriden by an application
> provide control

As mentioned above, I think it very specifically only tracks those auto->manual
transition that don't have an associated manual `ExposureTime`.


> 
>> +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
>> +		state.manual.exposure = activeAutoExposure;
>> +		frameContext.exposure = activeAutoExposure;
>> +	}
>> +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
>> +		state.manual.gain = activeAutoGain;
>> +		frameContext.gain = activeAutoGain;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +
>> +	frameContext.yTarget = state.automatic.yTarget;
>> +}
> 
> Ack, this matches RkISP1Agc::prepare()
> 
>> +
>> +/**
>> + * \brief Handle a \a process operation
>> + */
>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>> +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
>> +			   ControlList &metadata)
>> +{
>> +	const utils::Duration &lineDuration = session.lineDuration;
>> +	utils::Duration newExposureTime = {};
>> +
>> +	if (params) {
> 
> I presume (well, you told me) that this matches the
> 
>          if (!stats)
> 
> case in RkISP1.
> 
> I wonder, should
> 
> The RkISP1 IPA does
> 
> 	if (!stats) {
> 		processFrameDuration(context, frameContext,
> 				     frameContext.agc.minFrameDuration);
> 		fillMetadata(context, frameContext, metadata);
> 		return;
> 	}
> 
> I guess it's fine for IPA to call AgcAlgorithm::process() even when
> !stats, as it is simpler for them to make the call unconditionally, so
> I'm not against this.
> 
> But maybe we can do like RkISP1 does (split processFrameDuration() and
> fillMetadata() to helpers) to save some indentation here.

My preference would be not to add back those separate functions. Maybe
moving the `if (params) { ... }` block into a separate function if
that is acceptable.


> 
>> +		ASSERT(session.autoAllowed);
> 
> Does it mean IPA should not call process if (autoAllowed) ?

Yes.


> 
> Can't we simply bail out here to make IPAs simpler ?

Possibly, but I don't see how that would make them simpler. The assumption
is that `autoAllowed == false` means that statistics will not be available
(and hence no automatic control) (usually for raw capture - but there is no
technical reason why agc could not run even for raw capture if statistics
are available), so providing statistics is an api violation in a sense, so
I think the assert is reasonable.


> 
>> +
>> +		/*
>> +		* Set the AGC limits using the fixed exposure time and/or gain in
>> +		* manual mode, or the sensor limits in auto mode.
>> +		*/
> 
> Weird indent

Fixed.

> 
>> +		utils::Duration minExposureTime;
>> +		utils::Duration maxExposureTime;
>> +		double minAnalogueGain;
>> +		double maxAnalogueGain;
>> +
>> +		if (frameContext.autoExposureEnabled) {
>> +			minExposureTime = session.minExposureTime;
>> +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
> 
> very long line

Adjusted.


> 
>> +		} else {
>> +			minExposureTime = lineDuration * frameContext.exposure;
>> +			maxExposureTime = minExposureTime;
>> +		}
>> +
>> +		if (frameContext.autoGainEnabled) {
>> +			minAnalogueGain = session.minAnalogueGain;
>> +			maxAnalogueGain = session.maxAnalogueGain;
>> +		} else {
>> +			minAnalogueGain = frameContext.gain;
>> +			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;
> 
> RkISP1 gets exposure and gain from the frameContext.
> 
> I see in the next patches that port RkISP1 to AgcAlgorithm this hunk
> 
>          agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,
> 
>                          ...
> 
>                         .exposure = frameContext.sensor.exposure,
>                         /*
>                          * Include the quantization gain if it was applied. Do not use
>                          * compress.gain because it will include gains that shall not be
>                          * reported to the user when HDR is implemented.
>                          */
>                         .gain = frameContext.sensor.gain
>                                 * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> 
> Is the reason you can't take exposure and gain from the FrameContext
> the gain adjustment ?

That, and the fact that it is `frameContext.sensor`, which is separate
from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs
the exposure/gain that was actually in effect.


> 
>> +
>> +		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),
>> +		});
>> +
>> +		LOG(Agc, Debug)
>> +			<< "Divided up exposure time, analogue gain, quantization gain"
>> +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
>> +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
>> +
>> +		/* Update the estimated exposure and gain. */
>> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
>> +		state.automatic.gain = newEv.analogueGain;
>> +		state.automatic.quantizationGain = newEv.quantizationGain;
>> +		state.automatic.yTarget = newEv.yTarget;
>> +
>> +		newExposureTime = newEv.exposureTime;
>> +	}
>> +
>> +	/*
>> +	 * Expand the target frame duration so that we do not run faster than
>> +	 * the minimum frame duration when we have short exposures.
>> +	 */
>> +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
>> +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
>> +
>> +	/* Update frame duration accounting for line length quantization. */
>> +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
> 
> Easy to shorten to
> 
> 	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)
> 				   * lineDuration;
> 

This was also copied verbatim.


>> +
>> +	metadata.set(controls::AnalogueGain, frameContext.gain);
>> +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> 
> I'm not sure I understand the strategy here.. this line is very long
> and could be easily made
> 
> 	metadata.set(controls::ExposureTime,
> 		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> 

Adjusted.


> 
>> +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
>> +	metadata.set(controls::ExposureTimeMode,
>> +		     frameContext.autoExposureEnabled
>> +		     ? controls::ExposureTimeModeAuto
>> +		     : controls::ExposureTimeModeManual);
>> +	metadata.set(controls::AnalogueGainMode,
>> +		     frameContext.autoGainEnabled
>> +		     ? controls::AnalogueGainModeAuto
>> +		     : controls::AnalogueGainModeManual);
> 
> These ones instead are very short and could easily be made
> 
> 	metadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?
> 		     controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);


These were copied verbatim. So please confirm if you want me to adjust the
verbatim copies here and in the whole file.


> 
>> +
>> +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
>> +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
>> +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
>> +}
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> index 5247425952..1eace12908 100644
>> --- a/src/ipa/libipa/agc.h
>> +++ b/src/ipa/libipa/agc.h
>> @@ -7,13 +7,19 @@
>>
>>   #pragma once
>>
>> +#include <optional>
>>   #include <utility>
>>
>>   #include <linux/v4l2-controls.h>
>>
>> +#include <libcamera/control_ids.h>
>>   #include <libcamera/controls.h>
>>
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include "agc_mean_luminance.h"
>>   #include "camera_sensor_helper.h"
>> +#include "histogram.h"
>>
>>   namespace libcamera {
>>
>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>>   	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>>   }
>>
>> +struct Session {
>> +	utils::Duration minExposureTime;
>> +	utils::Duration maxExposureTime;
>> +	double minAnalogueGain;
>> +	double maxAnalogueGain;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +
>> +	utils::Duration lineDuration;
>> +
>> +	struct {
>> +		Size outputSize;
>> +	} sensor;
>> +
>> +	bool autoAllowed;
>> +};
>> +
>> +struct ActiveState {
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +	} manual;
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +		double quantizationGain;
>> +		double yTarget;
>> +	} automatic;
>> +
>> +	bool autoExposureEnabled;
>> +	bool autoGainEnabled;
>> +	double exposureValue;
>> +	controls::AeConstraintModeEnum constraintMode;
>> +	controls::AeExposureModeEnum exposureMode;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +};
>> +
>> +struct FrameContext {
>> +	uint32_t exposure;
>> +	double gain;
>> +	double quantizationGain;
>> +	double exposureValue;
>> +	double yTarget;
>> +	uint32_t vblank;
>> +	bool autoExposureEnabled;
>> +	bool autoGainEnabled;
>> +	controls::AeConstraintModeEnum constraintMode;
>> +	controls::AeExposureModeEnum exposureMode;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +	utils::Duration frameDuration;
>> +	bool autoExposureModeChange;
>> +	bool autoGainModeChange;
>> +};
>> +
>>   } /* namespace agc */
>>
>> +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
> 
> easy to shorten

Done.


> 
>> +
>> +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
>> +			  agc::FrameContext &frameContext, const ControlList &controls);
>> +
>> +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
>> +
>> +	struct ProcessParams {
>> +		const AgcMeanLuminance::Traits &traits;
>> +		const Histogram &yHist;
>> +		uint32_t exposure;
>> +		double gain;
>> +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
>> +		double lux = 0;
>> +	};
>> +
>> +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
> 
> same here

Done.


> 
>> +		     std::optional<ProcessParams> &&params, ControlList &metadata);
>> +
>> +private:
>> +	AgcMeanLuminance impl_;
>> +};
>> +
>>   } /* namespace ipa */
>>
>>   } /* namespace libcamera */
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index 05f1a8749c..ca681fa5af 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -23,6 +23,7 @@ libipa_headers = files([
>>   ])
>>
>>   libipa_sources = files([
>> +    'agc.cpp',
>>       'agc_mean_luminance.cpp',
>>       'algorithm.cpp',
>>       'awb_bayes.cpp',
> 
> Enormous work overall!
> 
> Agc is the most complex beast we have and I'm looking forwards to
> getting this merged already!
> 
> 
>> --
>> 2.55.0
>>

Hi Barnabás

On Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:
> 2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
> > > Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
> > > based on the rkisp1 `Agc` algorithm, with slight adjustments.
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
> > >   src/ipa/libipa/agc.h       |  98 ++++++
> > >   src/ipa/libipa/meson.build |   1 +
> > >   3 files changed, 738 insertions(+)
> > >   create mode 100644 src/ipa/libipa/agc.cpp
> > >
> > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> > > new file mode 100644
> > > index 0000000000..e16a02fdde
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/agc.cpp
> > > @@ -0,0 +1,639 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021-2022, 2026 Ideas On Board
> > > + *
> > > + * Generic AGC algorithm
> > > + */
> > > +
> > > +#include "agc.h"
> > > +
> > > +#include <algorithm>
> > > +#include <array>
> > > +#include <chrono>
> > > +#include <optional>
> > > +
> > > +#include <linux/v4l2-controls.h>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/control_ids.h>
> > > +#include <libcamera/controls.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa {
> > > +
> > > +using namespace std::chrono_literals;
> > > +
> > > +LOG_DEFINE_CATEGORY(Agc)
> > > +
> > > +/**
> > > + * \class AgcAlgorithm
> > > + * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
> > > + *
> > > + * \todo DigitalGain, DigitalGainMode
> > > + */
> > > +
> > > +/**
> > > + * \struct agc::Session
> > > + * \brief Session configuration for AgcAlgorithm
> > > + *
> > > + * \var agc::Session::minExposureTime
> > > + * \brief Minimum exposure time supported with the configured sensor
> > > + *
> > > + * \var agc::Session::maxExposureTime
> > > + * \brief Maximum exposure time supported with the configured sensor
> > > + *
> > > + * \var agc::Session::minAnalogueGain
> > > + * \brief Minimum analogue gain supported with the configured sensor
> > > + *
> > > + * \var agc::Session::maxAnalogueGain
> > > + * \brief Maximum analogue gain supported with the configured sensor
> > > + *
> > > + * \var agc::Session::minFrameDuration
> > > + * \brief Minimum frame duration supported with the configured sensor
> > > + *
> > > + * \var agc::Session::maxFrameDuration
> > > + * \brief Maximum frame duration supported with the configured sensor
> >
> > I would use "for the streaming session" in place of "with the
> > configured sensor" in all previous entries.
> >
> > yes, those settings depend on the sensor configuration but I found the
> > wording a bit confusing
> >
> > Or maybe "for the current sensor configuration" ?
>
> I'll drop "supported" and use "for the streaming session".
>
>

thanks

> >
> > > + *
> > > + * \var agc::Session::lineDuration
> > > + * \brief Line duration with the configured sensor and output size
> > > + *
> > > + * \var agc::Session::sensor
> > > + * \brief Details of the sensor configuration
> > > + *
> > > + * \var agc::Session::sensor.outputSize
> > > + * \brief Configured output size of the sensor
> > > + *
> > > + * \var agc::Session::autoAllowed
> > > + * \brief Whether automatic controls are allowed
> >
> > As I was really confused by this, is it worth adding one line saying
> > that auto is not allowed when capturing raw frames ?
>
> I suppose if statistics are provided, then it can work for raw capture as well.
> So my suggestion:
>
>   \brief Whether automatic controls are allowed
>   Determines whether statistics are generally expected to be available for

s/generally//

>   automatic exposure and gain control. If \a false, statistics must not
>   be provided to \a AgcAlgorithm::process(), and \a ExposureTimeMode and
>   \a AnalogueGainMode will only avertise manual control, and multiple other
>   controls will also be omitted.

Ok.

I would however drop everything after ', and multiple .."
because either we list them or this might sound vague

>
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \struct agc::ActiveState
> > > + * \brief Active state for AgcAlgorithm
> > > + *
> > > + * The \a automatic variables track the latest values computed by algorithm
> > > + * based on the latest processed statistics. All other variables track the
> > > + * consolidated controls requested in queued requests.
> > > + *
> > > + * \var agc::ActiveState::manual
> > > + * \brief Manual exposure time and analog gain (set through requests)
> > > + *
> > > + * \var agc::ActiveState::manual.exposure
> > > + * \brief Manual exposure time expressed as a number of lines as set by the
> > > + * ExposureTime control
> > > + *
> > > + * \var agc::ActiveState::manual.gain
> > > + * \brief Manual analogue gain as set by the AnalogueGain control
> > > + *
> > > + * \var agc::ActiveState::automatic
> > > + * \brief Automatic exposure time and analog gain (computed by the algorithm)
> > > + *
> > > + * \var agc::ActiveState::automatic.exposure
> > > + * \brief Automatic exposure time expressed as a number of lines
> > > + *
> > > + * \var agc::ActiveState::automatic.gain
> > > + * \brief Automatic analogue gain multiplier
> > > + *
> > > + * \var agc::ActiveState::automatic.quantizationGain
> > > + * \brief Automatic quantization gain multiplier
> > > + *
> > > + * \var agc::ActiveState::automatic.yTarget
> > > + * \brief Automatically determined luminance target
> > > + *
> > > + * \var agc::ActiveState::autoExposureEnabled
> > > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> >
> > s/AGC state (exposure)/exposure mode
> >
> > > + *
> > > + * \var agc::ActiveState::autoGainEnabled
> > > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> >
> > same here ?
>
> These were copied verbatim.
>

I don't think we should stick too strictly to what's in rkisp1
especially when it's about documentation which can so easily be
improved

>
> >
> > > + *
> > > + * \var agc::ActiveState::exposureValue
> > > + * \brief Exposure value as set by the ExposureValue control
> > > + *
> > > + * \var agc::ActiveState::constraintMode
> > > + * \brief Constraint mode as set by the AeConstraintMode control
> > > + *
> > > + * \var agc::ActiveState::exposureMode
> > > + * \brief Exposure mode as set by the AeExposureMode control
> > > + *
> > > + * \var agc::ActiveState::minFrameDuration
> > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > + *
> > > + * \var agc::ActiveState::maxFrameDuration
> > > + * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > + */
> > > +
> > > +/**
> > > + * \struct agc::FrameContext
> > > + * \brief Per-frame context for AgcAlgorithm
> > > + *
> > > + * \var agc::FrameContext::exposure
> > > + * \brief Exposure time expressed as a number of lines computed by the algorithm
> > > + *
> > > + * \var agc::FrameContext::gain
> > > + * \brief Analogue gain multiplier computed by the algorithm
> >
> > Are these computed by the algorithms or can these be the manually
> > programmed values when running in manual mode ?
>
> This is always the gain that needs to be applied to the frame, so
> the manual gain in manual mode, the automatic gain in automatic mode.
>

So maybe "computed by the algorithm" isn't exactly correct ?


 * \brief Analogue gain multiplier applied to the frame

?

>
> >
> > I would replace "computed by the algorithm" with "for the frame" or
> > similar
> >
> > > + *
> > > + * The gain should be adapted to the sensor specific gain code before applying.
> >
> > s/adapted/translated ?
>
> These were also all copied verbatim, but I'll adjust it.
>
>
> >
> > > + *
> > > + * \var agc::FrameContext::quantizationGain
> > > + * \brief Quantization gain multiplier computed by the algorithm
> > > + *
> > > + * \var agc::FrameContext::exposureValue
> > > + * \brief Exposure value as set by the ExposureValue control
> > > + *
> > > + * \var agc::FrameContext::yTarget
> > > + * \brief Luminance target computed by the algorithm
> > > + *
> > > + * \var agc::FrameContext::vblank
> > > + * \brief Vertical blanking parameter computed by the algorithm
> > > + *
> > > + * \var agc::FrameContext::autoExposureEnabled
> > > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> > > + *
> > > + * \var agc::FrameContext::autoGainEnabled
> > > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> > > + *
> > > + * \var agc::FrameContext::constraintMode
> > > + * \brief Constraint mode as set by the AeConstraintMode control
> > > + *
> > > + * \var agc::FrameContext::exposureMode
> > > + * \brief Exposure mode as set by the AeExposureMode control
> > > + *
> > > + * \var agc::FrameContext::minFrameDuration
> > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > + *
> > > + * \var agc::FrameContext::maxFrameDuration
> > > + * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > + *
> > > + * \var agc::FrameContext::frameDuration
> > > + * \brief The actual FrameDuration used by the algorithm for the frame
> > > + *
> > > + * \var agc::FrameContext::autoExposureModeChange
> > > + * \brief Indicate if autoExposureEnabled has changed from true in the previous
> > > + * frame to false in the current frame, and no manual exposure value has been
> > > + * supplied in the current frame.
> >
> > does this only track auto->manual transaction or does it track the
> > other way around.
> >
> > Kind of a long brief (and no '.' at the end).
>
> These were also copied verbatim, but I have removed the dots.
>
>
> >
> > I would
> >
> >       \brief Exposure mode change flag
> >
> >       Indicate if the exposure mode has changed compared to the
> >       previous frame.
>
> It only tracks auto -> manual transitions without manual `ExposureTime`,
> so I think the above wouldn't be entirely correct.
>

Yeah, you're right

        if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
                frameContext.autoExposureModeChange = true;

The purpose of the flag is to keep track if the auto value should be
propagated to manual, so it's a bit of an unfortunate name. However,
not something that should be changed in this patch

>
> >
> > > + *
> > > + * \var agc::FrameContext::autoGainModeChange
> > > + * \brief Indicate if autoGainEnabled has changed from true in the previous
> > > + * frame to false in the current frame, and no manual gain value has been
> > > + * supplied in the current frame.
> > > + */
> > > +
> > > +/**
> > > + * \struct AgcAlgorithm::ConfigurationParams
> > > + * \brief Parameters for AgcAlgorithm::configure()
> > > + *
> > > + * \var AgcAlgorithm::ConfigurationParams::sensor
> > > + * \brief CameraSensorHelper for the sensor
> > > + *
> > > + * \var AgcAlgorithm::ConfigurationParams::sensorInfo
> > > + * \brief Details of the sensor
> >
> > The sensor configuration description
>
> I have adjusted it to
>
>   Current configuration of the sensor
>
> hope it is also acceptable.
>

It is indeed

>
> >
> > > + *
> > > + * \var AgcAlgorithm::ConfigurationParams::sensorControls
> > > + * \brief ControlInfoMap of the sensor
> > > + *
> > > + * \var AgcAlgorithm::ConfigurationParams::ctrlMap
> > > + * \brief ControlMap to update with controls
> > > + *
> > > + * \var AgcAlgorithm::ConfigurationParams::autoAllowed
> > > + * \brief Whether to enable auto controls
> > > + */
> > > +
> > > +/**
> > > + * \struct AgcAlgorithm::ProcessParams
> > > + * \brief Parameters for AgcAlgorithm::process()
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::traits
> > > + * \brief Implementation of AgcMeanLuminance::Traits
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::yHist
> > > + * \brief Luminance histogram of the frame
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::exposure
> > > + * \brief Effective exposure of the frame
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::gain
> > > + * \brief Effective gain of the frame
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::additionalConstraints
> > > + * \brief Additional AgcMeanLuminance::AgcConstraints to apply
> > > + *
> > > + * \var AgcAlgorithm::ProcessParams::lux
> > > + * \brief Effective lux value of the frame
> > > + */
> > > +
> > > +/**
> > > + * \brief Load tuning data
> > > + */
> > > +int AgcAlgorithm::init(const ValueNode &tuningData)
> > > +{
> > > +	int ret = impl_.parseTuningData(tuningData);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Initialize the session configuration and active state
> > > + */
> > > +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)
> >
> > Easy to shorten to
> >
> > int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> > 			    const ConfigurationParams &config)
>
> Done.
>
>
> >
> >
> > Seems like session is an out parameter.
> >
> > We used to have a rule: const & for input params, * for output ones.
> > Not sure anymore how much we enforce it these days..
>
> I'm not sure I would consider it an out param in the traditional sense
> (i.e. the caller is not really expected to use/inspect the result), and
> I really prefer the consistency of always passing by reference to every
> function of `AgcAlgorithm`.
>

Ok, as said, I don't exactly know how much that rule is even enforced
nowadays

>
> >
> > > +{
> > > +	session = {};
> > > +	session.lineDuration = config.sensorInfo.minLineLength * 1.0s
> > > +		/ config.sensorInfo.pixelRate;
> > > +	session.sensor.outputSize = config.sensorInfo.outputSize;
> > > +	session.autoAllowed = config.autoAllowed;
> > > +
> > > +	const double lineDurationUs = session.lineDuration.get<std::micro>();
> > > +
> > > +	/*
> > > +	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
> > > +	 * limits and the line duration.
> > > +	 */
> > > +
> > > +	const ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;
> > > +	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> > > +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> > > +	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
> > > +	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
> > > +		static_cast<int32_t>(minExposure * lineDurationUs),
> > > +		static_cast<int32_t>(maxExposure * lineDurationUs),
> > > +		static_cast<int32_t>(defExposure * lineDurationUs),
> > > +	};
> > > +
> > > +	/* Compute the analogue gain limits. */
> > > +	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>());
> > > +	config.ctrlMap[&controls::AnalogueGain] = ControlInfo{
> > > +		minGain,
> > > +		maxGain,
> > > +		defGain,
> > > +	};
> > > +
> > > +	LOG(Agc, Debug)
> > > +		<< "Exposure: [" << minExposure << ", " << maxExposure
> > > +		<< "], gain: [" << minGain << ", " << maxGain << "]";
> > > +
> > > +	/*
> > > +	 * Compute the frame duration limits.
> > > +	 *
> > > +	 * The frame length is computed assuming a fixed line length combined
> > > +	 * with the vertical frame sizes.
> > > +	 */
> > > +	const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
> > > +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> > > +	uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
> > > +
> > > +	const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
> > > +	std::array<uint32_t, 3> frameHeights{
> > > +		v4l2VBlank.min().get<int32_t>() + config.sensorInfo.outputSize.height,
> > > +		v4l2VBlank.max().get<int32_t>() + config.sensorInfo.outputSize.height,
> > > +		v4l2VBlank.def().get<int32_t>() + config.sensorInfo.outputSize.height,
> > > +	};
> > > +
> > > +	std::array<int64_t, 3> frameDurations;
> > > +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> > > +		uint64_t frameSize = lineLength * frameHeights[i];
> > > +		frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);
> > > +	}
> > > +
> > > +	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
> > > +		frameDurations[0],
> > > +		frameDurations[1],
> > > +		Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } },
> > > +	};
> > > +
> > > +	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
> > > +	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
> > > +
> > > +	/*
> > > +	 * When the AGC computes the new exposure values for a frame, it needs
> > > +	 * to know the limits for exposure time and analogue gain. As it depends
> > > +	 * on the sensor, update it with the controls.
> > > +	 *
> > > +	 * \todo take VBLANK into account for maximum exposure time
> > > +	 */
> > > +	session.minExposureTime = minExposure * session.lineDuration;
> > > +	session.maxExposureTime = maxExposure * session.lineDuration;
> > > +	session.minAnalogueGain = minGain;
> > > +	session.maxAnalogueGain = maxGain;
> >
> > The above matches the current RkISP1Agc::reconfigure() content
> >
> > > +
> > > +	impl_.configure(session.lineDuration, config.sensor);
> > > +	impl_.setLimits(session.minExposureTime, session.maxExposureTime,
> > > +			session.minAnalogueGain, session.maxAnalogueGain,
> > > +			{});
> > > +	impl_.resetFrameCount();
> > > +
> > > +	/* Configure the default exposure and gain. */
> > > +	state = {};
> > > +	state.automatic.gain = session.minAnalogueGain;
> > > +	state.automatic.exposure = 10ms / session.lineDuration;
> > > +	state.automatic.quantizationGain = 1;
> > > +	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
> >
> > And this matches RkISP1::Agc() apart from this early yTarget
> > computation which I presume is intentional
>
> Hmmm... that should match as well. There is
>
>   context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);
>
> at the end of `rkisp1.cpp:Agc::configure()`.

Ah yes, it's at the end of the function indeed

>
>
> >
> > > +	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;
> > > +
> > > +	const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {
> > > +		std::array<ControlValue, 2> values;
> > > +		size_t count = 0;
> > > +
> > > +		if (session.autoAllowed)
> > > +			values[count++] = ControlValue(automatic);
> > > +
> > > +		values[count++] = ControlValue(manual);
> > > +
> > > +		config.ctrlMap[&cid] = ControlInfo{
> > > +			{ values.data(), count },
> > > +			ControlValue(session.autoAllowed ? automatic : manual),
> > > +		};
> > > +	};
> > > +
> > > +	add(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
> > > +	add(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
> >
> > Could you please break trivially re-adjustable long lines ?
>
> Adjusted to
>
>   add(controls::ExposureTimeMode,
>       controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
>   add(controls::AnalogueGainMode,
>       controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>
> Does that look better?
>

indeed

>
> >
> > > +
> > > +	/* \todo Move this to the `Camera` class. */
> > > +	config.ctrlMap[&controls::AeEnable] = ControlInfo{
> > > +		false,
> > > +		session.autoAllowed,
> > > +		session.autoAllowed,
> > > +	};
> >
> > RkISP1Agc register controls in init() but I think what you're doing
> > here it's better, as some control such as ExposureTimeMode depend on
> > the session configuration.
> >
> > Is there any risk of having leftovers from a previous session in the
> > control list ?
>
> Every control should either be overwritten or removed.
>

Is that because the Camera::controls info map gets reset between
configure() calls ? I don't really see that happening in Camera or
PipelineHandler.

Same as per the data->ipaControls_ passed by the RkISP1 pipeline handle
to the IPA and used to populate Camera::controls().

>
> >
> > > +
> > > +	if (session.autoAllowed) {
> > > +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > > +
> > > +		for (const auto &[id, info] : impl_.controls())
> > > +			config.ctrlMap[id] = info;
> > > +	} else {
> > > +		config.ctrlMap.erase(&controls::ExposureValue);
> > > +
> > > +		for (const auto &[id, info] : impl_.controls())
> > > +			config.ctrlMap.erase(id);
> > > +	}
> >
> > Maybe it happens later on, but this is a bit cumbersome. Making
> > AgcMeanLuminance aware of autoAllowed would avoid this
>
> Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.
> And in any case, there will be a loop like this or equivalent, and

Ok, I thought it was AgcMeanLuminance registering ExposureValue but it
actually is this class doing so it here. What we're erasing are
possible leftovers from a previous session maybe ?

> I'm not sure it matters too much where it is.

I was bothered by the need to erase, as it seems like a lot of
bookeeping that could possibile go wrong.

But if I get this right this is about removing controls from a
previous session ? If that's the case should we pass a fresh list of
controls at every ::configure() call to avoid leftovers instead of
erasing them ?

>
>
> >
> > > +
> > > +
> >
> > Double empty line
>
> Fixed.
>
>
> >
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Handle a \a queueRequest operation
> > > + */
> > > +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
> > > +				agc::FrameContext &frameContext, const ControlList &controls)
> > > +{
> > > +	if (session.autoAllowed) {
> >
> > Now that you register controls at configure() time and not init()
> > time, am I correct that if !autoAllowed applications won't be able to select
> > controls::ExposureTimeModeAuto ?
>
> Yes.
>
>
> >
> > Which makes me wonder if controls::ExposureTimeMode should be
> > registered at all if !autoAllowed
>
> I think that's more of a philosophical question. My preference is to
> register controls even if they only really have one valid value. Maybe
> this needs more discussion to arrive at a libcamera-wide policy.

Ack, let's ask others

>
>
> >
> > > +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> > > +		if (aeEnable &&
> > > +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {
> > > +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
> > > +
> > > +			LOG(Agc, Debug)
> > > +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
> > > +				<< " AGC (exposure)";
> > > +
> > > +			/*
> > > +			 * If we go from auto -> manual with no manual control
> > > +			 * set, use the last computed value, which we don't
> > > +			 * know until prepare() so save this information.
> > > +			 *
> > > +			 * \todo Check the previous frame at prepare() time
> > > +			 * instead of saving a flag here
> > > +			 */
> > > +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
> > > +				frameContext.autoExposureModeChange = true;
> > > +		}
> > > +
> > > +		const auto &agEnable = controls.get(controls::AnalogueGainMode);
> >
> > Same reasoning for AnalogueGainMode
> >
> > > +		if (agEnable &&
> > > +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
> > > +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> > > +
> > > +			LOG(Agc, Debug)
> > > +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
> > > +				<< " AGC (gain)";
> > > +			/*
> > > +			 * If we go from auto -> manual with no manual control
> > > +			 * set, use the last computed value, which we don't
> > > +			 * know until prepare() so save this information.
> > > +			 */
> > > +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
> > > +				frameContext.autoGainModeChange = true;
> > > +		}
> > > +	}
> > > +
> > > +	const auto &exposure = controls.get(controls::ExposureTime);
> > > +	if (exposure && !state.autoExposureEnabled) {
> > > +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
> > > +
> > > +		LOG(Agc, Debug)
> > > +			<< "Set exposure to " << state.manual.exposure;
> > > +	}
> > > +
> > > +	const auto &gain = controls.get(controls::AnalogueGain);
> > > +	if (gain && !state.autoGainEnabled) {
> > > +		state.manual.gain = *gain;
> > > +
> > > +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
> > > +	}
> > > +
> > > +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
> > > +	frameContext.autoGainEnabled = state.autoGainEnabled;
> > > +
> > > +	if (!frameContext.autoExposureEnabled)
> > > +		frameContext.exposure = state.manual.exposure;
> > > +	if (!frameContext.autoGainEnabled)
> > > +		frameContext.gain = state.manual.gain;
> > > +
> > > +	if (!frameContext.autoExposureEnabled &&
> > > +	    !frameContext.autoGainEnabled)
> > > +		frameContext.quantizationGain = 1.0;
> > > +
> > > +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> > > +	if (exposureMode)
> > > +		state.exposureMode =
> > > +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
> > > +	frameContext.exposureMode = state.exposureMode;
> > > +
> > > +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> > > +	if (constraintMode)
> > > +		state.constraintMode =
> > > +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> > > +	frameContext.constraintMode = state.constraintMode;
> > > +
> > > +	const auto &exposureValue = controls.get(controls::ExposureValue);
> > > +	if (exposureValue)
> > > +		state.exposureValue = *exposureValue;
> > > +	frameContext.exposureValue = state.exposureValue;
> > > +
> > > +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > > +	if (frameDurationLimits) {
> > > +		/* Limit the control value to the limits in ControlInfo */
> > > +		state.minFrameDuration = std::clamp<utils::Duration>(
> > > +			std::chrono::microseconds((*frameDurationLimits).front()),
> > > +			session.minFrameDuration,
> > > +			session.maxFrameDuration
> > > +		);
> > > +
> > > +		state.maxFrameDuration = std::clamp<utils::Duration>(
> > > +			std::chrono::microseconds((*frameDurationLimits).back()),
> > > +			session.minFrameDuration,
> > > +			session.maxFrameDuration
> > > +		);
> > > +	}
> > > +	frameContext.minFrameDuration = state.minFrameDuration;
> > > +	frameContext.maxFrameDuration = state.maxFrameDuration;
> > > +}
> > > +
> >
> > This matches RkISP1Agc::queueRequest() (metering mode handling apart)
> >
> > > +/**
> > > + * \brief Handle a \a prepare operation
> > > + */
> > > +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
> > > +{
> > > +	uint32_t activeAutoExposure = state.automatic.exposure;
> > > +	double activeAutoGain = state.automatic.gain;
> > > +	double activeAutoQGain = state.automatic.quantizationGain;
> > > +
> > > +	/* Populate exposure and gain in auto mode */
> > > +	if (frameContext.autoExposureEnabled) {
> > > +		frameContext.exposure = activeAutoExposure;
> > > +		frameContext.quantizationGain = activeAutoQGain;
> > > +	}
> > > +	if (frameContext.autoGainEnabled) {
> > > +		frameContext.gain = activeAutoGain;
> > > +		frameContext.quantizationGain = activeAutoQGain;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Populate manual exposure and gain from the active auto values when
> > > +	 * transitioning from auto to manual
> > > +	 */
> >
> > I guess this answers to my question on the auto->manual transition
> > tracking for the frameContext.autoExposureModeChange flag.
> >
> > It tracks both state changes, but we use it to propagate auto values
> > to the manual state if they have not been overriden by an application
> > provide control
>
> As mentioned above, I think it very specifically only tracks those auto->manual
> transition that don't have an associated manual `ExposureTime`.
>

Yes you're right

>
> >
> > > +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
> > > +		state.manual.exposure = activeAutoExposure;
> > > +		frameContext.exposure = activeAutoExposure;
> > > +	}
> > > +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
> > > +		state.manual.gain = activeAutoGain;
> > > +		frameContext.gain = activeAutoGain;
> > > +		frameContext.quantizationGain = activeAutoQGain;
> > > +	}
> > > +
> > > +	frameContext.yTarget = state.automatic.yTarget;
> > > +}
> >
> > Ack, this matches RkISP1Agc::prepare()
> >
> > > +
> > > +/**
> > > + * \brief Handle a \a process operation
> > > + */
> > > +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > > +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
> > > +			   ControlList &metadata)
> > > +{
> > > +	const utils::Duration &lineDuration = session.lineDuration;
> > > +	utils::Duration newExposureTime = {};
> > > +
> > > +	if (params) {
> >
> > I presume (well, you told me) that this matches the
> >
> >          if (!stats)
> >
> > case in RkISP1.
> >
> > I wonder, should
> >
> > The RkISP1 IPA does
> >
> > 	if (!stats) {
> > 		processFrameDuration(context, frameContext,
> > 				     frameContext.agc.minFrameDuration);
> > 		fillMetadata(context, frameContext, metadata);
> > 		return;
> > 	}
> >
> > I guess it's fine for IPA to call AgcAlgorithm::process() even when
> > !stats, as it is simpler for them to make the call unconditionally, so
> > I'm not against this.
> >
> > But maybe we can do like RkISP1 does (split processFrameDuration() and
> > fillMetadata() to helpers) to save some indentation here.
>
> My preference would be not to add back those separate functions. Maybe
> moving the `if (params) { ... }` block into a separate function if
> that is acceptable.

I guess we're discussing tastes, but I usually get suspicious when
code gets to 4 indentation levels and we could easily avoid that

>
>
> >
> > > +		ASSERT(session.autoAllowed);
> >
> > Does it mean IPA should not call process if (autoAllowed) ?
>
> Yes.
>

Well, not exactly.

The IPA shall not provide stats if !autoAllowed

>
> >
> > Can't we simply bail out here to make IPAs simpler ?
>
> Possibly, but I don't see how that would make them simpler. The assumption

I would have liked for IPAs to be able to call ::process() with an
optionally initialized ProcessParams which in case of !stats
would be empty. If the core is running with !autoAllowed it will
ignore the params.

This doesn't work in case the system is running with autoAllowed but
for some reasons you don't have valid stats. RkISP1 seems to assume it
might happen you can get bad/corrupted stats, however in practice I'm
not sure how it could happen.


What you have right now (RkISP1 as an example)

	if (stats) {
		if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
			params = &stats->params;
		else
			LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
	}

	if (params) {
		std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
		if (context.activeState.wdr.mode != controls::WdrOff)
			additionalConstraints.push_back(context.activeState.wdr.constraint);

		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
			.traits = AgcTraits{
				{ params->ae.exp_mean, context.hw.numAeCells },
				meteringModes_.at(frameContext.agc.meteringMode),
			},
			.yHist = {
				/* The lower 4 bits are fractional and meant to be discarded. */
				{ params->hist.hist_bins, context.hw.numHistogramBins },
				[](uint32_t x) { return x >> 4; },
			},
			.exposure = frameContext.sensor.exposure,
			/*
			 * Include the quantization gain if it was applied. Do not use
			 * compress.gain because it will include gains that shall not be
			 * reported to the user when HDR is implemented.
			 */
			.gain = frameContext.sensor.gain
			        * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
			.additionalConstraints = std::move(additionalConstraints),
			.lux = frameContext.lux.lux,
		}}, metadata);
	} else {
		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
	}

Requires the IPA to validate stats and have to distinct calls to
agc_.process()

while

        agc::ProcessParams params {};
        if (stats && stats & ... )
                /* populate params. */
        }

        agc_.process( ... , ...,  params, ... )

is less verbose, but would require the core to bail out in case
(params.empty() || !autoAllowed)

This mean you would need a way to distinguish empty param blocks and
you can't use the declarative style you have here, which I kind of
like as well..

not sure, I have a feeling IPA could be made simpler, but at the same
time this seems more robust

> is that `autoAllowed == false` means that statistics will not be available
> (and hence no automatic control) (usually for raw capture - but there is no
> technical reason why agc could not run even for raw capture if statistics
> are available), so providing statistics is an api violation in a sense, so
> I think the assert is reasonable.
>

We can start strict and see. After all this is not something
application could trigger.

>
> >
> > > +
> > > +		/*
> > > +		* Set the AGC limits using the fixed exposure time and/or gain in
> > > +		* manual mode, or the sensor limits in auto mode.
> > > +		*/
> >
> > Weird indent
>
> Fixed.
>
> >
> > > +		utils::Duration minExposureTime;
> > > +		utils::Duration maxExposureTime;
> > > +		double minAnalogueGain;
> > > +		double maxAnalogueGain;
> > > +
> > > +		if (frameContext.autoExposureEnabled) {
> > > +			minExposureTime = session.minExposureTime;
> > > +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
> >
> > very long line
>
> Adjusted.
>
>
> >
> > > +		} else {
> > > +			minExposureTime = lineDuration * frameContext.exposure;
> > > +			maxExposureTime = minExposureTime;
> > > +		}
> > > +
> > > +		if (frameContext.autoGainEnabled) {
> > > +			minAnalogueGain = session.minAnalogueGain;
> > > +			maxAnalogueGain = session.maxAnalogueGain;
> > > +		} else {
> > > +			minAnalogueGain = frameContext.gain;
> > > +			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;
> >
> > RkISP1 gets exposure and gain from the frameContext.
> >
> > I see in the next patches that port RkISP1 to AgcAlgorithm this hunk
> >
> >          agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,
> >
> >                          ...
> >
> >                         .exposure = frameContext.sensor.exposure,
> >                         /*
> >                          * Include the quantization gain if it was applied. Do not use
> >                          * compress.gain because it will include gains that shall not be
> >                          * reported to the user when HDR is implemented.
> >                          */
> >                         .gain = frameContext.sensor.gain
> >                                 * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> >
> > Is the reason you can't take exposure and gain from the FrameContext
> > the gain adjustment ?
>
> That, and the fact that it is `frameContext.sensor`, which is separate
> from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs
> the exposure/gain that was actually in effect.
>

Ack, too bad, the Agc::process() interface is quite verbose already :(

>
> >
> > > +
> > > +		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),
> > > +		});
> > > +
> > > +		LOG(Agc, Debug)
> > > +			<< "Divided up exposure time, analogue gain, quantization gain"
> > > +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
> > > +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
> > > +
> > > +		/* Update the estimated exposure and gain. */
> > > +		state.automatic.exposure = newEv.exposureTime / lineDuration;
> > > +		state.automatic.gain = newEv.analogueGain;
> > > +		state.automatic.quantizationGain = newEv.quantizationGain;
> > > +		state.automatic.yTarget = newEv.yTarget;
> > > +
> > > +		newExposureTime = newEv.exposureTime;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Expand the target frame duration so that we do not run faster than
> > > +	 * the minimum frame duration when we have short exposures.
> > > +	 */
> > > +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
> > > +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
> > > +
> > > +	/* Update frame duration accounting for line length quantization. */
> > > +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
> >
> > Easy to shorten to
> >
> > 	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)
> > 				   * lineDuration;
> >
>
> This was also copied verbatim.
>
>
> > > +
> > > +	metadata.set(controls::AnalogueGain, frameContext.gain);
> > > +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> >
> > I'm not sure I understand the strategy here.. this line is very long
> > and could be easily made
> >
> > 	metadata.set(controls::ExposureTime,
> > 		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> >
>
> Adjusted.
>
>
> >
> > > +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
> > > +	metadata.set(controls::ExposureTimeMode,
> > > +		     frameContext.autoExposureEnabled
> > > +		     ? controls::ExposureTimeModeAuto
> > > +		     : controls::ExposureTimeModeManual);
> > > +	metadata.set(controls::AnalogueGainMode,
> > > +		     frameContext.autoGainEnabled
> > > +		     ? controls::AnalogueGainModeAuto
> > > +		     : controls::AnalogueGainModeManual);
> >
> > These ones instead are very short and could easily be made
> >
> > 	metadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?
> > 		     controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);
>
>
> These were copied verbatim. So please confirm if you want me to adjust the
> verbatim copies here and in the whole file.

As said, it's not that we're moving code in a single patch where we
can compare the lines moved around in the diff.

The way I did the review was looking at the code in Agc and RkISP1Agc
and they are of course different already, so I don't think we should
stick to verbatim copies when there's something trivially improvable
like spanning to the whole line length

>
>
> >
> > > +
> > > +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
> > > +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
> > > +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
> > > +}
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > index 5247425952..1eace12908 100644
> > > --- a/src/ipa/libipa/agc.h
> > > +++ b/src/ipa/libipa/agc.h
> > > @@ -7,13 +7,19 @@
> > >
> > >   #pragma once
> > >
> > > +#include <optional>
> > >   #include <utility>
> > >
> > >   #include <linux/v4l2-controls.h>
> > >
> > > +#include <libcamera/control_ids.h>
> > >   #include <libcamera/controls.h>
> > >
> > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > +
> > > +#include "agc_mean_luminance.h"
> > >   #include "camera_sensor_helper.h"
> > > +#include "histogram.h"
> > >
> > >   namespace libcamera {
> > >
> > > @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
> > >   	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
> > >   }
> > >
> > > +struct Session {
> > > +	utils::Duration minExposureTime;
> > > +	utils::Duration maxExposureTime;
> > > +	double minAnalogueGain;
> > > +	double maxAnalogueGain;
> > > +	utils::Duration minFrameDuration;
> > > +	utils::Duration maxFrameDuration;
> > > +
> > > +	utils::Duration lineDuration;
> > > +
> > > +	struct {
> > > +		Size outputSize;
> > > +	} sensor;
> > > +
> > > +	bool autoAllowed;
> > > +};
> > > +
> > > +struct ActiveState {
> > > +	struct {
> > > +		uint32_t exposure;
> > > +		double gain;
> > > +	} manual;
> > > +	struct {
> > > +		uint32_t exposure;
> > > +		double gain;
> > > +		double quantizationGain;
> > > +		double yTarget;
> > > +	} automatic;
> > > +
> > > +	bool autoExposureEnabled;
> > > +	bool autoGainEnabled;
> > > +	double exposureValue;
> > > +	controls::AeConstraintModeEnum constraintMode;
> > > +	controls::AeExposureModeEnum exposureMode;
> > > +	utils::Duration minFrameDuration;
> > > +	utils::Duration maxFrameDuration;
> > > +};
> > > +
> > > +struct FrameContext {
> > > +	uint32_t exposure;
> > > +	double gain;
> > > +	double quantizationGain;
> > > +	double exposureValue;
> > > +	double yTarget;
> > > +	uint32_t vblank;
> > > +	bool autoExposureEnabled;
> > > +	bool autoGainEnabled;
> > > +	controls::AeConstraintModeEnum constraintMode;
> > > +	controls::AeExposureModeEnum exposureMode;
> > > +	utils::Duration minFrameDuration;
> > > +	utils::Duration maxFrameDuration;
> > > +	utils::Duration frameDuration;
> > > +	bool autoExposureModeChange;
> > > +	bool autoGainModeChange;
> > > +};
> > > +
> > >   } /* namespace agc */
> > >
> > > +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
> >
> > easy to shorten
>
> Done.
>
>
> >
> > > +
> > > +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
> > > +			  agc::FrameContext &frameContext, const ControlList &controls);
> > > +
> > > +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
> > > +
> > > +	struct ProcessParams {
> > > +		const AgcMeanLuminance::Traits &traits;
> > > +		const Histogram &yHist;
> > > +		uint32_t exposure;
> > > +		double gain;
> > > +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
> > > +		double lux = 0;
> > > +	};
> > > +
> > > +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
> >
> > same here
>
> Done.
>
>
> >
> > > +		     std::optional<ProcessParams> &&params, ControlList &metadata);
> > > +
> > > +private:
> > > +	AgcMeanLuminance impl_;
> > > +};
> > > +
> > >   } /* namespace ipa */
> > >
> > >   } /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 05f1a8749c..ca681fa5af 100644
> > > --- a/src/ipa/libipa/meson.build
> > > +++ b/src/ipa/libipa/meson.build
> > > @@ -23,6 +23,7 @@ libipa_headers = files([
> > >   ])
> > >
> > >   libipa_sources = files([
> > > +    'agc.cpp',
> > >       'agc_mean_luminance.cpp',
> > >       'algorithm.cpp',
> > >       'awb_bayes.cpp',
> >
> > Enormous work overall!
> >
> > Agc is the most complex beast we have and I'm looking forwards to
> > getting this merged already!
> >
> >
> > > --
> > > 2.55.0
> > >
>

2026. 07. 27. 15:25 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:
>> 2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
>>>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
>>>> based on the rkisp1 `Agc` algorithm, with slight adjustments.
>>>>
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
>>>>    src/ipa/libipa/agc.h       |  98 ++++++
>>>>    src/ipa/libipa/meson.build |   1 +
>>>>    3 files changed, 738 insertions(+)
>>>>    create mode 100644 src/ipa/libipa/agc.cpp
>>>>
>>>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>>>> new file mode 100644
>>>> index 0000000000..e16a02fdde
>>>> --- /dev/null
>>>> +++ b/src/ipa/libipa/agc.cpp
>>>> @@ -0,0 +1,639 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2021-2022, 2026 Ideas On Board
>>>> + *
>>>> + * Generic AGC algorithm
>>>> + */
>>>> +
>>>> +#include "agc.h"
>>>> +
>>>> +#include <algorithm>
>>>> +#include <array>
>>>> +#include <chrono>
>>>> +#include <optional>
>>>> +
>>>> +#include <linux/v4l2-controls.h>
>>>> +
>>>> +#include <libcamera/base/log.h>
>>>> +
>>>> +#include <libcamera/control_ids.h>
>>>> +#include <libcamera/controls.h>
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +namespace ipa {
>>>> +
>>>> +using namespace std::chrono_literals;
>>>> +
>>>> +LOG_DEFINE_CATEGORY(Agc)
>>>> +
>>>> +/**
>>>> + * \class AgcAlgorithm
>>>> + * \brief AgcMeanLuminance wrapper for implementing the Algorithm interface
>>>> + *
>>>> + * \todo DigitalGain, DigitalGainMode
>>>> + */
>>>> +
>>>> +/**
>>>> + * \struct agc::Session
>>>> + * \brief Session configuration for AgcAlgorithm
>>>> + *
>>>> + * \var agc::Session::minExposureTime
>>>> + * \brief Minimum exposure time supported with the configured sensor
>>>> + *
>>>> + * \var agc::Session::maxExposureTime
>>>> + * \brief Maximum exposure time supported with the configured sensor
>>>> + *
>>>> + * \var agc::Session::minAnalogueGain
>>>> + * \brief Minimum analogue gain supported with the configured sensor
>>>> + *
>>>> + * \var agc::Session::maxAnalogueGain
>>>> + * \brief Maximum analogue gain supported with the configured sensor
>>>> + *
>>>> + * \var agc::Session::minFrameDuration
>>>> + * \brief Minimum frame duration supported with the configured sensor
>>>> + *
>>>> + * \var agc::Session::maxFrameDuration
>>>> + * \brief Maximum frame duration supported with the configured sensor
>>>
>>> I would use "for the streaming session" in place of "with the
>>> configured sensor" in all previous entries.
>>>
>>> yes, those settings depend on the sensor configuration but I found the
>>> wording a bit confusing
>>>
>>> Or maybe "for the current sensor configuration" ?
>>
>> I'll drop "supported" and use "for the streaming session".
>>
>>
> 
> thanks
> 
>>>
>>>> + *
>>>> + * \var agc::Session::lineDuration
>>>> + * \brief Line duration with the configured sensor and output size
>>>> + *
>>>> + * \var agc::Session::sensor
>>>> + * \brief Details of the sensor configuration
>>>> + *
>>>> + * \var agc::Session::sensor.outputSize
>>>> + * \brief Configured output size of the sensor
>>>> + *
>>>> + * \var agc::Session::autoAllowed
>>>> + * \brief Whether automatic controls are allowed
>>>
>>> As I was really confused by this, is it worth adding one line saying
>>> that auto is not allowed when capturing raw frames ?
>>
>> I suppose if statistics are provided, then it can work for raw capture as well.
>> So my suggestion:
>>
>>    \brief Whether automatic controls are allowed
>>    Determines whether statistics are generally expected to be available for
> 
> s/generally//
> 
>>    automatic exposure and gain control. If \a false, statistics must not
>>    be provided to \a AgcAlgorithm::process(), and \a ExposureTimeMode and
>>    \a AnalogueGainMode will only avertise manual control, and multiple other
>>    controls will also be omitted.
> 
> Ok.
> 
> I would however drop everything after ', and multiple .."
> because either we list them or this might sound vague

Current version:

  * \brief Whether to enable auto controls
  *
  * If \a false, the algorithm is set up for manual exposure and gain
  * control only, without automatic adjustments. In this mode statistics
  * must not be provided to AgcAlgorithm::process(), and ExposureTimeMode
  * and AnalogueGainMode will only advertise manual control.


> 
>>
>>
>>>
>>>> + */
>>>> +
>>>> +/**
>>>> + * \struct agc::ActiveState
>>>> + * \brief Active state for AgcAlgorithm
>>>> + *
>>>> + * The \a automatic variables track the latest values computed by algorithm
>>>> + * based on the latest processed statistics. All other variables track the
>>>> + * consolidated controls requested in queued requests.
>>>> + *
>>>> + * \var agc::ActiveState::manual
>>>> + * \brief Manual exposure time and analog gain (set through requests)
>>>> + *
>>>> + * \var agc::ActiveState::manual.exposure
>>>> + * \brief Manual exposure time expressed as a number of lines as set by the
>>>> + * ExposureTime control
>>>> + *
>>>> + * \var agc::ActiveState::manual.gain
>>>> + * \brief Manual analogue gain as set by the AnalogueGain control
>>>> + *
>>>> + * \var agc::ActiveState::automatic
>>>> + * \brief Automatic exposure time and analog gain (computed by the algorithm)
>>>> + *
>>>> + * \var agc::ActiveState::automatic.exposure
>>>> + * \brief Automatic exposure time expressed as a number of lines
>>>> + *
>>>> + * \var agc::ActiveState::automatic.gain
>>>> + * \brief Automatic analogue gain multiplier
>>>> + *
>>>> + * \var agc::ActiveState::automatic.quantizationGain
>>>> + * \brief Automatic quantization gain multiplier
>>>> + *
>>>> + * \var agc::ActiveState::automatic.yTarget
>>>> + * \brief Automatically determined luminance target
>>>> + *
>>>> + * \var agc::ActiveState::autoExposureEnabled
>>>> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
>>>
>>> s/AGC state (exposure)/exposure mode
>>>
>>>> + *
>>>> + * \var agc::ActiveState::autoGainEnabled
>>>> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
>>>
>>> same here ?
>>
>> These were copied verbatim.
>>
> 
> I don't think we should stick too strictly to what's in rkisp1
> especially when it's about documentation which can so easily be
> improved

Current version:

   * \brief Whether automatic exposure control is enabled by the ExposureTimeMode control


> 
>>
>>>
>>>> + *
>>>> + * \var agc::ActiveState::exposureValue
>>>> + * \brief Exposure value as set by the ExposureValue control
>>>> + *
>>>> + * \var agc::ActiveState::constraintMode
>>>> + * \brief Constraint mode as set by the AeConstraintMode control
>>>> + *
>>>> + * \var agc::ActiveState::exposureMode
>>>> + * \brief Exposure mode as set by the AeExposureMode control
>>>> + *
>>>> + * \var agc::ActiveState::minFrameDuration
>>>> + * \brief Minimum frame duration as set by the FrameDurationLimits control
>>>> + *
>>>> + * \var agc::ActiveState::maxFrameDuration
>>>> + * \brief Maximum frame duration as set by the FrameDurationLimits control
>>>> + */
>>>> +
>>>> +/**
>>>> + * \struct agc::FrameContext
>>>> + * \brief Per-frame context for AgcAlgorithm
>>>> + *
>>>> + * \var agc::FrameContext::exposure
>>>> + * \brief Exposure time expressed as a number of lines computed by the algorithm
>>>> + *
>>>> + * \var agc::FrameContext::gain
>>>> + * \brief Analogue gain multiplier computed by the algorithm
>>>
>>> Are these computed by the algorithms or can these be the manually
>>> programmed values when running in manual mode ?
>>
>> This is always the gain that needs to be applied to the frame, so
>> the manual gain in manual mode, the automatic gain in automatic mode.
>>
> 
> So maybe "computed by the algorithm" isn't exactly correct ?

I think is a bit of a philosophical question again, for the user,
the important thing to know is from where to retrieve the final
exposure and gain to apply to the frame. Even if manual control
is in effect, these values are in a sense calculated.

I think "computed by the algorithm" is fine, but maybe "to apply to the frame"
would be clearer?


> 
> 
>   * \brief Analogue gain multiplier applied to the frame
> 
> ?
> 
>>
>>>
>>> I would replace "computed by the algorithm" with "for the frame" or
>>> similar
>>>
>>>> + *
>>>> + * The gain should be adapted to the sensor specific gain code before applying.
>>>
>>> s/adapted/translated ?
>>
>> These were also all copied verbatim, but I'll adjust it.
>>
>>
>>>
>>>> + *
>>>> + * \var agc::FrameContext::quantizationGain
>>>> + * \brief Quantization gain multiplier computed by the algorithm
>>>> + *
>>>> + * \var agc::FrameContext::exposureValue
>>>> + * \brief Exposure value as set by the ExposureValue control
>>>> + *
>>>> + * \var agc::FrameContext::yTarget
>>>> + * \brief Luminance target computed by the algorithm
>>>> + *
>>>> + * \var agc::FrameContext::vblank
>>>> + * \brief Vertical blanking parameter computed by the algorithm
>>>> + *
>>>> + * \var agc::FrameContext::autoExposureEnabled
>>>> + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
>>>> + *
>>>> + * \var agc::FrameContext::autoGainEnabled
>>>> + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
>>>> + *
>>>> + * \var agc::FrameContext::constraintMode
>>>> + * \brief Constraint mode as set by the AeConstraintMode control
>>>> + *
>>>> + * \var agc::FrameContext::exposureMode
>>>> + * \brief Exposure mode as set by the AeExposureMode control
>>>> + *
>>>> + * \var agc::FrameContext::minFrameDuration
>>>> + * \brief Minimum frame duration as set by the FrameDurationLimits control
>>>> + *
>>>> + * \var agc::FrameContext::maxFrameDuration
>>>> + * \brief Maximum frame duration as set by the FrameDurationLimits control
>>>> + *
>>>> + * \var agc::FrameContext::frameDuration
>>>> + * \brief The actual FrameDuration used by the algorithm for the frame
>>>> + *
>>>> + * \var agc::FrameContext::autoExposureModeChange
>>>> + * \brief Indicate if autoExposureEnabled has changed from true in the previous
>>>> + * frame to false in the current frame, and no manual exposure value has been
>>>> + * supplied in the current frame.
>>>
>>> does this only track auto->manual transaction or does it track the
>>> other way around.
>>>
>>> Kind of a long brief (and no '.' at the end).
>>
>> These were also copied verbatim, but I have removed the dots.
>>
>>
>>>
>>> I would
>>>
>>>        \brief Exposure mode change flag
>>>
>>>        Indicate if the exposure mode has changed compared to the
>>>        previous frame.
>>
>> It only tracks auto -> manual transitions without manual `ExposureTime`,
>> so I think the above wouldn't be entirely correct.
>>
> 
> Yeah, you're right
> 
>          if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
>                  frameContext.autoExposureModeChange = true;
> 
> The purpose of the flag is to keep track if the auto value should be
> propagated to manual, so it's a bit of an unfortunate name. However,
> not something that should be changed in this patch
> 
>>
>>>
>>>> + *
>>>> + * \var agc::FrameContext::autoGainModeChange
>>>> + * \brief Indicate if autoGainEnabled has changed from true in the previous
>>>> + * frame to false in the current frame, and no manual gain value has been
>>>> + * supplied in the current frame.
>>>> + */
>>>> +
>>>> +/**
>>>> + * \struct AgcAlgorithm::ConfigurationParams
>>>> + * \brief Parameters for AgcAlgorithm::configure()
>>>> + *
>>>> + * \var AgcAlgorithm::ConfigurationParams::sensor
>>>> + * \brief CameraSensorHelper for the sensor
>>>> + *
>>>> + * \var AgcAlgorithm::ConfigurationParams::sensorInfo
>>>> + * \brief Details of the sensor
>>>
>>> The sensor configuration description
>>
>> I have adjusted it to
>>
>>    Current configuration of the sensor
>>
>> hope it is also acceptable.
>>
> 
> It is indeed
> 
>>
>>>
>>>> + *
>>>> + * \var AgcAlgorithm::ConfigurationParams::sensorControls
>>>> + * \brief ControlInfoMap of the sensor
>>>> + *
>>>> + * \var AgcAlgorithm::ConfigurationParams::ctrlMap
>>>> + * \brief ControlMap to update with controls
>>>> + *
>>>> + * \var AgcAlgorithm::ConfigurationParams::autoAllowed
>>>> + * \brief Whether to enable auto controls
>>>> + */
>>>> +
>>>> +/**
>>>> + * \struct AgcAlgorithm::ProcessParams
>>>> + * \brief Parameters for AgcAlgorithm::process()
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::traits
>>>> + * \brief Implementation of AgcMeanLuminance::Traits
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::yHist
>>>> + * \brief Luminance histogram of the frame
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::exposure
>>>> + * \brief Effective exposure of the frame
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::gain
>>>> + * \brief Effective gain of the frame
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::additionalConstraints
>>>> + * \brief Additional AgcMeanLuminance::AgcConstraints to apply
>>>> + *
>>>> + * \var AgcAlgorithm::ProcessParams::lux
>>>> + * \brief Effective lux value of the frame
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Load tuning data
>>>> + */
>>>> +int AgcAlgorithm::init(const ValueNode &tuningData)
>>>> +{
>>>> +	int ret = impl_.parseTuningData(tuningData);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Initialize the session configuration and active state
>>>> + */
>>>> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)
>>>
>>> Easy to shorten to
>>>
>>> int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>> 			    const ConfigurationParams &config)
>>
>> Done.
>>
>>
>>>
>>>
>>> Seems like session is an out parameter.
>>>
>>> We used to have a rule: const & for input params, * for output ones.
>>> Not sure anymore how much we enforce it these days..
>>
>> I'm not sure I would consider it an out param in the traditional sense
>> (i.e. the caller is not really expected to use/inspect the result), and
>> I really prefer the consistency of always passing by reference to every
>> function of `AgcAlgorithm`.
>>
> 
> Ok, as said, I don't exactly know how much that rule is even enforced
> nowadays
> 
>>
>>>
>>>> +{
>>>> +	session = {};
>>>> +	session.lineDuration = config.sensorInfo.minLineLength * 1.0s
>>>> +		/ config.sensorInfo.pixelRate;
>>>> +	session.sensor.outputSize = config.sensorInfo.outputSize;
>>>> +	session.autoAllowed = config.autoAllowed;
>>>> +
>>>> +	const double lineDurationUs = session.lineDuration.get<std::micro>();
>>>> +
>>>> +	/*
>>>> +	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
>>>> +	 * limits and the line duration.
>>>> +	 */
>>>> +
>>>> +	const ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>>> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
>>>> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
>>>> +	int32_t defExposure = v4l2Exposure.def().get<int32_t>();
>>>> +	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
>>>> +		static_cast<int32_t>(minExposure * lineDurationUs),
>>>> +		static_cast<int32_t>(maxExposure * lineDurationUs),
>>>> +		static_cast<int32_t>(defExposure * lineDurationUs),
>>>> +	};
>>>> +
>>>> +	/* Compute the analogue gain limits. */
>>>> +	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>());
>>>> +	config.ctrlMap[&controls::AnalogueGain] = ControlInfo{
>>>> +		minGain,
>>>> +		maxGain,
>>>> +		defGain,
>>>> +	};
>>>> +
>>>> +	LOG(Agc, Debug)
>>>> +		<< "Exposure: [" << minExposure << ", " << maxExposure
>>>> +		<< "], gain: [" << minGain << ", " << maxGain << "]";
>>>> +
>>>> +	/*
>>>> +	 * Compute the frame duration limits.
>>>> +	 *
>>>> +	 * The frame length is computed assuming a fixed line length combined
>>>> +	 * with the vertical frame sizes.
>>>> +	 */
>>>> +	const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
>>>> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
>>>> +	uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
>>>> +
>>>> +	const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
>>>> +	std::array<uint32_t, 3> frameHeights{
>>>> +		v4l2VBlank.min().get<int32_t>() + config.sensorInfo.outputSize.height,
>>>> +		v4l2VBlank.max().get<int32_t>() + config.sensorInfo.outputSize.height,
>>>> +		v4l2VBlank.def().get<int32_t>() + config.sensorInfo.outputSize.height,
>>>> +	};
>>>> +
>>>> +	std::array<int64_t, 3> frameDurations;
>>>> +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>>>> +		uint64_t frameSize = lineLength * frameHeights[i];
>>>> +		frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);
>>>> +	}
>>>> +
>>>> +	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
>>>> +		frameDurations[0],
>>>> +		frameDurations[1],
>>>> +		Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } },
>>>> +	};
>>>> +
>>>> +	session.minFrameDuration = std::chrono::microseconds(frameDurations[0]);
>>>> +	session.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);
>>>> +
>>>> +	/*
>>>> +	 * When the AGC computes the new exposure values for a frame, it needs
>>>> +	 * to know the limits for exposure time and analogue gain. As it depends
>>>> +	 * on the sensor, update it with the controls.
>>>> +	 *
>>>> +	 * \todo take VBLANK into account for maximum exposure time
>>>> +	 */
>>>> +	session.minExposureTime = minExposure * session.lineDuration;
>>>> +	session.maxExposureTime = maxExposure * session.lineDuration;
>>>> +	session.minAnalogueGain = minGain;
>>>> +	session.maxAnalogueGain = maxGain;
>>>
>>> The above matches the current RkISP1Agc::reconfigure() content
>>>
>>>> +
>>>> +	impl_.configure(session.lineDuration, config.sensor);
>>>> +	impl_.setLimits(session.minExposureTime, session.maxExposureTime,
>>>> +			session.minAnalogueGain, session.maxAnalogueGain,
>>>> +			{});
>>>> +	impl_.resetFrameCount();
>>>> +
>>>> +	/* Configure the default exposure and gain. */
>>>> +	state = {};
>>>> +	state.automatic.gain = session.minAnalogueGain;
>>>> +	state.automatic.exposure = 10ms / session.lineDuration;
>>>> +	state.automatic.quantizationGain = 1;
>>>> +	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
>>>
>>> And this matches RkISP1::Agc() apart from this early yTarget
>>> computation which I presume is intentional
>>
>> Hmmm... that should match as well. There is
>>
>>    context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);
>>
>> at the end of `rkisp1.cpp:Agc::configure()`.
> 
> Ah yes, it's at the end of the function indeed
> 
>>
>>
>>>
>>>> +	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;
>>>> +
>>>> +	const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {
>>>> +		std::array<ControlValue, 2> values;
>>>> +		size_t count = 0;
>>>> +
>>>> +		if (session.autoAllowed)
>>>> +			values[count++] = ControlValue(automatic);
>>>> +
>>>> +		values[count++] = ControlValue(manual);
>>>> +
>>>> +		config.ctrlMap[&cid] = ControlInfo{
>>>> +			{ values.data(), count },
>>>> +			ControlValue(session.autoAllowed ? automatic : manual),
>>>> +		};
>>>> +	};
>>>> +
>>>> +	add(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
>>>> +	add(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>>>
>>> Could you please break trivially re-adjustable long lines ?
>>
>> Adjusted to
>>
>>    add(controls::ExposureTimeMode,
>>        controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);
>>    add(controls::AnalogueGainMode,
>>        controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);
>>
>> Does that look better?
>>
> 
> indeed
> 
>>
>>>
>>>> +
>>>> +	/* \todo Move this to the `Camera` class. */
>>>> +	config.ctrlMap[&controls::AeEnable] = ControlInfo{
>>>> +		false,
>>>> +		session.autoAllowed,
>>>> +		session.autoAllowed,
>>>> +	};
>>>
>>> RkISP1Agc register controls in init() but I think what you're doing
>>> here it's better, as some control such as ExposureTimeMode depend on
>>> the session configuration.
>>>
>>> Is there any risk of having leftovers from a previous session in the
>>> control list ?
>>
>> Every control should either be overwritten or removed.
>>
> 
> Is that because the Camera::controls info map gets reset between
> configure() calls ? I don't really see that happening in Camera or
> PipelineHandler.

As far as I can tell, currently no IPA module clears its `ControlInfoMap::Map`
because most controls are added in each algorithm's `init()`. I think this is
again a policy question. But adjusting this might be inconvenient because I think
it is expected to have registered controls even before the first configuration,
so in effect controls will have to be added in both `init()` and `configure()`
by each algorithm. Or alternatively each IPA module should call `configure()`
right after `init()`.


> 
> Same as per the data->ipaControls_ passed by the RkISP1 pipeline handle
> to the IPA and used to populate Camera::controls().
> 
>>
>>>
>>>> +
>>>> +	if (session.autoAllowed) {
>>>> +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>>>> +
>>>> +		for (const auto &[id, info] : impl_.controls())
>>>> +			config.ctrlMap[id] = info;
>>>> +	} else {
>>>> +		config.ctrlMap.erase(&controls::ExposureValue);
>>>> +
>>>> +		for (const auto &[id, info] : impl_.controls())
>>>> +			config.ctrlMap.erase(id);
>>>> +	}
>>>
>>> Maybe it happens later on, but this is a bit cumbersome. Making
>>> AgcMeanLuminance aware of autoAllowed would avoid this
>>
>> Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.
>> And in any case, there will be a loop like this or equivalent, and
> 
> Ok, I thought it was AgcMeanLuminance registering ExposureValue but it
> actually is this class doing so it here. What we're erasing are
> possible leftovers from a previous session maybe ?
> 
>> I'm not sure it matters too much where it is.
> 
> I was bothered by the need to erase, as it seems like a lot of
> bookeeping that could possibile go wrong.
> 
> But if I get this right this is about removing controls from a
> previous session ? If that's the case should we pass a fresh list of
> controls at every ::configure() call to avoid leftovers instead of
> erasing them ?

See above.


> 
>>
>>
>>>
>>>> +
>>>> +
>>>
>>> Double empty line
>>
>> Fixed.
>>
>>
>>>
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Handle a \a queueRequest operation
>>>> + */
>>>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
>>>> +				agc::FrameContext &frameContext, const ControlList &controls)
>>>> +{
>>>> +	if (session.autoAllowed) {
>>>
>>> Now that you register controls at configure() time and not init()
>>> time, am I correct that if !autoAllowed applications won't be able to select
>>> controls::ExposureTimeModeAuto ?
>>
>> Yes.
>>
>>
>>>
>>> Which makes me wonder if controls::ExposureTimeMode should be
>>> registered at all if !autoAllowed
>>
>> I think that's more of a philosophical question. My preference is to
>> register controls even if they only really have one valid value. Maybe
>> this needs more discussion to arrive at a libcamera-wide policy.
> 
> Ack, let's ask others
> 
>>
>>
>>>
>>>> +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
>>>> +		if (aeEnable &&
>>>> +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {
>>>> +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
>>>> +
>>>> +			LOG(Agc, Debug)
>>>> +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
>>>> +				<< " AGC (exposure)";
>>>> +
>>>> +			/*
>>>> +			 * If we go from auto -> manual with no manual control
>>>> +			 * set, use the last computed value, which we don't
>>>> +			 * know until prepare() so save this information.
>>>> +			 *
>>>> +			 * \todo Check the previous frame at prepare() time
>>>> +			 * instead of saving a flag here
>>>> +			 */
>>>> +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
>>>> +				frameContext.autoExposureModeChange = true;
>>>> +		}
>>>> +
>>>> +		const auto &agEnable = controls.get(controls::AnalogueGainMode);
>>>
>>> Same reasoning for AnalogueGainMode
>>>
>>>> +		if (agEnable &&
>>>> +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
>>>> +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
>>>> +
>>>> +			LOG(Agc, Debug)
>>>> +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
>>>> +				<< " AGC (gain)";
>>>> +			/*
>>>> +			 * If we go from auto -> manual with no manual control
>>>> +			 * set, use the last computed value, which we don't
>>>> +			 * know until prepare() so save this information.
>>>> +			 */
>>>> +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
>>>> +				frameContext.autoGainModeChange = true;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	const auto &exposure = controls.get(controls::ExposureTime);
>>>> +	if (exposure && !state.autoExposureEnabled) {
>>>> +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
>>>> +
>>>> +		LOG(Agc, Debug)
>>>> +			<< "Set exposure to " << state.manual.exposure;
>>>> +	}
>>>> +
>>>> +	const auto &gain = controls.get(controls::AnalogueGain);
>>>> +	if (gain && !state.autoGainEnabled) {
>>>> +		state.manual.gain = *gain;
>>>> +
>>>> +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
>>>> +	}
>>>> +
>>>> +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
>>>> +	frameContext.autoGainEnabled = state.autoGainEnabled;
>>>> +
>>>> +	if (!frameContext.autoExposureEnabled)
>>>> +		frameContext.exposure = state.manual.exposure;
>>>> +	if (!frameContext.autoGainEnabled)
>>>> +		frameContext.gain = state.manual.gain;
>>>> +
>>>> +	if (!frameContext.autoExposureEnabled &&
>>>> +	    !frameContext.autoGainEnabled)
>>>> +		frameContext.quantizationGain = 1.0;
>>>> +
>>>> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
>>>> +	if (exposureMode)
>>>> +		state.exposureMode =
>>>> +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
>>>> +	frameContext.exposureMode = state.exposureMode;
>>>> +
>>>> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
>>>> +	if (constraintMode)
>>>> +		state.constraintMode =
>>>> +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
>>>> +	frameContext.constraintMode = state.constraintMode;
>>>> +
>>>> +	const auto &exposureValue = controls.get(controls::ExposureValue);
>>>> +	if (exposureValue)
>>>> +		state.exposureValue = *exposureValue;
>>>> +	frameContext.exposureValue = state.exposureValue;
>>>> +
>>>> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>>>> +	if (frameDurationLimits) {
>>>> +		/* Limit the control value to the limits in ControlInfo */
>>>> +		state.minFrameDuration = std::clamp<utils::Duration>(
>>>> +			std::chrono::microseconds((*frameDurationLimits).front()),
>>>> +			session.minFrameDuration,
>>>> +			session.maxFrameDuration
>>>> +		);
>>>> +
>>>> +		state.maxFrameDuration = std::clamp<utils::Duration>(
>>>> +			std::chrono::microseconds((*frameDurationLimits).back()),
>>>> +			session.minFrameDuration,
>>>> +			session.maxFrameDuration
>>>> +		);
>>>> +	}
>>>> +	frameContext.minFrameDuration = state.minFrameDuration;
>>>> +	frameContext.maxFrameDuration = state.maxFrameDuration;
>>>> +}
>>>> +
>>>
>>> This matches RkISP1Agc::queueRequest() (metering mode handling apart)
>>>
>>>> +/**
>>>> + * \brief Handle a \a prepare operation
>>>> + */
>>>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
>>>> +{
>>>> +	uint32_t activeAutoExposure = state.automatic.exposure;
>>>> +	double activeAutoGain = state.automatic.gain;
>>>> +	double activeAutoQGain = state.automatic.quantizationGain;
>>>> +
>>>> +	/* Populate exposure and gain in auto mode */
>>>> +	if (frameContext.autoExposureEnabled) {
>>>> +		frameContext.exposure = activeAutoExposure;
>>>> +		frameContext.quantizationGain = activeAutoQGain;
>>>> +	}
>>>> +	if (frameContext.autoGainEnabled) {
>>>> +		frameContext.gain = activeAutoGain;
>>>> +		frameContext.quantizationGain = activeAutoQGain;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Populate manual exposure and gain from the active auto values when
>>>> +	 * transitioning from auto to manual
>>>> +	 */
>>>
>>> I guess this answers to my question on the auto->manual transition
>>> tracking for the frameContext.autoExposureModeChange flag.
>>>
>>> It tracks both state changes, but we use it to propagate auto values
>>> to the manual state if they have not been overriden by an application
>>> provide control
>>
>> As mentioned above, I think it very specifically only tracks those auto->manual
>> transition that don't have an associated manual `ExposureTime`.
>>
> 
> Yes you're right
> 
>>
>>>
>>>> +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
>>>> +		state.manual.exposure = activeAutoExposure;
>>>> +		frameContext.exposure = activeAutoExposure;
>>>> +	}
>>>> +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
>>>> +		state.manual.gain = activeAutoGain;
>>>> +		frameContext.gain = activeAutoGain;
>>>> +		frameContext.quantizationGain = activeAutoQGain;
>>>> +	}
>>>> +
>>>> +	frameContext.yTarget = state.automatic.yTarget;
>>>> +}
>>>
>>> Ack, this matches RkISP1Agc::prepare()
>>>
>>>> +
>>>> +/**
>>>> + * \brief Handle a \a process operation
>>>> + */
>>>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>>> +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
>>>> +			   ControlList &metadata)
>>>> +{
>>>> +	const utils::Duration &lineDuration = session.lineDuration;
>>>> +	utils::Duration newExposureTime = {};
>>>> +
>>>> +	if (params) {
>>>
>>> I presume (well, you told me) that this matches the
>>>
>>>           if (!stats)
>>>
>>> case in RkISP1.
>>>
>>> I wonder, should
>>>
>>> The RkISP1 IPA does
>>>
>>> 	if (!stats) {
>>> 		processFrameDuration(context, frameContext,
>>> 				     frameContext.agc.minFrameDuration);
>>> 		fillMetadata(context, frameContext, metadata);
>>> 		return;
>>> 	}
>>>
>>> I guess it's fine for IPA to call AgcAlgorithm::process() even when
>>> !stats, as it is simpler for them to make the call unconditionally, so
>>> I'm not against this.
>>>
>>> But maybe we can do like RkISP1 does (split processFrameDuration() and
>>> fillMetadata() to helpers) to save some indentation here.
>>
>> My preference would be not to add back those separate functions. Maybe
>> moving the `if (params) { ... }` block into a separate function if
>> that is acceptable.
> 
> I guess we're discussing tastes, but I usually get suspicious when
> code gets to 4 indentation levels and we could easily avoid that
> 
>>
>>
>>>
>>>> +		ASSERT(session.autoAllowed);
>>>
>>> Does it mean IPA should not call process if (autoAllowed) ?
>>
>> Yes.
>>
> 
> Well, not exactly.
> 
> The IPA shall not provide stats if !autoAllowed

Right, I don't know why I wrote that.


> 
>>
>>>
>>> Can't we simply bail out here to make IPAs simpler ?
>>
>> Possibly, but I don't see how that would make them simpler. The assumption
> 
> I would have liked for IPAs to be able to call ::process() with an
> optionally initialized ProcessParams which in case of !stats
> would be empty. If the core is running with !autoAllowed it will
> ignore the params.
> 
> This doesn't work in case the system is running with autoAllowed but
> for some reasons you don't have valid stats. RkISP1 seems to assume it
> might happen you can get bad/corrupted stats, however in practice I'm
> not sure how it could happen.
> 
> 
> What you have right now (RkISP1 as an example)
> 
> 	if (stats) {
> 		if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> 			params = &stats->params;
> 		else
> 			LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> 	}
> 
> 	if (params) {
> 		std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> 		if (context.activeState.wdr.mode != controls::WdrOff)
> 			additionalConstraints.push_back(context.activeState.wdr.constraint);
> 
> 		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> 			.traits = AgcTraits{
> 				{ params->ae.exp_mean, context.hw.numAeCells },
> 				meteringModes_.at(frameContext.agc.meteringMode),
> 			},
> 			.yHist = {
> 				/* The lower 4 bits are fractional and meant to be discarded. */
> 				{ params->hist.hist_bins, context.hw.numHistogramBins },
> 				[](uint32_t x) { return x >> 4; },
> 			},
> 			.exposure = frameContext.sensor.exposure,
> 			/*
> 			 * Include the quantization gain if it was applied. Do not use
> 			 * compress.gain because it will include gains that shall not be
> 			 * reported to the user when HDR is implemented.
> 			 */
> 			.gain = frameContext.sensor.gain
> 			        * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> 			.additionalConstraints = std::move(additionalConstraints),
> 			.lux = frameContext.lux.lux,
> 		}}, metadata);
> 	} else {
> 		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> 	}
> 
> Requires the IPA to validate stats and have to distinct calls to
> agc_.process()
> 
> while
> 
>          agc::ProcessParams params {};
>          if (stats && stats & ... )
>                  /* populate params. */
>          }
> 
>          agc_.process( ... , ...,  params, ... )

The issue is the "traits" and "yHist" members, they are references,
so the referenced things must stay alive for the duration of the call.
The above snippet you suggested is syntactically possible, but avoiding
use-after-free issues makes it a bit cumbersome.


> 
> is less verbose, but would require the core to bail out in case
> (params.empty() || !autoAllowed)
> 
> This mean you would need a way to distinguish empty param blocks and
> you can't use the declarative style you have here, which I kind of
> like as well..
> 
> not sure, I have a feeling IPA could be made simpler, but at the same
> time this seems more robust
> 
>> is that `autoAllowed == false` means that statistics will not be available
>> (and hence no automatic control) (usually for raw capture - but there is no
>> technical reason why agc could not run even for raw capture if statistics
>> are available), so providing statistics is an api violation in a sense, so
>> I think the assert is reasonable.
>>
> 
> We can start strict and see. After all this is not something
> application could trigger.
> 
>>
>>>
>>>> +
>>>> +		/*
>>>> +		* Set the AGC limits using the fixed exposure time and/or gain in
>>>> +		* manual mode, or the sensor limits in auto mode.
>>>> +		*/
>>>
>>> Weird indent
>>
>> Fixed.
>>
>>>
>>>> +		utils::Duration minExposureTime;
>>>> +		utils::Duration maxExposureTime;
>>>> +		double minAnalogueGain;
>>>> +		double maxAnalogueGain;
>>>> +
>>>> +		if (frameContext.autoExposureEnabled) {
>>>> +			minExposureTime = session.minExposureTime;
>>>> +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
>>>
>>> very long line
>>
>> Adjusted.
>>
>>
>>>
>>>> +		} else {
>>>> +			minExposureTime = lineDuration * frameContext.exposure;
>>>> +			maxExposureTime = minExposureTime;
>>>> +		}
>>>> +
>>>> +		if (frameContext.autoGainEnabled) {
>>>> +			minAnalogueGain = session.minAnalogueGain;
>>>> +			maxAnalogueGain = session.maxAnalogueGain;
>>>> +		} else {
>>>> +			minAnalogueGain = frameContext.gain;
>>>> +			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;
>>>
>>> RkISP1 gets exposure and gain from the frameContext.
>>>
>>> I see in the next patches that port RkISP1 to AgcAlgorithm this hunk
>>>
>>>           agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,
>>>
>>>                           ...
>>>
>>>                          .exposure = frameContext.sensor.exposure,
>>>                          /*
>>>                           * Include the quantization gain if it was applied. Do not use
>>>                           * compress.gain because it will include gains that shall not be
>>>                           * reported to the user when HDR is implemented.
>>>                           */
>>>                          .gain = frameContext.sensor.gain
>>>                                  * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
>>>
>>> Is the reason you can't take exposure and gain from the FrameContext
>>> the gain adjustment ?
>>
>> That, and the fact that it is `frameContext.sensor`, which is separate
>> from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs
>> the exposure/gain that was actually in effect.
>>
> 
> Ack, too bad, the Agc::process() interface is quite verbose already :(
> 
>>
>>>
>>>> +
>>>> +		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),
>>>> +		});
>>>> +
>>>> +		LOG(Agc, Debug)
>>>> +			<< "Divided up exposure time, analogue gain, quantization gain"
>>>> +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
>>>> +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
>>>> +
>>>> +		/* Update the estimated exposure and gain. */
>>>> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
>>>> +		state.automatic.gain = newEv.analogueGain;
>>>> +		state.automatic.quantizationGain = newEv.quantizationGain;
>>>> +		state.automatic.yTarget = newEv.yTarget;
>>>> +
>>>> +		newExposureTime = newEv.exposureTime;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Expand the target frame duration so that we do not run faster than
>>>> +	 * the minimum frame duration when we have short exposures.
>>>> +	 */
>>>> +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
>>>> +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
>>>> +
>>>> +	/* Update frame duration accounting for line length quantization. */
>>>> +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
>>>
>>> Easy to shorten to
>>>
>>> 	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)
>>> 				   * lineDuration;
>>>
>>
>> This was also copied verbatim.
>>
>>
>>>> +
>>>> +	metadata.set(controls::AnalogueGain, frameContext.gain);
>>>> +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
>>>
>>> I'm not sure I understand the strategy here.. this line is very long
>>> and could be easily made
>>>
>>> 	metadata.set(controls::ExposureTime,
>>> 		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
>>>
>>
>> Adjusted.
>>
>>
>>>
>>>> +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
>>>> +	metadata.set(controls::ExposureTimeMode,
>>>> +		     frameContext.autoExposureEnabled
>>>> +		     ? controls::ExposureTimeModeAuto
>>>> +		     : controls::ExposureTimeModeManual);
>>>> +	metadata.set(controls::AnalogueGainMode,
>>>> +		     frameContext.autoGainEnabled
>>>> +		     ? controls::AnalogueGainModeAuto
>>>> +		     : controls::AnalogueGainModeManual);
>>>
>>> These ones instead are very short and could easily be made
>>>
>>> 	metadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?
>>> 		     controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);
>>
>>
>> These were copied verbatim. So please confirm if you want me to adjust the
>> verbatim copies here and in the whole file.
> 
> As said, it's not that we're moving code in a single patch where we
> can compare the lines moved around in the diff.
> 
> The way I did the review was looking at the code in Agc and RkISP1Agc
> and they are of course different already, so I don't think we should
> stick to verbatim copies when there's something trivially improvable
> like spanning to the whole line length
> 
>>
>>
>>>
>>>> +
>>>> +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
>>>> +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
>>>> +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
>>>> +}
>>>> +
>>>> +} /* namespace ipa */
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>>>> index 5247425952..1eace12908 100644
>>>> --- a/src/ipa/libipa/agc.h
>>>> +++ b/src/ipa/libipa/agc.h
>>>> @@ -7,13 +7,19 @@
>>>>
>>>>    #pragma once
>>>>
>>>> +#include <optional>
>>>>    #include <utility>
>>>>
>>>>    #include <linux/v4l2-controls.h>
>>>>
>>>> +#include <libcamera/control_ids.h>
>>>>    #include <libcamera/controls.h>
>>>>
>>>> +#include <libcamera/ipa/core_ipa_interface.h>
>>>> +
>>>> +#include "agc_mean_luminance.h"
>>>>    #include "camera_sensor_helper.h"
>>>> +#include "histogram.h"
>>>>
>>>>    namespace libcamera {
>>>>
>>>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>>>>    	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>>>>    }
>>>>
>>>> +struct Session {
>>>> +	utils::Duration minExposureTime;
>>>> +	utils::Duration maxExposureTime;
>>>> +	double minAnalogueGain;
>>>> +	double maxAnalogueGain;
>>>> +	utils::Duration minFrameDuration;
>>>> +	utils::Duration maxFrameDuration;
>>>> +
>>>> +	utils::Duration lineDuration;
>>>> +
>>>> +	struct {
>>>> +		Size outputSize;
>>>> +	} sensor;
>>>> +
>>>> +	bool autoAllowed;
>>>> +};
>>>> +
>>>> +struct ActiveState {
>>>> +	struct {
>>>> +		uint32_t exposure;
>>>> +		double gain;
>>>> +	} manual;
>>>> +	struct {
>>>> +		uint32_t exposure;
>>>> +		double gain;
>>>> +		double quantizationGain;
>>>> +		double yTarget;
>>>> +	} automatic;
>>>> +
>>>> +	bool autoExposureEnabled;
>>>> +	bool autoGainEnabled;
>>>> +	double exposureValue;
>>>> +	controls::AeConstraintModeEnum constraintMode;
>>>> +	controls::AeExposureModeEnum exposureMode;
>>>> +	utils::Duration minFrameDuration;
>>>> +	utils::Duration maxFrameDuration;
>>>> +};
>>>> +
>>>> +struct FrameContext {
>>>> +	uint32_t exposure;
>>>> +	double gain;
>>>> +	double quantizationGain;
>>>> +	double exposureValue;
>>>> +	double yTarget;
>>>> +	uint32_t vblank;
>>>> +	bool autoExposureEnabled;
>>>> +	bool autoGainEnabled;
>>>> +	controls::AeConstraintModeEnum constraintMode;
>>>> +	controls::AeExposureModeEnum exposureMode;
>>>> +	utils::Duration minFrameDuration;
>>>> +	utils::Duration maxFrameDuration;
>>>> +	utils::Duration frameDuration;
>>>> +	bool autoExposureModeChange;
>>>> +	bool autoGainModeChange;
>>>> +};
>>>> +
>>>>    } /* namespace agc */
>>>>
>>>> +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
>>>
>>> easy to shorten
>>
>> Done.
>>
>>
>>>
>>>> +
>>>> +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
>>>> +			  agc::FrameContext &frameContext, const ControlList &controls);
>>>> +
>>>> +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
>>>> +
>>>> +	struct ProcessParams {
>>>> +		const AgcMeanLuminance::Traits &traits;
>>>> +		const Histogram &yHist;
>>>> +		uint32_t exposure;
>>>> +		double gain;
>>>> +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
>>>> +		double lux = 0;
>>>> +	};
>>>> +
>>>> +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
>>>
>>> same here
>>
>> Done.
>>
>>
>>>
>>>> +		     std::optional<ProcessParams> &&params, ControlList &metadata);
>>>> +
>>>> +private:
>>>> +	AgcMeanLuminance impl_;
>>>> +};
>>>> +
>>>>    } /* namespace ipa */
>>>>
>>>>    } /* namespace libcamera */
>>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>>>> index 05f1a8749c..ca681fa5af 100644
>>>> --- a/src/ipa/libipa/meson.build
>>>> +++ b/src/ipa/libipa/meson.build
>>>> @@ -23,6 +23,7 @@ libipa_headers = files([
>>>>    ])
>>>>
>>>>    libipa_sources = files([
>>>> +    'agc.cpp',
>>>>        'agc_mean_luminance.cpp',
>>>>        'algorithm.cpp',
>>>>        'awb_bayes.cpp',
>>>
>>> Enormous work overall!
>>>
>>> Agc is the most complex beast we have and I'm looking forwards to
>>> getting this merged already!
>>>
>>>
>>>> --
>>>> 2.55.0
>>>>
>>

Hi Barnabás

On Mon, Jul 27, 2026 at 04:06:31PM +0200, Barnabás Pőcze wrote:
> 2026. 07. 27. 15:25 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:

[snip]

> > >
> > > >
> > > > > +
> > > > > +	/* \todo Move this to the `Camera` class. */
> > > > > +	config.ctrlMap[&controls::AeEnable] = ControlInfo{
> > > > > +		false,
> > > > > +		session.autoAllowed,
> > > > > +		session.autoAllowed,
> > > > > +	};
> > > >
> > > > RkISP1Agc register controls in init() but I think what you're doing
> > > > here it's better, as some control such as ExposureTimeMode depend on
> > > > the session configuration.
> > > >
> > > > Is there any risk of having leftovers from a previous session in the
> > > > control list ?
> > >
> > > Every control should either be overwritten or removed.
> > >
> >
> > Is that because the Camera::controls info map gets reset between
> > configure() calls ? I don't really see that happening in Camera or
> > PipelineHandler.
>
> As far as I can tell, currently no IPA module clears its `ControlInfoMap::Map`
> because most controls are added in each algorithm's `init()`. I think this is
> again a policy question. But adjusting this might be inconvenient because I think
> it is expected to have registered controls even before the first configuration,
> so in effect controls will have to be added in both `init()` and `configure()`
> by each algorithm. Or alternatively each IPA module should call `configure()`
> right after `init()`.
>

mm, this might be tricky...

One possibilty, at the expense of efficiency, is to pass to
:configure() a fresh control list from the IPA module, and then copy
it back to the pipeline.

But we need to enforce a global policy for that..

>
> >
> > Same as per the data->ipaControls_ passed by the RkISP1 pipeline handle
> > to the IPA and used to populate Camera::controls().
> >
> > >
> > > >
> > > > > +
> > > > > +	if (session.autoAllowed) {
> > > > > +		config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > > > > +
> > > > > +		for (const auto &[id, info] : impl_.controls())
> > > > > +			config.ctrlMap[id] = info;
> > > > > +	} else {
> > > > > +		config.ctrlMap.erase(&controls::ExposureValue);
> > > > > +
> > > > > +		for (const auto &[id, info] : impl_.controls())
> > > > > +			config.ctrlMap.erase(id);
> > > > > +	}
> > > >
> > > > Maybe it happens later on, but this is a bit cumbersome. Making
> > > > AgcMeanLuminance aware of autoAllowed would avoid this
> > >
> > > Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.
> > > And in any case, there will be a loop like this or equivalent, and
> >
> > Ok, I thought it was AgcMeanLuminance registering ExposureValue but it
> > actually is this class doing so it here. What we're erasing are
> > possible leftovers from a previous session maybe ?
> >
> > > I'm not sure it matters too much where it is.
> >
> > I was bothered by the need to erase, as it seems like a lot of
> > bookeeping that could possibile go wrong.
> >
> > But if I get this right this is about removing controls from a
> > previous session ? If that's the case should we pass a fresh list of
> > controls at every ::configure() call to avoid leftovers instead of
> > erasing them ?
>
> See above.
>

[snip]

> > > > > +		ASSERT(session.autoAllowed);
> > > >
> > > > Does it mean IPA should not call process if (autoAllowed) ?
> > >
> > > Yes.
> > >
> >
> > Well, not exactly.
> >
> > The IPA shall not provide stats if !autoAllowed
>
> Right, I don't know why I wrote that.
>
>
> >
> > >
> > > >
> > > > Can't we simply bail out here to make IPAs simpler ?
> > >
> > > Possibly, but I don't see how that would make them simpler. The assumption
> >
> > I would have liked for IPAs to be able to call ::process() with an
> > optionally initialized ProcessParams which in case of !stats
> > would be empty. If the core is running with !autoAllowed it will
> > ignore the params.
> >
> > This doesn't work in case the system is running with autoAllowed but
> > for some reasons you don't have valid stats. RkISP1 seems to assume it
> > might happen you can get bad/corrupted stats, however in practice I'm
> > not sure how it could happen.
> >
> >
> > What you have right now (RkISP1 as an example)
> >
> > 	if (stats) {
> > 		if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)
> > 			params = &stats->params;
> > 		else
> > 			LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> > 	}
> >
> > 	if (params) {
> > 		std::vector<AgcMeanLuminance::AgcConstraint> additionalConstraints;
> > 		if (context.activeState.wdr.mode != controls::WdrOff)
> > 			additionalConstraints.push_back(context.activeState.wdr.constraint);
> >
> > 		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> > 			.traits = AgcTraits{
> > 				{ params->ae.exp_mean, context.hw.numAeCells },
> > 				meteringModes_.at(frameContext.agc.meteringMode),
> > 			},
> > 			.yHist = {
> > 				/* The lower 4 bits are fractional and meant to be discarded. */
> > 				{ params->hist.hist_bins, context.hw.numHistogramBins },
> > 				[](uint32_t x) { return x >> 4; },
> > 			},
> > 			.exposure = frameContext.sensor.exposure,
> > 			/*
> > 			 * Include the quantization gain if it was applied. Do not use
> > 			 * compress.gain because it will include gains that shall not be
> > 			 * reported to the user when HDR is implemented.
> > 			 */
> > 			.gain = frameContext.sensor.gain
> > 			        * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> > 			.additionalConstraints = std::move(additionalConstraints),
> > 			.lux = frameContext.lux.lux,
> > 		}}, metadata);
> > 	} else {
> > 		agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> > 	}
> >
> > Requires the IPA to validate stats and have to distinct calls to
> > agc_.process()
> >
> > while
> >
> >          agc::ProcessParams params {};
> >          if (stats && stats & ... )
> >                  /* populate params. */
> >          }
> >
> >          agc_.process( ... , ...,  params, ... )
>
> The issue is the "traits" and "yHist" members, they are references,

I tried to compile the above snippet and quickly found that out

> so the referenced things must stay alive for the duration of the call.
> The above snippet you suggested is syntactically possible, but avoiding
> use-after-free issues makes it a bit cumbersome.

Well, we can construct/copy inside ProcessParams..

Right now, you're taking a reference to an object constructed inside
the call location

	.traits = AgcTraits{
		{ params->ae.exp_mean, context.hw.numAeCells },
		meteringModes_.at(frameContext.agc.meteringMode),
	},
	.yHist = {
		/* The lower 4 bits are fractional and meant to be discarded. */
		{ params->hist.hist_bins, context.hw.numHistogramBins },
		[](uint32_t x) { return x >> 4; },
	},

I don't think constructing them in place would be too different.

Anyway, don't bother for now..



>
>
> >
> > is less verbose, but would require the core to bail out in case
> > (params.empty() || !autoAllowed)
> >
> > This mean you would need a way to distinguish empty param blocks and
> > you can't use the declarative style you have here, which I kind of
> > like as well..
> >
> > not sure, I have a feeling IPA could be made simpler, but at the same
> > time this seems more robust
> >
> > > is that `autoAllowed == false` means that statistics will not be available
> > > (and hence no automatic control) (usually for raw capture - but there is no
> > > technical reason why agc could not run even for raw capture if statistics
> > > are available), so providing statistics is an api violation in a sense, so
> > > I think the assert is reasonable.
> > >
> >
> > We can start strict and see. After all this is not something
> > application could trigger.
> >
> > >
> > > >
> > > > > +
> > > > > +		/*
> > > > > +		* Set the AGC limits using the fixed exposure time and/or gain in
> > > > > +		* manual mode, or the sensor limits in auto mode.
> > > > > +		*/
> > > >
> > > > Weird indent
> > >
> > > Fixed.
> > >
> > > >
> > > > > +		utils::Duration minExposureTime;
> > > > > +		utils::Duration maxExposureTime;
> > > > > +		double minAnalogueGain;
> > > > > +		double maxAnalogueGain;
> > > > > +
> > > > > +		if (frameContext.autoExposureEnabled) {
> > > > > +			minExposureTime = session.minExposureTime;
> > > > > +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
> > > >
> > > > very long line
> > >
> > > Adjusted.
> > >
> > >
> > > >
> > > > > +		} else {
> > > > > +			minExposureTime = lineDuration * frameContext.exposure;
> > > > > +			maxExposureTime = minExposureTime;
> > > > > +		}
> > > > > +
> > > > > +		if (frameContext.autoGainEnabled) {
> > > > > +			minAnalogueGain = session.minAnalogueGain;
> > > > > +			maxAnalogueGain = session.maxAnalogueGain;
> > > > > +		} else {
> > > > > +			minAnalogueGain = frameContext.gain;
> > > > > +			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;
> > > >
> > > > RkISP1 gets exposure and gain from the frameContext.
> > > >
> > > > I see in the next patches that port RkISP1 to AgcAlgorithm this hunk
> > > >
> > > >           agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,
> > > >
> > > >                           ...
> > > >
> > > >                          .exposure = frameContext.sensor.exposure,
> > > >                          /*
> > > >                           * Include the quantization gain if it was applied. Do not use
> > > >                           * compress.gain because it will include gains that shall not be
> > > >                           * reported to the user when HDR is implemented.
> > > >                           */
> > > >                          .gain = frameContext.sensor.gain
> > > >                                  * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),
> > > >
> > > > Is the reason you can't take exposure and gain from the FrameContext
> > > > the gain adjustment ?
> > >
> > > That, and the fact that it is `frameContext.sensor`, which is separate
> > > from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs
> > > the exposure/gain that was actually in effect.
> > >
> >
> > Ack, too bad, the Agc::process() interface is quite verbose already :(
> >
> > >
> > > >
> > > > > +
> > > > > +		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),
> > > > > +		});
> > > > > +
> > > > > +		LOG(Agc, Debug)
> > > > > +			<< "Divided up exposure time, analogue gain, quantization gain"
> > > > > +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
> > > > > +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
> > > > > +
> > > > > +		/* Update the estimated exposure and gain. */
> > > > > +		state.automatic.exposure = newEv.exposureTime / lineDuration;
> > > > > +		state.automatic.gain = newEv.analogueGain;
> > > > > +		state.automatic.quantizationGain = newEv.quantizationGain;
> > > > > +		state.automatic.yTarget = newEv.yTarget;
> > > > > +
> > > > > +		newExposureTime = newEv.exposureTime;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Expand the target frame duration so that we do not run faster than
> > > > > +	 * the minimum frame duration when we have short exposures.
> > > > > +	 */
> > > > > +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
> > > > > +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
> > > > > +
> > > > > +	/* Update frame duration accounting for line length quantization. */
> > > > > +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
> > > >
> > > > Easy to shorten to
> > > >
> > > > 	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)
> > > > 				   * lineDuration;
> > > >
> > >
> > > This was also copied verbatim.
> > >
> > >
> > > > > +
> > > > > +	metadata.set(controls::AnalogueGain, frameContext.gain);
> > > > > +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> > > >
> > > > I'm not sure I understand the strategy here.. this line is very long
> > > > and could be easily made
> > > >
> > > > 	metadata.set(controls::ExposureTime,
> > > > 		     utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> > > >
> > >
> > > Adjusted.
> > >
> > >
> > > >
> > > > > +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
> > > > > +	metadata.set(controls::ExposureTimeMode,
> > > > > +		     frameContext.autoExposureEnabled
> > > > > +		     ? controls::ExposureTimeModeAuto
> > > > > +		     : controls::ExposureTimeModeManual);
> > > > > +	metadata.set(controls::AnalogueGainMode,
> > > > > +		     frameContext.autoGainEnabled
> > > > > +		     ? controls::AnalogueGainModeAuto
> > > > > +		     : controls::AnalogueGainModeManual);
> > > >
> > > > These ones instead are very short and could easily be made
> > > >
> > > > 	metadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?
> > > > 		     controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);
> > >
> > >
> > > These were copied verbatim. So please confirm if you want me to adjust the
> > > verbatim copies here and in the whole file.
> >
> > As said, it's not that we're moving code in a single patch where we
> > can compare the lines moved around in the diff.
> >
> > The way I did the review was looking at the code in Agc and RkISP1Agc
> > and they are of course different already, so I don't think we should
> > stick to verbatim copies when there's something trivially improvable
> > like spanning to the whole line length
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
> > > > > +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
> > > > > +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
> > > > > +}
> > > > > +
> > > > > +} /* namespace ipa */
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > > > index 5247425952..1eace12908 100644
> > > > > --- a/src/ipa/libipa/agc.h
> > > > > +++ b/src/ipa/libipa/agc.h
> > > > > @@ -7,13 +7,19 @@
> > > > >
> > > > >    #pragma once
> > > > >
> > > > > +#include <optional>
> > > > >    #include <utility>
> > > > >
> > > > >    #include <linux/v4l2-controls.h>
> > > > >
> > > > > +#include <libcamera/control_ids.h>
> > > > >    #include <libcamera/controls.h>
> > > > >
> > > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > > +
> > > > > +#include "agc_mean_luminance.h"
> > > > >    #include "camera_sensor_helper.h"
> > > > > +#include "histogram.h"
> > > > >
> > > > >    namespace libcamera {
> > > > >
> > > > > @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
> > > > >    	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
> > > > >    }
> > > > >
> > > > > +struct Session {
> > > > > +	utils::Duration minExposureTime;
> > > > > +	utils::Duration maxExposureTime;
> > > > > +	double minAnalogueGain;
> > > > > +	double maxAnalogueGain;
> > > > > +	utils::Duration minFrameDuration;
> > > > > +	utils::Duration maxFrameDuration;
> > > > > +
> > > > > +	utils::Duration lineDuration;
> > > > > +
> > > > > +	struct {
> > > > > +		Size outputSize;
> > > > > +	} sensor;
> > > > > +
> > > > > +	bool autoAllowed;
> > > > > +};
> > > > > +
> > > > > +struct ActiveState {
> > > > > +	struct {
> > > > > +		uint32_t exposure;
> > > > > +		double gain;
> > > > > +	} manual;
> > > > > +	struct {
> > > > > +		uint32_t exposure;
> > > > > +		double gain;
> > > > > +		double quantizationGain;
> > > > > +		double yTarget;
> > > > > +	} automatic;
> > > > > +
> > > > > +	bool autoExposureEnabled;
> > > > > +	bool autoGainEnabled;
> > > > > +	double exposureValue;
> > > > > +	controls::AeConstraintModeEnum constraintMode;
> > > > > +	controls::AeExposureModeEnum exposureMode;
> > > > > +	utils::Duration minFrameDuration;
> > > > > +	utils::Duration maxFrameDuration;
> > > > > +};
> > > > > +
> > > > > +struct FrameContext {
> > > > > +	uint32_t exposure;
> > > > > +	double gain;
> > > > > +	double quantizationGain;
> > > > > +	double exposureValue;
> > > > > +	double yTarget;
> > > > > +	uint32_t vblank;
> > > > > +	bool autoExposureEnabled;
> > > > > +	bool autoGainEnabled;
> > > > > +	controls::AeConstraintModeEnum constraintMode;
> > > > > +	controls::AeExposureModeEnum exposureMode;
> > > > > +	utils::Duration minFrameDuration;
> > > > > +	utils::Duration maxFrameDuration;
> > > > > +	utils::Duration frameDuration;
> > > > > +	bool autoExposureModeChange;
> > > > > +	bool autoGainModeChange;
> > > > > +};
> > > > > +
> > > > >    } /* namespace agc */
> > > > >
> > > > > +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
> > > >
> > > > easy to shorten
> > >
> > > Done.
> > >
> > >
> > > >
> > > > > +
> > > > > +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
> > > > > +			  agc::FrameContext &frameContext, const ControlList &controls);
> > > > > +
> > > > > +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
> > > > > +
> > > > > +	struct ProcessParams {
> > > > > +		const AgcMeanLuminance::Traits &traits;
> > > > > +		const Histogram &yHist;
> > > > > +		uint32_t exposure;
> > > > > +		double gain;
> > > > > +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
> > > > > +		double lux = 0;
> > > > > +	};
> > > > > +
> > > > > +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
> > > >
> > > > same here
> > >
> > > Done.
> > >
> > >
> > > >
> > > > > +		     std::optional<ProcessParams> &&params, ControlList &metadata);
> > > > > +
> > > > > +private:
> > > > > +	AgcMeanLuminance impl_;
> > > > > +};
> > > > > +
> > > > >    } /* namespace ipa */
> > > > >
> > > > >    } /* namespace libcamera */
> > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > > index 05f1a8749c..ca681fa5af 100644
> > > > > --- a/src/ipa/libipa/meson.build
> > > > > +++ b/src/ipa/libipa/meson.build
> > > > > @@ -23,6 +23,7 @@ libipa_headers = files([
> > > > >    ])
> > > > >
> > > > >    libipa_sources = files([
> > > > > +    'agc.cpp',
> > > > >        'agc_mean_luminance.cpp',
> > > > >        'algorithm.cpp',
> > > > >        'awb_bayes.cpp',
> > > >
> > > > Enormous work overall!
> > > >
> > > > Agc is the most complex beast we have and I'm looking forwards to
> > > > getting this merged already!
> > > >
> > > >
> > > > > --
> > > > > 2.55.0
> > > > >
> > >
>

Hi Barnabás
  let me start a fresh reply, on a separate little thing

On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
> based on the rkisp1 `Agc` algorithm, with slight adjustments.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/agc.h       |  98 ++++++
>  src/ipa/libipa/meson.build |   1 +
>  3 files changed, 738 insertions(+)
>  create mode 100644 src/ipa/libipa/agc.cpp
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> new file mode 100644
> index 0000000000..e16a02fdde
> --- /dev/null
> +++ b/src/ipa/libipa/agc.cpp
> @@ -0,0 +1,639 @@

[snip]

> + * \brief Handle a \a queueRequest operation
> + */
> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
> +				agc::FrameContext &frameContext, const ControlList &controls)
> +{
> +	if (session.autoAllowed) {
> +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> +		if (aeEnable &&
> +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {

if !autoAllowed we initialize

        state.autoExposureEnabled = false;

and only register controls::ExposureTimeModeManual as an option for
control::ExposureTimeMode.

The above condition will always result then in

                if (aeEnable && (false != false))

making this a nop.

Do we then need to wrap this code with:

	if (session.autoAllowed) {


        }

at all ?

> +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
> +
> +			LOG(Agc, Debug)
> +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
> +				<< " AGC (exposure)";
> +
> +			/*
> +			 * If we go from auto -> manual with no manual control
> +			 * set, use the last computed value, which we don't
> +			 * know until prepare() so save this information.
> +			 *
> +			 * \todo Check the previous frame at prepare() time
> +			 * instead of saving a flag here
> +			 */
> +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
> +				frameContext.autoExposureModeChange = true;
> +		}
> +
> +		const auto &agEnable = controls.get(controls::AnalogueGainMode);
> +		if (agEnable &&
> +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
> +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> +
> +			LOG(Agc, Debug)
> +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
> +				<< " AGC (gain)";
> +			/*
> +			 * If we go from auto -> manual with no manual control
> +			 * set, use the last computed value, which we don't
> +			 * know until prepare() so save this information.
> +			 */
> +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
> +				frameContext.autoGainModeChange = true;
> +		}
> +	}
> +
> +	const auto &exposure = controls.get(controls::ExposureTime);
> +	if (exposure && !state.autoExposureEnabled) {
> +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
> +
> +		LOG(Agc, Debug)
> +			<< "Set exposure to " << state.manual.exposure;
> +	}
> +
> +	const auto &gain = controls.get(controls::AnalogueGain);
> +	if (gain && !state.autoGainEnabled) {
> +		state.manual.gain = *gain;
> +
> +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
> +	}
> +
> +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
> +	frameContext.autoGainEnabled = state.autoGainEnabled;
> +
> +	if (!frameContext.autoExposureEnabled)
> +		frameContext.exposure = state.manual.exposure;
> +	if (!frameContext.autoGainEnabled)
> +		frameContext.gain = state.manual.gain;
> +
> +	if (!frameContext.autoExposureEnabled &&
> +	    !frameContext.autoGainEnabled)
> +		frameContext.quantizationGain = 1.0;
> +
> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> +	if (exposureMode)
> +		state.exposureMode =
> +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
> +	frameContext.exposureMode = state.exposureMode;
> +
> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> +	if (constraintMode)
> +		state.constraintMode =
> +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> +	frameContext.constraintMode = state.constraintMode;
> +
> +	const auto &exposureValue = controls.get(controls::ExposureValue);
> +	if (exposureValue)
> +		state.exposureValue = *exposureValue;
> +	frameContext.exposureValue = state.exposureValue;
> +
> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> +	if (frameDurationLimits) {
> +		/* Limit the control value to the limits in ControlInfo */
> +		state.minFrameDuration = std::clamp<utils::Duration>(
> +			std::chrono::microseconds((*frameDurationLimits).front()),
> +			session.minFrameDuration,
> +			session.maxFrameDuration
> +		);
> +
> +		state.maxFrameDuration = std::clamp<utils::Duration>(
> +			std::chrono::microseconds((*frameDurationLimits).back()),
> +			session.minFrameDuration,
> +			session.maxFrameDuration
> +		);
> +	}
> +	frameContext.minFrameDuration = state.minFrameDuration;
> +	frameContext.maxFrameDuration = state.maxFrameDuration;
> +}
> +
> +/**
> + * \brief Handle a \a prepare operation
> + */
> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
> +{
> +	uint32_t activeAutoExposure = state.automatic.exposure;
> +	double activeAutoGain = state.automatic.gain;
> +	double activeAutoQGain = state.automatic.quantizationGain;
> +
> +	/* Populate exposure and gain in auto mode */
> +	if (frameContext.autoExposureEnabled) {
> +		frameContext.exposure = activeAutoExposure;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +	if (frameContext.autoGainEnabled) {
> +		frameContext.gain = activeAutoGain;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +
> +	/*
> +	 * Populate manual exposure and gain from the active auto values when
> +	 * transitioning from auto to manual
> +	 */
> +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
> +		state.manual.exposure = activeAutoExposure;
> +		frameContext.exposure = activeAutoExposure;
> +	}
> +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
> +		state.manual.gain = activeAutoGain;
> +		frameContext.gain = activeAutoGain;
> +		frameContext.quantizationGain = activeAutoQGain;
> +	}
> +
> +	frameContext.yTarget = state.automatic.yTarget;
> +}
> +
> +/**
> + * \brief Handle a \a process operation
> + */
> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
> +			   ControlList &metadata)
> +{
> +	const utils::Duration &lineDuration = session.lineDuration;
> +	utils::Duration newExposureTime = {};
> +
> +	if (params) {
> +		ASSERT(session.autoAllowed);
> +
> +		/*
> +		* Set the AGC limits using the fixed exposure time and/or gain in
> +		* manual mode, or the sensor limits in auto mode.
> +		*/
> +		utils::Duration minExposureTime;
> +		utils::Duration maxExposureTime;
> +		double minAnalogueGain;
> +		double maxAnalogueGain;
> +
> +		if (frameContext.autoExposureEnabled) {
> +			minExposureTime = session.minExposureTime;
> +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
> +		} else {
> +			minExposureTime = lineDuration * frameContext.exposure;
> +			maxExposureTime = minExposureTime;
> +		}
> +
> +		if (frameContext.autoGainEnabled) {
> +			minAnalogueGain = session.minAnalogueGain;
> +			maxAnalogueGain = session.maxAnalogueGain;
> +		} else {
> +			minAnalogueGain = frameContext.gain;
> +			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),
> +		});
> +
> +		LOG(Agc, Debug)
> +			<< "Divided up exposure time, analogue gain, quantization gain"
> +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
> +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
> +
> +		/* Update the estimated exposure and gain. */
> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
> +		state.automatic.gain = newEv.analogueGain;
> +		state.automatic.quantizationGain = newEv.quantizationGain;
> +		state.automatic.yTarget = newEv.yTarget;
> +
> +		newExposureTime = newEv.exposureTime;
> +	}
> +
> +	/*
> +	 * Expand the target frame duration so that we do not run faster than
> +	 * the minimum frame duration when we have short exposures.
> +	 */
> +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
> +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
> +
> +	/* Update frame duration accounting for line length quantization. */
> +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
> +
> +	metadata.set(controls::AnalogueGain, frameContext.gain);
> +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
> +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
> +	metadata.set(controls::ExposureTimeMode,
> +		     frameContext.autoExposureEnabled
> +		     ? controls::ExposureTimeModeAuto
> +		     : controls::ExposureTimeModeManual);
> +	metadata.set(controls::AnalogueGainMode,
> +		     frameContext.autoGainEnabled
> +		     ? controls::AnalogueGainModeAuto
> +		     : controls::AnalogueGainModeManual);
> +
> +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
> +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
> +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 5247425952..1eace12908 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -7,13 +7,19 @@
>
>  #pragma once
>
> +#include <optional>
>  #include <utility>
>
>  #include <linux/v4l2-controls.h>
>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "agc_mean_luminance.h"
>  #include "camera_sensor_helper.h"
> +#include "histogram.h"
>
>  namespace libcamera {
>
> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>  	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>  }
>
> +struct Session {
> +	utils::Duration minExposureTime;
> +	utils::Duration maxExposureTime;
> +	double minAnalogueGain;
> +	double maxAnalogueGain;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +
> +	utils::Duration lineDuration;
> +
> +	struct {
> +		Size outputSize;
> +	} sensor;
> +
> +	bool autoAllowed;
> +};
> +
> +struct ActiveState {
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +	} manual;
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +		double quantizationGain;
> +		double yTarget;
> +	} automatic;
> +
> +	bool autoExposureEnabled;
> +	bool autoGainEnabled;
> +	double exposureValue;
> +	controls::AeConstraintModeEnum constraintMode;
> +	controls::AeExposureModeEnum exposureMode;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +};
> +
> +struct FrameContext {
> +	uint32_t exposure;
> +	double gain;
> +	double quantizationGain;
> +	double exposureValue;
> +	double yTarget;
> +	uint32_t vblank;
> +	bool autoExposureEnabled;
> +	bool autoGainEnabled;
> +	controls::AeConstraintModeEnum constraintMode;
> +	controls::AeExposureModeEnum exposureMode;
> +	utils::Duration minFrameDuration;
> +	utils::Duration maxFrameDuration;
> +	utils::Duration frameDuration;
> +	bool autoExposureModeChange;
> +	bool autoGainModeChange;
> +};
> +
>  } /* namespace agc */
>
> +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
> +
> +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
> +			  agc::FrameContext &frameContext, const ControlList &controls);
> +
> +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
> +
> +	struct ProcessParams {
> +		const AgcMeanLuminance::Traits &traits;
> +		const Histogram &yHist;
> +		uint32_t exposure;
> +		double gain;
> +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
> +		double lux = 0;
> +	};
> +
> +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
> +		     std::optional<ProcessParams> &&params, ControlList &metadata);
> +
> +private:
> +	AgcMeanLuminance impl_;
> +};
> +
>  } /* namespace ipa */
>
>  } /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 05f1a8749c..ca681fa5af 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -23,6 +23,7 @@ libipa_headers = files([
>  ])
>
>  libipa_sources = files([
> +    'agc.cpp',
>      'agc_mean_luminance.cpp',
>      'algorithm.cpp',
>      'awb_bayes.cpp',
> --
> 2.55.0
>

2026. 07. 27. 16:29 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
>    let me start a fresh reply, on a separate little thing
> 
> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:
>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`
>> based on the rkisp1 `Agc` algorithm, with slight adjustments.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp     | 639 +++++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/agc.h       |  98 ++++++
>>   src/ipa/libipa/meson.build |   1 +
>>   3 files changed, 738 insertions(+)
>>   create mode 100644 src/ipa/libipa/agc.cpp
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> new file mode 100644
>> index 0000000000..e16a02fdde
>> --- /dev/null
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -0,0 +1,639 @@
> 
> [snip]
> 
>> + * \brief Handle a \a queueRequest operation
>> + */
>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,
>> +				agc::FrameContext &frameContext, const ControlList &controls)
>> +{
>> +	if (session.autoAllowed) {
>> +		const auto &aeEnable = controls.get(controls::ExposureTimeMode);
>> +		if (aeEnable &&
>> +		    (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {
> 
> if !autoAllowed we initialize
> 
>          state.autoExposureEnabled = false;
> 
> and only register controls::ExposureTimeModeManual as an option for
> control::ExposureTimeMode.
> 
> The above condition will always result then in
> 
>                  if (aeEnable && (false != false))
> 
> making this a nop.
> 
> Do we then need to wrap this code with:
> 
> 	if (session.autoAllowed) {
> 
> 
>          }
> 
> at all ?

There is no control value validation, so the question is how to handle invalid
values. If it's "undefined", then the check can be dropped. I think this is also
a policy question that's worth discussing.



> 
>> +			state.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
>> +
>> +			LOG(Agc, Debug)
>> +				<< (state.autoExposureEnabled ? "Enabling" : "Disabling")
>> +				<< " AGC (exposure)";
>> +
>> +			/*
>> +			 * If we go from auto -> manual with no manual control
>> +			 * set, use the last computed value, which we don't
>> +			 * know until prepare() so save this information.
>> +			 *
>> +			 * \todo Check the previous frame at prepare() time
>> +			 * instead of saving a flag here
>> +			 */
>> +			if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))
>> +				frameContext.autoExposureModeChange = true;
>> +		}
>> +
>> +		const auto &agEnable = controls.get(controls::AnalogueGainMode);
>> +		if (agEnable &&
>> +		    (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {
>> +			state.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
>> +
>> +			LOG(Agc, Debug)
>> +				<< (state.autoGainEnabled ? "Enabling" : "Disabling")
>> +				<< " AGC (gain)";
>> +			/*
>> +			 * If we go from auto -> manual with no manual control
>> +			 * set, use the last computed value, which we don't
>> +			 * know until prepare() so save this information.
>> +			 */
>> +			if (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))
>> +				frameContext.autoGainModeChange = true;
>> +		}
>> +	}
>> +
>> +	const auto &exposure = controls.get(controls::ExposureTime);
>> +	if (exposure && !state.autoExposureEnabled) {
>> +		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
>> +
>> +		LOG(Agc, Debug)
>> +			<< "Set exposure to " << state.manual.exposure;
>> +	}
>> +
>> +	const auto &gain = controls.get(controls::AnalogueGain);
>> +	if (gain && !state.autoGainEnabled) {
>> +		state.manual.gain = *gain;
>> +
>> +		LOG(Agc, Debug) << "Set gain to " << state.manual.gain;
>> +	}
>> +
>> +	frameContext.autoExposureEnabled = state.autoExposureEnabled;
>> +	frameContext.autoGainEnabled = state.autoGainEnabled;
>> +
>> +	if (!frameContext.autoExposureEnabled)
>> +		frameContext.exposure = state.manual.exposure;
>> +	if (!frameContext.autoGainEnabled)
>> +		frameContext.gain = state.manual.gain;
>> +
>> +	if (!frameContext.autoExposureEnabled &&
>> +	    !frameContext.autoGainEnabled)
>> +		frameContext.quantizationGain = 1.0;
>> +
>> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
>> +	if (exposureMode)
>> +		state.exposureMode =
>> +			static_cast<controls::AeExposureModeEnum>(*exposureMode);
>> +	frameContext.exposureMode = state.exposureMode;
>> +
>> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
>> +	if (constraintMode)
>> +		state.constraintMode =
>> +			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
>> +	frameContext.constraintMode = state.constraintMode;
>> +
>> +	const auto &exposureValue = controls.get(controls::ExposureValue);
>> +	if (exposureValue)
>> +		state.exposureValue = *exposureValue;
>> +	frameContext.exposureValue = state.exposureValue;
>> +
>> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>> +	if (frameDurationLimits) {
>> +		/* Limit the control value to the limits in ControlInfo */
>> +		state.minFrameDuration = std::clamp<utils::Duration>(
>> +			std::chrono::microseconds((*frameDurationLimits).front()),
>> +			session.minFrameDuration,
>> +			session.maxFrameDuration
>> +		);
>> +
>> +		state.maxFrameDuration = std::clamp<utils::Duration>(
>> +			std::chrono::microseconds((*frameDurationLimits).back()),
>> +			session.minFrameDuration,
>> +			session.maxFrameDuration
>> +		);
>> +	}
>> +	frameContext.minFrameDuration = state.minFrameDuration;
>> +	frameContext.maxFrameDuration = state.maxFrameDuration;
>> +}
>> +
>> +/**
>> + * \brief Handle a \a prepare operation
>> + */
>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)
>> +{
>> +	uint32_t activeAutoExposure = state.automatic.exposure;
>> +	double activeAutoGain = state.automatic.gain;
>> +	double activeAutoQGain = state.automatic.quantizationGain;
>> +
>> +	/* Populate exposure and gain in auto mode */
>> +	if (frameContext.autoExposureEnabled) {
>> +		frameContext.exposure = activeAutoExposure;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +	if (frameContext.autoGainEnabled) {
>> +		frameContext.gain = activeAutoGain;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +
>> +	/*
>> +	 * Populate manual exposure and gain from the active auto values when
>> +	 * transitioning from auto to manual
>> +	 */
>> +	if (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {
>> +		state.manual.exposure = activeAutoExposure;
>> +		frameContext.exposure = activeAutoExposure;
>> +	}
>> +	if (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {
>> +		state.manual.gain = activeAutoGain;
>> +		frameContext.gain = activeAutoGain;
>> +		frameContext.quantizationGain = activeAutoQGain;
>> +	}
>> +
>> +	frameContext.yTarget = state.automatic.yTarget;
>> +}
>> +
>> +/**
>> + * \brief Handle a \a process operation
>> + */
>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>> +			   agc::FrameContext &frameContext, std::optional<ProcessParams> &&params,
>> +			   ControlList &metadata)
>> +{
>> +	const utils::Duration &lineDuration = session.lineDuration;
>> +	utils::Duration newExposureTime = {};
>> +
>> +	if (params) {
>> +		ASSERT(session.autoAllowed);
>> +
>> +		/*
>> +		* Set the AGC limits using the fixed exposure time and/or gain in
>> +		* manual mode, or the sensor limits in auto mode.
>> +		*/
>> +		utils::Duration minExposureTime;
>> +		utils::Duration maxExposureTime;
>> +		double minAnalogueGain;
>> +		double maxAnalogueGain;
>> +
>> +		if (frameContext.autoExposureEnabled) {
>> +			minExposureTime = session.minExposureTime;
>> +			maxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);
>> +		} else {
>> +			minExposureTime = lineDuration * frameContext.exposure;
>> +			maxExposureTime = minExposureTime;
>> +		}
>> +
>> +		if (frameContext.autoGainEnabled) {
>> +			minAnalogueGain = session.minAnalogueGain;
>> +			maxAnalogueGain = session.maxAnalogueGain;
>> +		} else {
>> +			minAnalogueGain = frameContext.gain;
>> +			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),
>> +		});
>> +
>> +		LOG(Agc, Debug)
>> +			<< "Divided up exposure time, analogue gain, quantization gain"
>> +			<< " and digital gain are " << newEv.exposureTime << ", " << newEv.analogueGain
>> +			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
>> +
>> +		/* Update the estimated exposure and gain. */
>> +		state.automatic.exposure = newEv.exposureTime / lineDuration;
>> +		state.automatic.gain = newEv.analogueGain;
>> +		state.automatic.quantizationGain = newEv.quantizationGain;
>> +		state.automatic.yTarget = newEv.yTarget;
>> +
>> +		newExposureTime = newEv.exposureTime;
>> +	}
>> +
>> +	/*
>> +	 * Expand the target frame duration so that we do not run faster than
>> +	 * the minimum frame duration when we have short exposures.
>> +	 */
>> +	const auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);
>> +	frameContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;
>> +
>> +	/* Update frame duration accounting for line length quantization. */
>> +	frameContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;
>> +
>> +	metadata.set(controls::AnalogueGain, frameContext.gain);
>> +	metadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get<std::micro>());
>> +	metadata.set(controls::FrameDuration, frameContext.frameDuration.get<std::micro>());
>> +	metadata.set(controls::ExposureTimeMode,
>> +		     frameContext.autoExposureEnabled
>> +		     ? controls::ExposureTimeModeAuto
>> +		     : controls::ExposureTimeModeManual);
>> +	metadata.set(controls::AnalogueGainMode,
>> +		     frameContext.autoGainEnabled
>> +		     ? controls::AnalogueGainModeAuto
>> +		     : controls::AnalogueGainModeManual);
>> +
>> +	metadata.set(controls::AeExposureMode, frameContext.exposureMode);
>> +	metadata.set(controls::AeConstraintMode, frameContext.constraintMode);
>> +	metadata.set(controls::ExposureValue, frameContext.exposureValue);
>> +}
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> index 5247425952..1eace12908 100644
>> --- a/src/ipa/libipa/agc.h
>> +++ b/src/ipa/libipa/agc.h
>> @@ -7,13 +7,19 @@
>>
>>   #pragma once
>>
>> +#include <optional>
>>   #include <utility>
>>
>>   #include <linux/v4l2-controls.h>
>>
>> +#include <libcamera/control_ids.h>
>>   #include <libcamera/controls.h>
>>
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include "agc_mean_luminance.h"
>>   #include "camera_sensor_helper.h"
>> +#include "histogram.h"
>>
>>   namespace libcamera {
>>
>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>>   	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>>   }
>>
>> +struct Session {
>> +	utils::Duration minExposureTime;
>> +	utils::Duration maxExposureTime;
>> +	double minAnalogueGain;
>> +	double maxAnalogueGain;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +
>> +	utils::Duration lineDuration;
>> +
>> +	struct {
>> +		Size outputSize;
>> +	} sensor;
>> +
>> +	bool autoAllowed;
>> +};
>> +
>> +struct ActiveState {
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +	} manual;
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +		double quantizationGain;
>> +		double yTarget;
>> +	} automatic;
>> +
>> +	bool autoExposureEnabled;
>> +	bool autoGainEnabled;
>> +	double exposureValue;
>> +	controls::AeConstraintModeEnum constraintMode;
>> +	controls::AeExposureModeEnum exposureMode;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +};
>> +
>> +struct FrameContext {
>> +	uint32_t exposure;
>> +	double gain;
>> +	double quantizationGain;
>> +	double exposureValue;
>> +	double yTarget;
>> +	uint32_t vblank;
>> +	bool autoExposureEnabled;
>> +	bool autoGainEnabled;
>> +	controls::AeConstraintModeEnum constraintMode;
>> +	controls::AeExposureModeEnum exposureMode;
>> +	utils::Duration minFrameDuration;
>> +	utils::Duration maxFrameDuration;
>> +	utils::Duration frameDuration;
>> +	bool autoExposureModeChange;
>> +	bool autoGainModeChange;
>> +};
>> +
>>   } /* namespace agc */
>>
>> +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 configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);
>> +
>> +	void queueRequest(const agc::Session &session, agc::ActiveState &state,
>> +			  agc::FrameContext &frameContext, const ControlList &controls);
>> +
>> +	void prepare(agc::ActiveState &state, agc::FrameContext &frameContext);
>> +
>> +	struct ProcessParams {
>> +		const AgcMeanLuminance::Traits &traits;
>> +		const Histogram &yHist;
>> +		uint32_t exposure;
>> +		double gain;
>> +		std::vector<AgcMeanLuminance::AgcConstraint> &&additionalConstraints = {};
>> +		double lux = 0;
>> +	};
>> +
>> +	void process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,
>> +		     std::optional<ProcessParams> &&params, ControlList &metadata);
>> +
>> +private:
>> +	AgcMeanLuminance impl_;
>> +};
>> +
>>   } /* namespace ipa */
>>
>>   } /* namespace libcamera */
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index 05f1a8749c..ca681fa5af 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -23,6 +23,7 @@ libipa_headers = files([
>>   ])
>>
>>   libipa_sources = files([
>> +    'agc.cpp',
>>       'agc_mean_luminance.cpp',
>>       'algorithm.cpp',
>>       'awb_bayes.cpp',
>> --
>> 2.55.0
>>