[RFC,v1,8/8] ipa: libipa: agc: Take exposure margin into account

28119 diff mbox series

The sensor exposure time depends on the vertical blanking amount and the
exposure margin. Instead of relying on the maximum exposure available at
onfiguration, use the frame duration limits (~ vblank) and exposure
margin to dynamically calculate the max available exposure time.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
 src/ipa/libipa/agc.h   |  3 +-
 2 files changed, 73 insertions(+), 23 deletions(-)

Hi Barnabás

On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> The sensor exposure time depends on the vertical blanking amount and the
> exposure margin. Instead of relying on the maximum exposure available at
> onfiguration, use the frame duration limits (~ vblank) and exposure
> margin to dynamically calculate the max available exposure time.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
>  src/ipa/libipa/agc.h   |  3 +-
>  2 files changed, 73 insertions(+), 23 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 6783055a5e..6ef1f938bd 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -70,12 +70,12 @@ namespace agc {
>   * \struct Session
>   * \brief Session configuration for AgcAlgorithm
>   *
> + * \var Session::minExposure
> + * \brief Minimum exposure (in lines) for the streaming session
> + *

Is this used for clamping only, right ?

We have the same information as a Duration

>   * \var Session::minExposureTime
>   * \brief Minimum exposure time for the streaming session

here

It would be trivial to convert back and forth from lines to Durations,
isn't it ? Do we need to store both values ?

>   *
> - * \var Session::maxExposureTime
> - * \brief Maximum exposure time for the streaming session
> - *
>   * \var Session::minAnalogueGain
>   * \brief Minimum analogue gain for the streaming session
>   *
> @@ -100,6 +100,11 @@ namespace agc {
>   * \var Session::sensor.outputSize
>   * \brief Configured output size of the sensor
>   *
> + * \var Session::sensor.exposureMargin
> + * \brief Exposure margin of the sensor
> + *
> + * \sa CameraSensorHelper::exposureMargin()
> + *
>   * \var Session::autoAllowed
>   * \copybrief AgcAlgorithm::ConfigurationParams::autoAllowed
>   * \sa AgcAlgorithm::ConfigurationParams::autoAllowed
> @@ -221,6 +226,31 @@ namespace agc {
>
>  } /* namespace agc */
>
> +namespace {
> +
> +[[nodiscard]]
> +uint32_t clampExposure(const agc::Session &session, const agc::ActiveState &state,
> +		       uint32_t exposure)
> +{
> +	return std::max(
> +		std::min<uint32_t>(
> +			exposure,
> +			(state.maxFrameDuration / session.lineDuration) - session.sensor.exposureMargin

.. it's trivial to shorten this line

> +		),
> +		session.minExposure
> +	);
> +}
> +
> +[[nodiscard]]
> +uint32_t clampExposure(const agc::Session &session, const agc::ActiveState &state,
> +		       utils::Duration exposureTime)
> +{
> +       return clampExposure(session, state, exposureTime / session.lineDuration);
> +}
> +

isn't it confusing that a function that takes an exposure as
Duration clamps it back in lines ?

Do we want this overload ?

> +} /* namespace */
> +
> +
>  /**
>   * \class AgcAlgorithm
>   * \brief libIPA LSC algorithm algorithm
> @@ -360,6 +390,14 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	session.lineDuration = lineLength * 1.0s / config.sensorInfo.pixelRate;
>  	session.sensor.outputSize = config.sensorInfo.outputSize;
>
> +	auto exposureMargin = sensor_ ? sensor_->exposureMargin() : std::nullopt;
> +	session.sensor.exposureMargin = exposureMargin.value_or(4);
> +	if (!exposureMargin) {
> +		LOG(Agc, Warning)
> +			<< "Sensor exposure margin not available, using "
> +			<< session.sensor.exposureMargin;
> +	}

No need for braces

> +
>  	const double lineDurationUs = session.lineDuration.get<std::micro>();
>
>  	/*
> @@ -369,7 +407,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>
>  	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>();
>
>  	/* Compute the analogue gain limits. */
> @@ -382,12 +419,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	float maxGain = extractGain(v4l2Gain.max());
>  	float defGain = extractGain(v4l2Gain.def());
>
> -	LOG(Agc, Debug)
> -		<< "exposure: [" << minExposure << ',' << maxExposure << "], "
> -		<< "gain: [" << minGain << ',' << maxGain << "], "
> -		<< "line-duration: " << session.lineDuration << ", "
> -		<< "sensor-output: " << session.sensor.outputSize;
> -
>  	/*
>  	 * Compute the frame duration limits.
>  	 *
> @@ -408,17 +439,32 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	 * 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.minExposure = minExposure;
>  	session.minExposureTime = minExposure * session.lineDuration;
> -	session.maxExposureTime = maxExposure * session.lineDuration;
>  	session.minAnalogueGain = minGain;
>  	session.maxAnalogueGain = maxGain;
>  	session.defAnalogueGain = defGain;
>  	session.minFrameDuration = frameHeights.min * session.lineDuration;
>  	session.maxFrameDuration = frameHeights.max * session.lineDuration;
>
> +	const uint32_t maxExposure = frameHeights.max - session.sensor.exposureMargin;
> +	const utils::Duration maxExposureTime = maxExposure * session.lineDuration;
> +
> +	ASSERT(frameHeights.max > session.sensor.exposureMargin);
> +	ASSERT(session.minExposure + session.sensor.exposureMargin <= frameHeights.min);
> +	ASSERT(static_cast<uint32_t>(session.minExposure) < maxExposure);
> +
> +	LOG(Agc, Debug)
> +		<< "exposure: [" << session.minExposure << ',' << maxExposure << "], "
> +		<< "exposure-time: [" << session.minExposureTime << ',' << maxExposureTime << "], "
> +		<< "gain: [" << session.minAnalogueGain << ',' << session.maxAnalogueGain << "], "
> +		<< "line-length: " << lineLength << ", "
> +		<< "line-duration: " << session.lineDuration << ", "
> +		<< "frame-height: [" << frameHeights.min << ',' << frameHeights.max << "], "
> +		<< "sensor-output: " << session.sensor.outputSize << ", "
> +		<< "sensor-exposure-margin: " << session.sensor.exposureMargin;
> +

If you want to change the logging of the session configuration, maybe
a tiny patch to do so is worth it ?

>  	/* Configure the default exposure and gain. */
>  	state = {};
>  	state.automatic.gain = session.minAnalogueGain;
> @@ -431,7 +477,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  	state.autoGainEnabled = session.autoAllowed;
>  	state.exposureValue = 0;
>  	state.minFrameDuration = session.minFrameDuration;
> -	state.maxFrameDuration = session.maxFrameDuration;
> +	state.maxFrameDuration = std::clamp(
> +		utils::Duration(1.0s / 5), /* Try to achieve at least 5 fps by default. */
> +		session.minFrameDuration, session.maxFrameDuration);

Ok, we're here entering policies..

Personally, I think it's reasonable, but now we're creating a
something that affects all libIPA platforms, and I would like to know
what others think

>
>  	/*
>  	 * The IPA control maps keep their states, so the removal is necessary.
> @@ -451,9 +499,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>  		minGain, maxGain, defGain
>  	};
>  	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
> -		static_cast<int32_t>(minExposure * lineDurationUs),
> -		static_cast<int32_t>(maxExposure * lineDurationUs),
> -		static_cast<int32_t>(defExposure * lineDurationUs),
> +		static_cast<int32_t>(session.minExposureTime.get<std::micro>()),
> +		static_cast<int32_t>(maxExposureTime.get<std::micro>()),
> +		static_cast<int32_t>(state.automatic.exposure * lineDurationUs),

I wonder if keeping the exposure time in ActiveContext (both Manual
and Auto) as a Duration and only converting it to lines when assigning
it to the frame context, or even keeping everything as a Duration and
only converting to lines when populating the control would be nicer.

Not for this patch

>  	};
>  	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
>  		static_cast<int64_t>(session.minFrameDuration.get<std::micro>()),
> @@ -606,7 +654,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
>
>  	const auto &exposure = controls.get(controls::ExposureTime);
>  	if (exposure && !state.autoExposureEnabled) {
> -		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
> +		state.manual.exposure = clampExposure(session, state, *exposure * 1.0us);
>
>  		LOG(Agc, Debug) << "Set exposure to " << state.manual.exposure;
>  	}
> @@ -700,7 +748,7 @@ void AgcAlgorithm::prepare(const agc::Session& session, agc::ActiveState &state,
>  	*/
>  	const auto frameDuration = std::max<uint32_t>(
>  		frameContext.minFrameDuration / session.lineDuration,
> -		frameContext.exposure);
> +		frameContext.exposure + session.sensor.exposureMargin);
>
>  	frameContext.vblank = frameDuration - session.sensor.outputSize.height;
>
> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>
>  	if (state.autoExposureEnabled) {
>  		minExposureTime = session.minExposureTime;
> -		maxExposureTime = std::clamp(state.maxFrameDuration,
> -					     session.minExposureTime,
> -					     session.maxExposureTime);
> +		maxExposureTime =
> +			state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;

trivial to break this line

		maxExposureTime = state.maxFrameDuration
				- session.sensor.exposureMargin * session.lineDuration;

Could you kindly configure your editor to match the coding style, as I
feel like I've repeated the same comment quite some times already

>  	} else {
>  		minExposureTime = lineDuration * state.manual.exposure;
>  		maxExposureTime = minExposureTime;
> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  		},
>  	}, impl_);
>
> +	state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> +
>  	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>
>  	LOG(Agc, Debug)
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 388693d21d..bfb60150db 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -36,8 +36,8 @@ class Histogram;
>  namespace agc {
>
>  struct Session {
> +	uint32_t minExposure;
>  	utils::Duration minExposureTime;
> -	utils::Duration maxExposureTime;
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
>  	double defAnalogueGain;
> @@ -47,6 +47,7 @@ struct Session {
>
>  	struct {
>  		Size outputSize;
> +		uint32_t exposureMargin;
>  	} sensor;
>
>  	bool autoAllowed;

I've tested this, and with a sensor which by default has a very short
exposure, this series makes the AGC push the frame duration until it
reaches the 5fps limits, effectively adjusting durtion to the desired
exposure

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

> --
> 2.55.0
>

2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
>> The sensor exposure time depends on the vertical blanking amount and the
>> exposure margin. Instead of relying on the maximum exposure available at
>> onfiguration, use the frame duration limits (~ vblank) and exposure
>> margin to dynamically calculate the max available exposure time.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
>>   src/ipa/libipa/agc.h   |  3 +-
>>   2 files changed, 73 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index 6783055a5e..6ef1f938bd 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -70,12 +70,12 @@ namespace agc {
>>    * \struct Session
>>    * \brief Session configuration for AgcAlgorithm
>>    *
>> + * \var Session::minExposure
>> + * \brief Minimum exposure (in lines) for the streaming session
>> + *
> 
> Is this used for clamping only, right ?
> 
> We have the same information as a Duration
> 
>>    * \var Session::minExposureTime
>>    * \brief Minimum exposure time for the streaming session
> 
> here
> 
> It would be trivial to convert back and forth from lines to Durations,
> isn't it ? Do we need to store both values ?

Yes, but I'm a bit wary of doing the floating point arithmetic. At multiple
occasions I felt it would be better to use the actual underlying integer
values, that won't ever be affected by rounding, etc.


> 
>>    *
>> - * \var Session::maxExposureTime
>> - * \brief Maximum exposure time for the streaming session
>> - *
>>    * \var Session::minAnalogueGain
>>    * \brief Minimum analogue gain for the streaming session
>>    *
>> @@ -100,6 +100,11 @@ namespace agc {
>>    * \var Session::sensor.outputSize
>>    * \brief Configured output size of the sensor
>>    *
>> + * \var Session::sensor.exposureMargin
>> + * \brief Exposure margin of the sensor
>> + *
>> + * \sa CameraSensorHelper::exposureMargin()
>> + *
>>    * \var Session::autoAllowed
>>    * \copybrief AgcAlgorithm::ConfigurationParams::autoAllowed
>>    * \sa AgcAlgorithm::ConfigurationParams::autoAllowed
>> @@ -221,6 +226,31 @@ namespace agc {
>>
>>   } /* namespace agc */
>>
>> +namespace {
>> +
>> +[[nodiscard]]
>> +uint32_t clampExposure(const agc::Session &session, const agc::ActiveState &state,
>> +		       uint32_t exposure)
>> +{
>> +	return std::max(
>> +		std::min<uint32_t>(
>> +			exposure,
>> +			(state.maxFrameDuration / session.lineDuration) - session.sensor.exposureMargin
> 
> .. it's trivial to shorten this line

Tried something.


> 
>> +		),
>> +		session.minExposure
>> +	);
>> +}
>> +
>> +[[nodiscard]]
>> +uint32_t clampExposure(const agc::Session &session, const agc::ActiveState &state,
>> +		       utils::Duration exposureTime)
>> +{
>> +       return clampExposure(session, state, exposureTime / session.lineDuration);
>> +}
>> +
> 
> isn't it confusing that a function that takes an exposure as
> Duration clamps it back in lines ?
> 
> Do we want this overload ?

Well, if you ask me, no, because both very clearly return `uint32_t`
and are used in places where the result is an exposure amount in lines.

But each one is only really used once, so maybe one could be removed.


> 
>> +} /* namespace */
>> +
>> +
>>   /**
>>    * \class AgcAlgorithm
>>    * \brief libIPA LSC algorithm algorithm
>> @@ -360,6 +390,14 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   	session.lineDuration = lineLength * 1.0s / config.sensorInfo.pixelRate;
>>   	session.sensor.outputSize = config.sensorInfo.outputSize;
>>
>> +	auto exposureMargin = sensor_ ? sensor_->exposureMargin() : std::nullopt;
>> +	session.sensor.exposureMargin = exposureMargin.value_or(4);
>> +	if (!exposureMargin) {
>> +		LOG(Agc, Warning)
>> +			<< "Sensor exposure margin not available, using "
>> +			<< session.sensor.exposureMargin;
>> +	}
> 
> No need for braces

Okay, I'm apparently too used to adding brances to any multi-line blocks.


> 
>> +
>>   	const double lineDurationUs = session.lineDuration.get<std::micro>();
>>
>>   	/*
>> @@ -369,7 +407,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>
>>   	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>();
>>
>>   	/* Compute the analogue gain limits. */
>> @@ -382,12 +419,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   	float maxGain = extractGain(v4l2Gain.max());
>>   	float defGain = extractGain(v4l2Gain.def());
>>
>> -	LOG(Agc, Debug)
>> -		<< "exposure: [" << minExposure << ',' << maxExposure << "], "
>> -		<< "gain: [" << minGain << ',' << maxGain << "], "
>> -		<< "line-duration: " << session.lineDuration << ", "
>> -		<< "sensor-output: " << session.sensor.outputSize;
>> -
>>   	/*
>>   	 * Compute the frame duration limits.
>>   	 *
>> @@ -408,17 +439,32 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   	 * 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.minExposure = minExposure;
>>   	session.minExposureTime = minExposure * session.lineDuration;
>> -	session.maxExposureTime = maxExposure * session.lineDuration;
>>   	session.minAnalogueGain = minGain;
>>   	session.maxAnalogueGain = maxGain;
>>   	session.defAnalogueGain = defGain;
>>   	session.minFrameDuration = frameHeights.min * session.lineDuration;
>>   	session.maxFrameDuration = frameHeights.max * session.lineDuration;
>>
>> +	const uint32_t maxExposure = frameHeights.max - session.sensor.exposureMargin;
>> +	const utils::Duration maxExposureTime = maxExposure * session.lineDuration;
>> +
>> +	ASSERT(frameHeights.max > session.sensor.exposureMargin);
>> +	ASSERT(session.minExposure + session.sensor.exposureMargin <= frameHeights.min);
>> +	ASSERT(static_cast<uint32_t>(session.minExposure) < maxExposure);
>> +
>> +	LOG(Agc, Debug)
>> +		<< "exposure: [" << session.minExposure << ',' << maxExposure << "], "
>> +		<< "exposure-time: [" << session.minExposureTime << ',' << maxExposureTime << "], "
>> +		<< "gain: [" << session.minAnalogueGain << ',' << session.maxAnalogueGain << "], "
>> +		<< "line-length: " << lineLength << ", "
>> +		<< "line-duration: " << session.lineDuration << ", "
>> +		<< "frame-height: [" << frameHeights.min << ',' << frameHeights.max << "], "
>> +		<< "sensor-output: " << session.sensor.outputSize << ", "
>> +		<< "sensor-exposure-margin: " << session.sensor.exposureMargin;
>> +
> 
> If you want to change the logging of the session configuration, maybe
> a tiny patch to do so is worth it ?

Possibly, although I think it's reasonably natural to change the related
logging at the same time as when changing the code.


> 
>>   	/* Configure the default exposure and gain. */
>>   	state = {};
>>   	state.automatic.gain = session.minAnalogueGain;
>> @@ -431,7 +477,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   	state.autoGainEnabled = session.autoAllowed;
>>   	state.exposureValue = 0;
>>   	state.minFrameDuration = session.minFrameDuration;
>> -	state.maxFrameDuration = session.maxFrameDuration;
>> +	state.maxFrameDuration = std::clamp(
>> +		utils::Duration(1.0s / 5), /* Try to achieve at least 5 fps by default. */
>> +		session.minFrameDuration, session.maxFrameDuration);
> 
> Ok, we're here entering policies..
> 
> Personally, I think it's reasonable, but now we're creating a
> something that affects all libIPA platforms, and I would like to know
> what others think

Adding a new member to `ConfigurationParams` to override this seems
like a simple solution to me. Or maybe something in the tuning file?
But in any case, 5fps does not seem unreasonably low, especially that
it should only be used with dark scenes.


> 
>>
>>   	/*
>>   	 * The IPA control maps keep their states, so the removal is necessary.
>> @@ -451,9 +499,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>   		minGain, maxGain, defGain
>>   	};
>>   	config.ctrlMap[&controls::ExposureTime] = ControlInfo{
>> -		static_cast<int32_t>(minExposure * lineDurationUs),
>> -		static_cast<int32_t>(maxExposure * lineDurationUs),
>> -		static_cast<int32_t>(defExposure * lineDurationUs),
>> +		static_cast<int32_t>(session.minExposureTime.get<std::micro>()),
>> +		static_cast<int32_t>(maxExposureTime.get<std::micro>()),
>> +		static_cast<int32_t>(state.automatic.exposure * lineDurationUs),
> 
> I wonder if keeping the exposure time in ActiveContext (both Manual
> and Auto) as a Duration and only converting it to lines when assigning
> it to the frame context, or even keeping everything as a Duration and
> only converting to lines when populating the control would be nicer.
> 
> Not for this patch

I feel like I would go the opposite direction, and keep everything in lines
to avoid as much floating point arithmetic as possible.


> 
>>   	};
>>   	config.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{
>>   		static_cast<int64_t>(session.minFrameDuration.get<std::micro>()),
>> @@ -606,7 +654,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
>>
>>   	const auto &exposure = controls.get(controls::ExposureTime);
>>   	if (exposure && !state.autoExposureEnabled) {
>> -		state.manual.exposure = *exposure * 1.0us / session.lineDuration;
>> +		state.manual.exposure = clampExposure(session, state, *exposure * 1.0us);
>>
>>   		LOG(Agc, Debug) << "Set exposure to " << state.manual.exposure;
>>   	}
>> @@ -700,7 +748,7 @@ void AgcAlgorithm::prepare(const agc::Session& session, agc::ActiveState &state,
>>   	*/
>>   	const auto frameDuration = std::max<uint32_t>(
>>   		frameContext.minFrameDuration / session.lineDuration,
>> -		frameContext.exposure);
>> +		frameContext.exposure + session.sensor.exposureMargin);
>>
>>   	frameContext.vblank = frameDuration - session.sensor.outputSize.height;
>>
>> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>
>>   	if (state.autoExposureEnabled) {
>>   		minExposureTime = session.minExposureTime;
>> -		maxExposureTime = std::clamp(state.maxFrameDuration,
>> -					     session.minExposureTime,
>> -					     session.maxExposureTime);
>> +		maxExposureTime =
>> +			state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> 
> trivial to break this line
> 
> 		maxExposureTime = state.maxFrameDuration
> 				- session.sensor.exposureMargin * session.lineDuration;
> 
> Could you kindly configure your editor to match the coding style, as I
> feel like I've repeated the same comment quite some times already

If you provide me with a blessed clang-format configuration I'd be glad to do that.

That brings me to the following:

1. The change you suggest is undone by clang-format. So what is the point of using
    a code formatter if it will ultimately be overridden by manual review? (And I'm
    not just talking about this case, it has happened to me multiple times.)

2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
    configuration for it, that is enforced strictly, this will keep happening.

3. And consider e.g. new/occasional contributors, their changes pass the provided
    clang-format configuration, and then we want them to change this, that, and whatnot.
    That is not ideal in my opinion.

This really makes me question the point of shipping a `.clang-format` in the libcamera
repository, if it's effectively meaningless. And there is also no configuration provided
for any alternative tool.

So all in all, my opinion is that if one wants conssitent formatting, that has to be
enforced by automated checks.


> 
>>   	} else {
>>   		minExposureTime = lineDuration * state.manual.exposure;
>>   		maxExposureTime = minExposureTime;
>> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>   		},
>>   	}, impl_);
>>
>> +	state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
>> +
>>   	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>>
>>   	LOG(Agc, Debug)
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> index 388693d21d..bfb60150db 100644
>> --- a/src/ipa/libipa/agc.h
>> +++ b/src/ipa/libipa/agc.h
>> @@ -36,8 +36,8 @@ class Histogram;
>>   namespace agc {
>>
>>   struct Session {
>> +	uint32_t minExposure;
>>   	utils::Duration minExposureTime;
>> -	utils::Duration maxExposureTime;
>>   	double minAnalogueGain;
>>   	double maxAnalogueGain;
>>   	double defAnalogueGain;
>> @@ -47,6 +47,7 @@ struct Session {
>>
>>   	struct {
>>   		Size outputSize;
>> +		uint32_t exposureMargin;
>>   	} sensor;
>>
>>   	bool autoAllowed;
> 
> I've tested this, and with a sensor which by default has a very short
> exposure, this series makes the AGC push the frame duration until it
> reaches the 5fps limits, effectively adjusting durtion to the desired
> exposure
> 
> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>> --
>> 2.55.0
>>

Hi Barnabás, Jacopo,

Quoting Jacopo Mondi (2026-08-27 16:52:24)
> Hi Barnabás
V> 
> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> > The sensor exposure time depends on the vertical blanking amount and the
> > exposure margin. Instead of relying on the maximum exposure available at
> > onfiguration, use the frame duration limits (~ vblank) and exposure
> > margin to dynamically calculate the max available exposure time.
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---

... 

> >       /* Configure the default exposure and gain. */
> >       state = {};
> >       state.automatic.gain = session.minAnalogueGain;
> > @@ -431,7 +477,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
> >       state.autoGainEnabled = session.autoAllowed;
> >       state.exposureValue = 0;
> >       state.minFrameDuration = session.minFrameDuration;
> > -     state.maxFrameDuration = session.maxFrameDuration;
> > +     state.maxFrameDuration = std::clamp(
> > +             utils::Duration(1.0s / 5), /* Try to achieve at least 5 fps by default. */
> > +             session.minFrameDuration, session.maxFrameDuration);
> 
> Ok, we're here entering policies..
> 
> Personally, I think it's reasonable, but now we're creating a
> something that affects all libIPA platforms, and I would like to know
> what others think

I ran this branch on my x13s last night, and the framerate went down to
5FPS which was far too slow and blurry for my preferences at least.

I moved to a brighter environment, and couldn't (in that instance) get
the framerate back up.

Often on UVC, cameras will have a supported range of 15-30 FPS to be
able to adapt to dark conditions, so I could envisage 15 being a default
minimum if necessary - but all of this is definitely 'application
specific policy'...

--
Kieran

2026. 08. 28. 11:25 keltezéssel, Kieran Bingham írta:
> Hi Barnabás, Jacopo,
> 
> Quoting Jacopo Mondi (2026-08-27 16:52:24)
>> Hi Barnabás
> V>
>> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
>>> The sensor exposure time depends on the vertical blanking amount and the
>>> exposure margin. Instead of relying on the maximum exposure available at
>>> onfiguration, use the frame duration limits (~ vblank) and exposure
>>> margin to dynamically calculate the max available exposure time.
>>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> ---
> 
> ...
> 
>>>        /* Configure the default exposure and gain. */
>>>        state = {};
>>>        state.automatic.gain = session.minAnalogueGain;
>>> @@ -431,7 +477,9 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>>        state.autoGainEnabled = session.autoAllowed;
>>>        state.exposureValue = 0;
>>>        state.minFrameDuration = session.minFrameDuration;
>>> -     state.maxFrameDuration = session.maxFrameDuration;
>>> +     state.maxFrameDuration = std::clamp(
>>> +             utils::Duration(1.0s / 5), /* Try to achieve at least 5 fps by default. */
>>> +             session.minFrameDuration, session.maxFrameDuration);
>>
>> Ok, we're here entering policies..
>>
>> Personally, I think it's reasonable, but now we're creating a
>> something that affects all libIPA platforms, and I would like to know
>> what others think
> 
> I ran this branch on my x13s last night, and the framerate went down to
> 5FPS which was far too slow and blurry for my preferences at least.
> 
> I moved to a brighter environment, and couldn't (in that instance) get
> the framerate back up.

That's unfortunate. Is that easily repeatable? Any chance you could
provide some logs with `*Agc*:DEBUG` set? And how long did you wait?
Admittedly, from 5fps the recovery takes more time than ideal, so maybe
10 or 15 is a better default, or the algorithm needs to be sped up; or both.


> 
> Often on UVC, cameras will have a supported range of 15-30 FPS to be
> able to adapt to dark conditions, so I could envisage 15 being a default
> minimum if necessary - but all of this is definitely 'application
> specific policy'...

The application can always override `FrameDurationLimits`, they are free
to change this at any time they like. So I don't think we need to do
anything other than settling on a default value.


> 
> --
> Kieran

On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
> 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> > > The sensor exposure time depends on the vertical blanking amount and the
> > > exposure margin. Instead of relying on the maximum exposure available at
> > > onfiguration, use the frame duration limits (~ vblank) and exposure
> > > margin to dynamically calculate the max available exposure time.
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
> > >   src/ipa/libipa/agc.h   |  3 +-
> > >   2 files changed, 73 insertions(+), 23 deletions(-)
> > >

[snip]

> > > @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >
> > >   	if (state.autoExposureEnabled) {
> > >   		minExposureTime = session.minExposureTime;
> > > -		maxExposureTime = std::clamp(state.maxFrameDuration,
> > > -					     session.minExposureTime,
> > > -					     session.maxExposureTime);
> > > +		maxExposureTime =
> > > +			state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> >
> > trivial to break this line
> >
> > 		maxExposureTime = state.maxFrameDuration
> > 				- session.sensor.exposureMargin * session.lineDuration;
> >
> > Could you kindly configure your editor to match the coding style, as I
> > feel like I've repeated the same comment quite some times already
>
> If you provide me with a blessed clang-format configuration I'd be glad to do that.
>
> That brings me to the following:
>
> 1. The change you suggest is undone by clang-format. So what is the point of using
>    a code formatter if it will ultimately be overridden by manual review? (And I'm
>    not just talking about this case, it has happened to me multiple times.)

As this discussion already happened with Milan, and somehow I'm the
one that had made both times comments about conflict between
clang-format and the libcamera coding style, I'll repeat my position
here:

No one has ever asked anyone to integrate clang-format in their
development environment, and the tool has never been considered to
take precedence over the coding style rules.

If you want to change them please send a patch, maybe someone else
will like the idea of fully relying on clang format for code style and
give up our coding style.

I just got this suggestion from checkstyle:

-	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
-		.sensorInfo = context.sensorInfo,
-		.sensorControls = context.sensorControls,
-		.ctrlMap = context.ctrlMap,
-		.autoAllowed = !context.configuration.raw,
-		.maxDigitalGain = context.configuration.compress.supported ?
-					kAgcMaxDigitalGain : 1.0
-	});
+	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });

Do you think it's a good idea ?

>
> 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
>    configuration for it, that is enforced strictly, this will keep happening.
>
> 3. And consider e.g. new/occasional contributors, their changes pass the provided
>    clang-format configuration, and then we want them to change this, that, and whatnot.
>    That is not ideal in my opinion.
>
> This really makes me question the point of shipping a `.clang-format` in the libcamera
> repository, if it's effectively meaningless. And there is also no configuration provided
> for any alternative tool.

Fine, let's drop it :)

>
> So all in all, my opinion is that if one wants conssitent formatting, that has to be
> enforced by automated checks.
>
>
> >
> > >   	} else {
> > >   		minExposureTime = lineDuration * state.manual.exposure;
> > >   		maxExposureTime = minExposureTime;
> > > @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >   		},
> > >   	}, impl_);
> > >
> > > +	state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> > > +
> > >   	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > >
> > >   	LOG(Agc, Debug)
> > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > index 388693d21d..bfb60150db 100644
> > > --- a/src/ipa/libipa/agc.h
> > > +++ b/src/ipa/libipa/agc.h
> > > @@ -36,8 +36,8 @@ class Histogram;
> > >   namespace agc {
> > >
> > >   struct Session {
> > > +	uint32_t minExposure;
> > >   	utils::Duration minExposureTime;
> > > -	utils::Duration maxExposureTime;
> > >   	double minAnalogueGain;
> > >   	double maxAnalogueGain;
> > >   	double defAnalogueGain;
> > > @@ -47,6 +47,7 @@ struct Session {
> > >
> > >   	struct {
> > >   		Size outputSize;
> > > +		uint32_t exposureMargin;
> > >   	} sensor;
> > >
> > >   	bool autoAllowed;
> >
> > I've tested this, and with a sensor which by default has a very short
> > exposure, this series makes the AGC push the frame duration until it
> > reaches the 5fps limits, effectively adjusting durtion to the desired
> > exposure
> >
> > Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > --
> > > 2.55.0
> > >
>

2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:
> On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
>> 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
>>>> The sensor exposure time depends on the vertical blanking amount and the
>>>> exposure margin. Instead of relying on the maximum exposure available at
>>>> onfiguration, use the frame duration limits (~ vblank) and exposure
>>>> margin to dynamically calculate the max available exposure time.
>>>>
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
>>>>    src/ipa/libipa/agc.h   |  3 +-
>>>>    2 files changed, 73 insertions(+), 23 deletions(-)
>>>>
> 
> [snip]
> 
>>>> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>>>
>>>>    	if (state.autoExposureEnabled) {
>>>>    		minExposureTime = session.minExposureTime;
>>>> -		maxExposureTime = std::clamp(state.maxFrameDuration,
>>>> -					     session.minExposureTime,
>>>> -					     session.maxExposureTime);
>>>> +		maxExposureTime =
>>>> +			state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
>>>
>>> trivial to break this line
>>>
>>> 		maxExposureTime = state.maxFrameDuration
>>> 				- session.sensor.exposureMargin * session.lineDuration;
>>>
>>> Could you kindly configure your editor to match the coding style, as I
>>> feel like I've repeated the same comment quite some times already
>>
>> If you provide me with a blessed clang-format configuration I'd be glad to do that.
>>
>> That brings me to the following:
>>
>> 1. The change you suggest is undone by clang-format. So what is the point of using
>>     a code formatter if it will ultimately be overridden by manual review? (And I'm
>>     not just talking about this case, it has happened to me multiple times.)
> 
> As this discussion already happened with Milan, and somehow I'm the
> one that had made both times comments about conflict between
> clang-format and the libcamera coding style, I'll repeat my position
> here:
> 
> No one has ever asked anyone to integrate clang-format in their
> development environment, and the tool has never been considered to
> take precedence over the coding style rules.
> 
> If you want to change them please send a patch, maybe someone else
> will like the idea of fully relying on clang format for code style and
> give up our coding style.
> 
> I just got this suggestion from checkstyle:
> 
> -	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> -		.sensorInfo = context.sensorInfo,
> -		.sensorControls = context.sensorControls,
> -		.ctrlMap = context.ctrlMap,
> -		.autoAllowed = !context.configuration.raw,
> -		.maxDigitalGain = context.configuration.compress.supported ?
> -					kAgcMaxDigitalGain : 1.0
> -	});
> +	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });
> 
> Do you think it's a good idea ?

I don't see that as a big issue because parts can be exempted from formatting
as desired using `// clang-format on/off` comments. Is it pretty? No, but one
can be sure that everything outside is more or less deterministically formatted.


> 
>>
>> 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
>>     configuration for it, that is enforced strictly, this will keep happening.
>>
>> 3. And consider e.g. new/occasional contributors, their changes pass the provided
>>     clang-format configuration, and then we want them to change this, that, and whatnot.
>>     That is not ideal in my opinion.
>>
>> This really makes me question the point of shipping a `.clang-format` in the libcamera
>> repository, if it's effectively meaningless. And there is also no configuration provided
>> for any alternative tool.
> 
> Fine, let's drop it :)

As much as it saddens me to say it, I have to agree since at the moment I would consider
it worse than useless.


> 
>>
>> So all in all, my opinion is that if one wants conssitent formatting, that has to be
>> enforced by automated checks.
>>
>>
>>>
>>>>    	} else {
>>>>    		minExposureTime = lineDuration * state.manual.exposure;
>>>>    		maxExposureTime = minExposureTime;
>>>> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>>>    		},
>>>>    	}, impl_);
>>>>
>>>> +	state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
>>>> +
>>>>    	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
>>>>
>>>>    	LOG(Agc, Debug)
>>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>>>> index 388693d21d..bfb60150db 100644
>>>> --- a/src/ipa/libipa/agc.h
>>>> +++ b/src/ipa/libipa/agc.h
>>>> @@ -36,8 +36,8 @@ class Histogram;
>>>>    namespace agc {
>>>>
>>>>    struct Session {
>>>> +	uint32_t minExposure;
>>>>    	utils::Duration minExposureTime;
>>>> -	utils::Duration maxExposureTime;
>>>>    	double minAnalogueGain;
>>>>    	double maxAnalogueGain;
>>>>    	double defAnalogueGain;
>>>> @@ -47,6 +47,7 @@ struct Session {
>>>>
>>>>    	struct {
>>>>    		Size outputSize;
>>>> +		uint32_t exposureMargin;
>>>>    	} sensor;
>>>>
>>>>    	bool autoAllowed;
>>>
>>> I've tested this, and with a sensor which by default has a very short
>>> exposure, this series makes the AGC push the frame duration until it
>>> reaches the 5fps limits, effectively adjusting durtion to the desired
>>> exposure
>>>
>>> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>
>>>> --
>>>> 2.55.0
>>>>
>>

On Fri, Aug 28, 2026 at 04:39:51PM +0200, Barnabás Pőcze wrote:
> 2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:
> > On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
> > > 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> > > > Hi Barnabás
> > > >
> > > > On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> > > > > The sensor exposure time depends on the vertical blanking amount and the
> > > > > exposure margin. Instead of relying on the maximum exposure available at
> > > > > onfiguration, use the frame duration limits (~ vblank) and exposure
> > > > > margin to dynamically calculate the max available exposure time.
> > > > >
> > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > > ---
> > > > >    src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
> > > > >    src/ipa/libipa/agc.h   |  3 +-
> > > > >    2 files changed, 73 insertions(+), 23 deletions(-)
> > > > >
> >
> > [snip]
> >
> > > > > @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > > > >
> > > > >    	if (state.autoExposureEnabled) {
> > > > >    		minExposureTime = session.minExposureTime;
> > > > > -		maxExposureTime = std::clamp(state.maxFrameDuration,
> > > > > -					     session.minExposureTime,
> > > > > -					     session.maxExposureTime);
> > > > > +		maxExposureTime =
> > > > > +			state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> > > >
> > > > trivial to break this line
> > > >
> > > > 		maxExposureTime = state.maxFrameDuration
> > > > 				- session.sensor.exposureMargin * session.lineDuration;
> > > >
> > > > Could you kindly configure your editor to match the coding style, as I
> > > > feel like I've repeated the same comment quite some times already
> > >
> > > If you provide me with a blessed clang-format configuration I'd be glad to do that.
> > >
> > > That brings me to the following:
> > >
> > > 1. The change you suggest is undone by clang-format. So what is the point of using
> > >     a code formatter if it will ultimately be overridden by manual review? (And I'm
> > >     not just talking about this case, it has happened to me multiple times.)
> >
> > As this discussion already happened with Milan, and somehow I'm the
> > one that had made both times comments about conflict between
> > clang-format and the libcamera coding style, I'll repeat my position
> > here:
> >
> > No one has ever asked anyone to integrate clang-format in their
> > development environment, and the tool has never been considered to
> > take precedence over the coding style rules.
> >
> > If you want to change them please send a patch, maybe someone else
> > will like the idea of fully relying on clang format for code style and
> > give up our coding style.
> >
> > I just got this suggestion from checkstyle:
> >
> > -	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > -		.sensorInfo = context.sensorInfo,
> > -		.sensorControls = context.sensorControls,
> > -		.ctrlMap = context.ctrlMap,
> > -		.autoAllowed = !context.configuration.raw,
> > -		.maxDigitalGain = context.configuration.compress.supported ?
> > -					kAgcMaxDigitalGain : 1.0
> > -	});
> > +	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });
> >
> > Do you think it's a good idea ?
>
> I don't see that as a big issue because parts can be exempted from formatting
> as desired using `// clang-format on/off` comments. Is it pretty? No, but one

No it's not pretty to add clang-format specific comments in the code
base, in my opinion :)

Happy to know if I'm a minority here and back track


> can be sure that everything outside is more or less deterministically formatted.
>
>
> >
> > >
> > > 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
> > >     configuration for it, that is enforced strictly, this will keep happening.
> > >
> > > 3. And consider e.g. new/occasional contributors, their changes pass the provided
> > >     clang-format configuration, and then we want them to change this, that, and whatnot.
> > >     That is not ideal in my opinion.
> > >
> > > This really makes me question the point of shipping a `.clang-format` in the libcamera
> > > repository, if it's effectively meaningless. And there is also no configuration provided
> > > for any alternative tool.
> >
> > Fine, let's drop it :)
>
> As much as it saddens me to say it, I have to agree since at the moment I would consider
> it worse than useless.
>
>
> >
> > >
> > > So all in all, my opinion is that if one wants conssitent formatting, that has to be
> > > enforced by automated checks.
> > >
> > >
> > > >
> > > > >    	} else {
> > > > >    		minExposureTime = lineDuration * state.manual.exposure;
> > > > >    		maxExposureTime = minExposureTime;
> > > > > @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > > > >    		},
> > > > >    	}, impl_);
> > > > >
> > > > > +	state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> > > > > +
> > > > >    	const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > > > >
> > > > >    	LOG(Agc, Debug)
> > > > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > > > index 388693d21d..bfb60150db 100644
> > > > > --- a/src/ipa/libipa/agc.h
> > > > > +++ b/src/ipa/libipa/agc.h
> > > > > @@ -36,8 +36,8 @@ class Histogram;
> > > > >    namespace agc {
> > > > >
> > > > >    struct Session {
> > > > > +	uint32_t minExposure;
> > > > >    	utils::Duration minExposureTime;
> > > > > -	utils::Duration maxExposureTime;
> > > > >    	double minAnalogueGain;
> > > > >    	double maxAnalogueGain;
> > > > >    	double defAnalogueGain;
> > > > > @@ -47,6 +47,7 @@ struct Session {
> > > > >
> > > > >    	struct {
> > > > >    		Size outputSize;
> > > > > +		uint32_t exposureMargin;
> > > > >    	} sensor;
> > > > >
> > > > >    	bool autoAllowed;
> > > >
> > > > I've tested this, and with a sensor which by default has a very short
> > > > exposure, this series makes the AGC push the frame duration until it
> > > > reaches the 5fps limits, effectively adjusting durtion to the desired
> > > > exposure
> > > >
> > > > Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >
> > > > > --
> > > > > 2.55.0
> > > > >
> > >
>

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> 2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:

[...]

>> I just got this suggestion from checkstyle:
>> -	int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
>> -		.sensorInfo = context.sensorInfo,
>> -		.sensorControls = context.sensorControls,
>> -		.ctrlMap = context.ctrlMap,
>> -		.autoAllowed = !context.configuration.raw,
>> -		.maxDigitalGain = context.configuration.compress.supported ?
>> -					kAgcMaxDigitalGain : 1.0
>> -	});
>> + int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo =
>> context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed =
>> !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ?
>> kAgcMaxDigitalGain : 1.0 });
>> Do you think it's a good idea ?

As already discussed once, the formatter doesn't like that multiple
arguments are on the same line, while one of them is split there.  Which
I consider reasonable, the formatter just indicates the issue in a
confusing way (putting everything on a single line rather than adding
newlines as needed).  One has to put the newlines there manually and
then the formatter becomes happy.

[...]

>>> This really makes me question the point of shipping a `.clang-format` in the libcamera
>>> repository, if it's effectively meaningless. And there is also no configuration provided
>>> for any alternative tool.
>> Fine, let's drop it :)
>
> As much as it saddens me to say it, I have to agree since at the moment I would consider
> it worse than useless.

I agree but doesn't checkstyle use it?  Or do you suggest dropping the
formatting check from checkstyle?  How could then the code be formatted
other than completely manually (which I find very difficult to do in
libcamera case)?  I can simply do `checkstyle.py | patch -p0' currently
to avoid wasting review resources on basic formatting issues.

Hi Barnabás, hi Jacopo,

Quoting Barnabás Pőcze (2026-08-28 16:39:51)
> 2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:
> > On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
> >> 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> >>> Hi Barnabás
> >>>
> >>> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> >>>> The sensor exposure time depends on the vertical blanking amount and the
> >>>> exposure margin. Instead of relying on the maximum exposure available at
> >>>> onfiguration, use the frame duration limits (~ vblank) and exposure
> >>>> margin to dynamically calculate the max available exposure time.
> >>>>
> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>> ---
> >>>>    src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
> >>>>    src/ipa/libipa/agc.h   |  3 +-
> >>>>    2 files changed, 73 insertions(+), 23 deletions(-)
> >>>>
> > 
> > [snip]
> > 
> >>>> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> >>>>
> >>>>            if (state.autoExposureEnabled) {
> >>>>                    minExposureTime = session.minExposureTime;
> >>>> -          maxExposureTime = std::clamp(state.maxFrameDuration,
> >>>> -                                       session.minExposureTime,
> >>>> -                                       session.maxExposureTime);
> >>>> +          maxExposureTime =
> >>>> +                  state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> >>>
> >>> trivial to break this line
> >>>
> >>>             maxExposureTime = state.maxFrameDuration
> >>>                             - session.sensor.exposureMargin * session.lineDuration;

In this specific case, the checkstyle accepted version would be:

maxExposureTime = state.maxFrameDuration -
		  session.sensor.exposureMargin * session.lineDuration;

@Jacopo could you go with that?

> >>>
> >>> Could you kindly configure your editor to match the coding style, as I
> >>> feel like I've repeated the same comment quite some times already
> >>
> >> If you provide me with a blessed clang-format configuration I'd be glad to do that.
> >>
> >> That brings me to the following:
> >>
> >> 1. The change you suggest is undone by clang-format. So what is the point of using
> >>     a code formatter if it will ultimately be overridden by manual review? (And I'm
> >>     not just talking about this case, it has happened to me multiple times.)
> > 
> > As this discussion already happened with Milan, and somehow I'm the
> > one that had made both times comments about conflict between
> > clang-format and the libcamera coding style, I'll repeat my position
> > here:
> > 
> > No one has ever asked anyone to integrate clang-format in their
> > development environment, and the tool has never been considered to
> > take precedence over the coding style rules.

Well in https://libcamera.org/coding-style.html#coding-style-guidelines

"The ‘clang-format’ code formatting tool can be used to reformat source
files with the libcamera coding style, ... <snip> ... it may generate
unrelated changes. To avoid this, libcamera provides a ‘checkstyle.py’
script wrapping the formatting tools to only retain related changes.
This should be used to validate modifications before submitting them for
review.

So we are actively asking our contributors to do that.

I think we should differentiate between "reformat everything by running
clang-format" and "try to format the code, so that checkstyle is ok with
it". I'm advocating for the latter.

> > 
> > If you want to change them please send a patch, maybe someone else
> > will like the idea of fully relying on clang format for code style and
> > give up our coding style.
> > 
> > I just got this suggestion from checkstyle:
> > 
> > -     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > -             .sensorInfo = context.sensorInfo,
> > -             .sensorControls = context.sensorControls,
> > -             .ctrlMap = context.ctrlMap,
> > -             .autoAllowed = !context.configuration.raw,
> > -             .maxDigitalGain = context.configuration.compress.supported ?
> > -                                     kAgcMaxDigitalGain : 1.0
> > -     });
> > +     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });
> > 
> > Do you think it's a good idea ?

As Milan mentioned, this can easily be improved by breaking before the
curly brace and adding a comma after the initializer list. So imho no
need for // clang-format on/off

> 
> I don't see that as a big issue because parts can be exempted from formatting
> as desired using `// clang-format on/off` comments. Is it pretty? No, but one
> can be sure that everything outside is more or less deterministically formatted.
> 
> 
> > 
> >>
> >> 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
> >>     configuration for it, that is enforced strictly, this will keep happening.
> >>
> >> 3. And consider e.g. new/occasional contributors, their changes pass the provided
> >>     clang-format configuration, and then we want them to change this, that, and whatnot.
> >>     That is not ideal in my opinion.
> >>
> >> This really makes me question the point of shipping a `.clang-format` in the libcamera
> >> repository, if it's effectively meaningless. And there is also no configuration provided
> >> for any alternative tool.
> > 
> > Fine, let's drop it :)
> 
> As much as it saddens me to say it, I have to agree since at the moment I would consider
> it worse than useless.

That would be really sad.

Best regards,
Stefan

> 
> 
> > 
> >>
> >> So all in all, my opinion is that if one wants conssitent formatting, that has to be
> >> enforced by automated checks.
> >>
> >>
> >>>
> >>>>            } else {
> >>>>                    minExposureTime = lineDuration * state.manual.exposure;
> >>>>                    maxExposureTime = minExposureTime;
> >>>> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> >>>>                    },
> >>>>            }, impl_);
> >>>>
> >>>> +  state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> >>>> +
> >>>>            const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> >>>>
> >>>>            LOG(Agc, Debug)
> >>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> >>>> index 388693d21d..bfb60150db 100644
> >>>> --- a/src/ipa/libipa/agc.h
> >>>> +++ b/src/ipa/libipa/agc.h
> >>>> @@ -36,8 +36,8 @@ class Histogram;
> >>>>    namespace agc {
> >>>>
> >>>>    struct Session {
> >>>> +  uint32_t minExposure;
> >>>>            utils::Duration minExposureTime;
> >>>> -  utils::Duration maxExposureTime;
> >>>>            double minAnalogueGain;
> >>>>            double maxAnalogueGain;
> >>>>            double defAnalogueGain;
> >>>> @@ -47,6 +47,7 @@ struct Session {
> >>>>
> >>>>            struct {
> >>>>                    Size outputSize;
> >>>> +          uint32_t exposureMargin;
> >>>>            } sensor;
> >>>>
> >>>>            bool autoAllowed;
> >>>
> >>> I've tested this, and with a sensor which by default has a very short
> >>> exposure, this series makes the AGC push the frame duration until it
> >>> reaches the 5fps limits, effectively adjusting durtion to the desired
> >>> exposure
> >>>
> >>> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>>
> >>>> --
> >>>> 2.55.0
> >>>>
> >>
>

Hi Stefan

On Wed, Sep 09, 2026 at 03:34:27PM +0200, Stefan Klug wrote:
> Hi Barnabás, hi Jacopo,
>
> Quoting Barnabás Pőcze (2026-08-28 16:39:51)
> > 2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:
> > > On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
> > >> 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> > >>> Hi Barnabás
> > >>>
> > >>> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> > >>>> The sensor exposure time depends on the vertical blanking amount and the
> > >>>> exposure margin. Instead of relying on the maximum exposure available at
> > >>>> onfiguration, use the frame duration limits (~ vblank) and exposure
> > >>>> margin to dynamically calculate the max available exposure time.
> > >>>>
> > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >>>> ---
> > >>>>    src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
> > >>>>    src/ipa/libipa/agc.h   |  3 +-
> > >>>>    2 files changed, 73 insertions(+), 23 deletions(-)
> > >>>>
> > >
> > > [snip]
> > >
> > >>>> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >>>>
> > >>>>            if (state.autoExposureEnabled) {
> > >>>>                    minExposureTime = session.minExposureTime;
> > >>>> -          maxExposureTime = std::clamp(state.maxFrameDuration,
> > >>>> -                                       session.minExposureTime,
> > >>>> -                                       session.maxExposureTime);
> > >>>> +          maxExposureTime =
> > >>>> +                  state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> > >>>
> > >>> trivial to break this line
> > >>>
> > >>>             maxExposureTime = state.maxFrameDuration
> > >>>                             - session.sensor.exposureMargin * session.lineDuration;
>
> In this specific case, the checkstyle accepted version would be:
>
> maxExposureTime = state.maxFrameDuration -
> 		  session.sensor.exposureMargin * session.lineDuration;
>
> @Jacopo could you go with that?
>

Sure, I just feel strongly against doing changes against the coding
style just to please the tool. This one makes perfect sense (even if
my OCD prefers the other version, but this doesn't matter).

Feeling good ?

Don't

Read below

> > >>>
> > >>> Could you kindly configure your editor to match the coding style, as I
> > >>> feel like I've repeated the same comment quite some times already
> > >>
> > >> If you provide me with a blessed clang-format configuration I'd be glad to do that.
> > >>
> > >> That brings me to the following:
> > >>
> > >> 1. The change you suggest is undone by clang-format. So what is the point of using
> > >>     a code formatter if it will ultimately be overridden by manual review? (And I'm
> > >>     not just talking about this case, it has happened to me multiple times.)
> > >
> > > As this discussion already happened with Milan, and somehow I'm the
> > > one that had made both times comments about conflict between
> > > clang-format and the libcamera coding style, I'll repeat my position
> > > here:
> > >
> > > No one has ever asked anyone to integrate clang-format in their
> > > development environment, and the tool has never been considered to
> > > take precedence over the coding style rules.
>
> Well in https://libcamera.org/coding-style.html#coding-style-guidelines
>
> "The ‘clang-format’ code formatting tool can be used to reformat source
> files with the libcamera coding style, ... <snip> ... it may generate
> unrelated changes. To avoid this, libcamera provides a ‘checkstyle.py’
> script wrapping the formatting tools to only retain related changes.
> This should be used to validate modifications before submitting them for
> review.
>
> So we are actively asking our contributors to do that.
>
> I think we should differentiate between "reformat everything by running
> clang-format" and "try to format the code, so that checkstyle is ok with
> it". I'm advocating for the latter.
>
> > >
> > > If you want to change them please send a patch, maybe someone else
> > > will like the idea of fully relying on clang format for code style and
> > > give up our coding style.
> > >
> > > I just got this suggestion from checkstyle:
> > >
> > > -     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > > -             .sensorInfo = context.sensorInfo,
> > > -             .sensorControls = context.sensorControls,
> > > -             .ctrlMap = context.ctrlMap,
> > > -             .autoAllowed = !context.configuration.raw,
> > > -             .maxDigitalGain = context.configuration.compress.supported ?
> > > -                                     kAgcMaxDigitalGain : 1.0
> > > -     });
> > > +     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });
> > >
> > > Do you think it's a good idea ?
>
> As Milan mentioned, this can easily be improved by breaking before the
> curly brace and adding a comma after the initializer list. So imho no
> need for // clang-format on/off
>

One thing I'm wouldn't like is trying different solutions until I
don't find one that pleases the tool.

Either it tells me exactly what to do without producing non-sense like the
above infinitely long line, or I shouldn't be bothered trying to find a
way to please both the tool and the coding style at the same time.

That said, I don't feel like fighting against any of this anymore. If most
people automates formatting with clang-format in their IDE and want
to do that, let's find ways to make it happen. But please do not use
this as an argument to get in clearly wrong stuff as the above
suggestion is.

> >
> > I don't see that as a big issue because parts can be exempted from formatting
> > as desired using `// clang-format on/off` comments. Is it pretty? No, but one
> > can be sure that everything outside is more or less deterministically formatted.
> >
> >
> > >
> > >>
> > >> 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
> > >>     configuration for it, that is enforced strictly, this will keep happening.
> > >>
> > >> 3. And consider e.g. new/occasional contributors, their changes pass the provided
> > >>     clang-format configuration, and then we want them to change this, that, and whatnot.
> > >>     That is not ideal in my opinion.
> > >>
> > >> This really makes me question the point of shipping a `.clang-format` in the libcamera
> > >> repository, if it's effectively meaningless. And there is also no configuration provided
> > >> for any alternative tool.
> > >
> > > Fine, let's drop it :)
> >
> > As much as it saddens me to say it, I have to agree since at the moment I would consider
> > it worse than useless.
>
> That would be really sad.
>
> Best regards,
> Stefan
>
> >
> >
> > >
> > >>
> > >> So all in all, my opinion is that if one wants conssitent formatting, that has to be
> > >> enforced by automated checks.
> > >>
> > >>
> > >>>
> > >>>>            } else {
> > >>>>                    minExposureTime = lineDuration * state.manual.exposure;
> > >>>>                    maxExposureTime = minExposureTime;
> > >>>> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >>>>                    },
> > >>>>            }, impl_);
> > >>>>
> > >>>> +  state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> > >>>> +
> > >>>>            const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > >>>>
> > >>>>            LOG(Agc, Debug)
> > >>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > >>>> index 388693d21d..bfb60150db 100644
> > >>>> --- a/src/ipa/libipa/agc.h
> > >>>> +++ b/src/ipa/libipa/agc.h
> > >>>> @@ -36,8 +36,8 @@ class Histogram;
> > >>>>    namespace agc {
> > >>>>
> > >>>>    struct Session {
> > >>>> +  uint32_t minExposure;
> > >>>>            utils::Duration minExposureTime;
> > >>>> -  utils::Duration maxExposureTime;
> > >>>>            double minAnalogueGain;
> > >>>>            double maxAnalogueGain;
> > >>>>            double defAnalogueGain;
> > >>>> @@ -47,6 +47,7 @@ struct Session {
> > >>>>
> > >>>>            struct {
> > >>>>                    Size outputSize;
> > >>>> +          uint32_t exposureMargin;
> > >>>>            } sensor;
> > >>>>
> > >>>>            bool autoAllowed;
> > >>>
> > >>> I've tested this, and with a sensor which by default has a very short
> > >>> exposure, this series makes the AGC push the frame duration until it
> > >>> reaches the 5fps limits, effectively adjusting durtion to the desired
> > >>> exposure
> > >>>
> > >>> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >>>
> > >>>> --
> > >>>> 2.55.0
> > >>>>
> > >>
> >

Hi Jacopo,

Quoting Jacopo Mondi (2026-09-09 17:16:39)
> Hi Stefan
> 
> On Wed, Sep 09, 2026 at 03:34:27PM +0200, Stefan Klug wrote:
> > Hi Barnabás, hi Jacopo,
> >
> > Quoting Barnabás Pőcze (2026-08-28 16:39:51)
> > > 2026. 08. 28. 14:46 keltezéssel, Jacopo Mondi írta:
> > > > On Fri, Aug 28, 2026 at 10:25:44AM +0200, Barnabás Pőcze wrote:
> > > >> 2026. 08. 27. 17:52 keltezéssel, Jacopo Mondi írta:
> > > >>> Hi Barnabás
> > > >>>
> > > >>> On Thu, Aug 27, 2026 at 12:41:08PM +0200, Barnabás Pőcze wrote:
> > > >>>> The sensor exposure time depends on the vertical blanking amount and the
> > > >>>> exposure margin. Instead of relying on the maximum exposure available at
> > > >>>> onfiguration, use the frame duration limits (~ vblank) and exposure
> > > >>>> margin to dynamically calculate the max available exposure time.
> > > >>>>
> > > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > >>>> ---
> > > >>>>    src/ipa/libipa/agc.cpp | 93 ++++++++++++++++++++++++++++++++----------
> > > >>>>    src/ipa/libipa/agc.h   |  3 +-
> > > >>>>    2 files changed, 73 insertions(+), 23 deletions(-)
> > > >>>>
> > > >
> > > > [snip]
> > > >
> > > >>>> @@ -755,9 +803,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > > >>>>
> > > >>>>            if (state.autoExposureEnabled) {
> > > >>>>                    minExposureTime = session.minExposureTime;
> > > >>>> -          maxExposureTime = std::clamp(state.maxFrameDuration,
> > > >>>> -                                       session.minExposureTime,
> > > >>>> -                                       session.maxExposureTime);
> > > >>>> +          maxExposureTime =
> > > >>>> +                  state.maxFrameDuration - session.sensor.exposureMargin * session.lineDuration;
> > > >>>
> > > >>> trivial to break this line
> > > >>>
> > > >>>             maxExposureTime = state.maxFrameDuration
> > > >>>                             - session.sensor.exposureMargin * session.lineDuration;
> >
> > In this specific case, the checkstyle accepted version would be:
> >
> > maxExposureTime = state.maxFrameDuration -
> >                 session.sensor.exposureMargin * session.lineDuration;
> >
> > @Jacopo could you go with that?
> >
> 
> Sure, I just feel strongly against doing changes against the coding
> style just to please the tool. This one makes perfect sense (even if
> my OCD prefers the other version, but this doesn't matter).
> 
> Feeling good ?
> 
> Don't

Grmpf

> 
> Read below
> 
> > > >>>
> > > >>> Could you kindly configure your editor to match the coding style, as I
> > > >>> feel like I've repeated the same comment quite some times already
> > > >>
> > > >> If you provide me with a blessed clang-format configuration I'd be glad to do that.
> > > >>
> > > >> That brings me to the following:
> > > >>
> > > >> 1. The change you suggest is undone by clang-format. So what is the point of using
> > > >>     a code formatter if it will ultimately be overridden by manual review? (And I'm
> > > >>     not just talking about this case, it has happened to me multiple times.)
> > > >
> > > > As this discussion already happened with Milan, and somehow I'm the
> > > > one that had made both times comments about conflict between
> > > > clang-format and the libcamera coding style, I'll repeat my position
> > > > here:
> > > >
> > > > No one has ever asked anyone to integrate clang-format in their
> > > > development environment, and the tool has never been considered to
> > > > take precedence over the coding style rules.
> >
> > Well in https://libcamera.org/coding-style.html#coding-style-guidelines
> >
> > "The ‘clang-format’ code formatting tool can be used to reformat source
> > files with the libcamera coding style, ... <snip> ... it may generate
> > unrelated changes. To avoid this, libcamera provides a ‘checkstyle.py’
> > script wrapping the formatting tools to only retain related changes.
> > This should be used to validate modifications before submitting them for
> > review.
> >
> > So we are actively asking our contributors to do that.
> >
> > I think we should differentiate between "reformat everything by running
> > clang-format" and "try to format the code, so that checkstyle is ok with
> > it". I'm advocating for the latter.
> >
> > > >
> > > > If you want to change them please send a patch, maybe someone else
> > > > will like the idea of fully relying on clang format for code style and
> > > > give up our coding style.
> > > >
> > > > I just got this suggestion from checkstyle:
> > > >
> > > > -     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, {
> > > > -             .sensorInfo = context.sensorInfo,
> > > > -             .sensorControls = context.sensorControls,
> > > > -             .ctrlMap = context.ctrlMap,
> > > > -             .autoAllowed = !context.configuration.raw,
> > > > -             .maxDigitalGain = context.configuration.compress.supported ?
> > > > -                                     kAgcMaxDigitalGain : 1.0
> > > > -     });
> > > > +     int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { .sensorInfo = context.sensorInfo, .sensorControls = context.sensorControls, .ctrlMap = context.ctrlMap, .autoAllowed = !context.configuration.raw, .maxDigitalGain = context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0 });
> > > >
> > > > Do you think it's a good idea ?
> >
> > As Milan mentioned, this can easily be improved by breaking before the
> > curly brace and adding a comma after the initializer list. So imho no
> > need for // clang-format on/off
> >
> 
> One thing I'm wouldn't like is trying different solutions until I
> don't find one that pleases the tool.

This ignores that the tool is very helpful in 90% of the cases where we
don't have to argue about a missing space after if or a curly brace on a
separate line.

> 
> Either it tells me exactly what to do without producing non-sense like the
> above infinitely long line, or I shouldn't be bothered trying to find a
> way to please both the tool and the coding style at the same time.

We don't have to argue about that line. It is nonsense and I guess a bug
in the tool. In newer versions of clang there are more flexible options
but these are not widely installed on current distros.

Just adding two line breaks and a comma produces this output:

	int ret = agc_.configure(
		context.configuration.agc, context.activeState.agc,
		{
			.sensorInfo = context.sensorInfo,
			.sensorControls = context.sensorControls,
			.ctrlMap = context.ctrlMap,
			.autoAllowed = !context.configuration.raw,
			.maxDigitalGain =
				context.configuration.compress.supported ? kAgcMaxDigitalGain : 1.0,
		});

Which is way more acceptable. Imho these cases/bugs are relatively rare
and it is easy to get them right (to pass checkstyle).

> 
> That said, I don't feel like fighting against any of this anymore. If most
> people automates formatting with clang-format in their IDE and want
> to do that, let's find ways to make it happen. But please do not use
> this as an argument to get in clearly wrong stuff as the above
> suggestion is.

I don't want to advocate for that. We won't find a clang-format that
makes everyone happy and automates everything (and is supported in a
clang version that is available everywhere). Reformatting the whole
codebase would be a nightmare. I just want to keep the current
post-commit-hook active and fix the formatting for cases where it is
easily doable (like above).

Best regards,
Stefan

> 
> > >
> > > I don't see that as a big issue because parts can be exempted from formatting
> > > as desired using `// clang-format on/off` comments. Is it pretty? No, but one
> > > can be sure that everything outside is more or less deterministically formatted.
> > >
> > >
> > > >
> > > >>
> > > >> 2. I'm fairly convinced that as long as there isn't a blessed formatter and blessed
> > > >>     configuration for it, that is enforced strictly, this will keep happening.
> > > >>
> > > >> 3. And consider e.g. new/occasional contributors, their changes pass the provided
> > > >>     clang-format configuration, and then we want them to change this, that, and whatnot.
> > > >>     That is not ideal in my opinion.
> > > >>
> > > >> This really makes me question the point of shipping a `.clang-format` in the libcamera
> > > >> repository, if it's effectively meaningless. And there is also no configuration provided
> > > >> for any alternative tool.
> > > >
> > > > Fine, let's drop it :)
> > >
> > > As much as it saddens me to say it, I have to agree since at the moment I would consider
> > > it worse than useless.
> >
> > That would be really sad.
> >
> > Best regards,
> > Stefan
> >
> > >
> > >
> > > >
> > > >>
> > > >> So all in all, my opinion is that if one wants conssitent formatting, that has to be
> > > >> enforced by automated checks.
> > > >>
> > > >>
> > > >>>
> > > >>>>            } else {
> > > >>>>                    minExposureTime = lineDuration * state.manual.exposure;
> > > >>>>                    maxExposureTime = minExposureTime;
> > > >>>> @@ -829,6 +876,8 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > > >>>>                    },
> > > >>>>            }, impl_);
> > > >>>>
> > > >>>> +  state.automatic.exposure = clampExposure(session, state, state.automatic.exposure);
> > > >>>> +
> > > >>>>            const utils::Duration newExposureTime = state.automatic.exposure * lineDuration;
> > > >>>>
> > > >>>>            LOG(Agc, Debug)
> > > >>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > >>>> index 388693d21d..bfb60150db 100644
> > > >>>> --- a/src/ipa/libipa/agc.h
> > > >>>> +++ b/src/ipa/libipa/agc.h
> > > >>>> @@ -36,8 +36,8 @@ class Histogram;
> > > >>>>    namespace agc {
> > > >>>>
> > > >>>>    struct Session {
> > > >>>> +  uint32_t minExposure;
> > > >>>>            utils::Duration minExposureTime;
> > > >>>> -  utils::Duration maxExposureTime;
> > > >>>>            double minAnalogueGain;
> > > >>>>            double maxAnalogueGain;
> > > >>>>            double defAnalogueGain;
> > > >>>> @@ -47,6 +47,7 @@ struct Session {
> > > >>>>
> > > >>>>            struct {
> > > >>>>                    Size outputSize;
> > > >>>> +          uint32_t exposureMargin;
> > > >>>>            } sensor;
> > > >>>>
> > > >>>>            bool autoAllowed;
> > > >>>
> > > >>> I've tested this, and with a sensor which by default has a very short
> > > >>> exposure, this series makes the AGC push the frame duration until it
> > > >>> reaches the 5fps limits, effectively adjusting durtion to the desired
> > > >>> exposure
> > > >>>
> > > >>> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >>>
> > > >>>> --
> > > >>>> 2.55.0
> > > >>>>
> > > >>
> > >