[RFC,v2,22/43] ipa: libipa: agc: Clamp exposure value
diff mbox series

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

Commit Message

Barnabás Pőcze July 23, 2026, 3:43 p.m. UTC 
It is possible that the minimum exposure time in microseconds is not an
integer. In that case the minimum `ExposureTime` in the `ControlInfo` will
therefore be actually lower than the real minimum. This can cause issues:
if the minimum published exposure time is set, the division when setting
`agc.manual.exposure` might produce a value in [0;1), leading to the manual
exposure being set to 0, leading to an assertion failure in
`ExposureModeHelper::splitExposure()`.

Note that this is theoretically possible even when the exposure time is
determined automatically by `AgcMeanLuminance`.

Store the true integer limits of the exposure as well, and ensure that
the exposure (time) -> exposure (line) conversions always clamp.

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

Comments

Jacopo Mondi July 27, 2026, 1:54 p.m. UTC | #1 
Hi Barnabás

On Thu, Jul 23, 2026 at 05:43:05PM +0200, Barnabás Pőcze wrote:
> It is possible that the minimum exposure time in microseconds is not an
> integer. In that case the minimum `ExposureTime` in the `ControlInfo` will
> therefore be actually lower than the real minimum. This can cause issues:
> if the minimum published exposure time is set, the division when setting
> `agc.manual.exposure` might produce a value in [0;1), leading to the manual
> exposure being set to 0, leading to an assertion failure in

I'm not sure how can this go to 0 as the min exposure is usually some
number of lines.

But I understand it's certainly possible to go below the minimum due
to rounding errors

> `ExposureModeHelper::splitExposure()`.
>
> Note that this is theoretically possible even when the exposure time is
> determined automatically by `AgcMeanLuminance`.
>
> Store the true integer limits of the exposure as well, and ensure that
> the exposure (time) -> exposure (line) conversions always clamp.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp | 27 ++++++++++++++++++++++++---
>  src/ipa/libipa/agc.h   |  2 ++
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index e16a02fdde..8a2525ec98 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -38,6 +38,12 @@ LOG_DEFINE_CATEGORY(Agc)
>   * \struct agc::Session
>   * \brief Session configuration for AgcAlgorithm
>   *
> + * \var agc::Session::minExposure
> + * \brief Minimum exposure (in lines) supported with the configured sensor
> + *
> + * \var agc::Session::maxExposure
> + * \brief Maximum exposure (in lines) supported with the configured sensor
> + *
>   * \var agc::Session::minExposureTime
>   * \brief Minimum exposure time supported with the configured sensor
>   *
> @@ -223,6 +229,19 @@ LOG_DEFINE_CATEGORY(Agc)
>   * \brief Effective lux value of the frame
>   */
>
> +namespace {
> +
> +[[nodiscard]] uint32_t clampExposure(utils::Duration exposureTime, const agc::Session &session)
> +{
> +	return std::clamp<uint32_t>(
> +		exposureTime / session.lineDuration,
> +		session.minExposure,
> +		session.maxExposure
> +	);
> +}
> +
> +} /* namespace */
> +
>  /**
>   * \brief Load tuning data
>   */
> @@ -317,6 +336,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>  	 *
>  	 * \todo take VBLANK into account for maximum exposure time
>  	 */
> +	session.minExposure = minExposure;
> +	session.maxExposure = maxExposure;
>  	session.minExposureTime = minExposure * session.lineDuration;
>  	session.maxExposureTime = maxExposure * session.lineDuration;
>  	session.minAnalogueGain = minGain;
> @@ -331,7 +352,7 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>  	/* Configure the default exposure and gain. */
>  	state = {};
>  	state.automatic.gain = session.minAnalogueGain;
> -	state.automatic.exposure = 10ms / session.lineDuration;
> +	state.automatic.exposure = clampExposure(10ms, session);
>  	state.automatic.quantizationGain = 1;
>  	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
>  	state.manual.gain = state.automatic.gain;
> @@ -437,7 +458,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(*exposure * 1us, session);
>
>  		LOG(Agc, Debug)
>  			<< "Set exposure to " << state.manual.exposure;
> @@ -599,7 +620,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
>
>  		/* Update the estimated exposure and gain. */
> -		state.automatic.exposure = newEv.exposureTime / lineDuration;
> +		state.automatic.exposure = clampExposure(newEv.exposureTime, session);
>  		state.automatic.gain = newEv.analogueGain;
>  		state.automatic.quantizationGain = newEv.quantizationGain;
>  		state.automatic.yTarget = newEv.yTarget;
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> index 1eace12908..c11dbf80cd 100644
> --- a/src/ipa/libipa/agc.h
> +++ b/src/ipa/libipa/agc.h
> @@ -49,6 +49,8 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>  }
>
>  struct Session {
> +	uint32_t minExposure;
> +	uint32_t maxExposure;
>  	utils::Duration minExposureTime;
>  	utils::Duration maxExposureTime;
>  	double minAnalogueGain;

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

> --
> 2.55.0
>
Barnabás Pőcze July 27, 2026, 2:49 p.m. UTC | #2 
2026. 07. 27. 15:54 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Jul 23, 2026 at 05:43:05PM +0200, Barnabás Pőcze wrote:
>> It is possible that the minimum exposure time in microseconds is not an
>> integer. In that case the minimum `ExposureTime` in the `ControlInfo` will
>> therefore be actually lower than the real minimum. This can cause issues:
>> if the minimum published exposure time is set, the division when setting
>> `agc.manual.exposure` might produce a value in [0;1), leading to the manual
>> exposure being set to 0, leading to an assertion failure in
> 
> I'm not sure how can this go to 0 as the min exposure is usually some
> number of lines.

The "problematic part" is

   state.manual.exposure = *exposure * 1.0us / session.lineDuration;

Firstly, if the application does not obey the `ExposureTime` limits,
it can set it to 0us, trivially leading to the issue.

However, even if it adheres to the limits, the issues is that the minimum
exposure time is determined by division, and then float->integer truncation,
essentially leading to rounding down.

So it could be that the true min exposure time is 13.36us, but that will
go into the `ControlInfo` as 13. So setting an exposure of 13:

   (*exposure * 1.0us / session.lineDuration) == 0.X

and then due to the truncation causes `state.manual.exposure == 0`.

So rounding-up for the minimum seems like a sensible choice in any case,
but that's not implemented here.

And maybe the `clampGain()` below should do proper rounding before the clamping.

> 
> But I understand it's certainly possible to go below the minimum due
> to rounding errors
> 
>> `ExposureModeHelper::splitExposure()`.
>>
>> Note that this is theoretically possible even when the exposure time is
>> determined automatically by `AgcMeanLuminance`.
>>
>> Store the true integer limits of the exposure as well, and ensure that
>> the exposure (time) -> exposure (line) conversions always clamp.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp | 27 ++++++++++++++++++++++++---
>>   src/ipa/libipa/agc.h   |  2 ++
>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index e16a02fdde..8a2525ec98 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -38,6 +38,12 @@ LOG_DEFINE_CATEGORY(Agc)
>>    * \struct agc::Session
>>    * \brief Session configuration for AgcAlgorithm
>>    *
>> + * \var agc::Session::minExposure
>> + * \brief Minimum exposure (in lines) supported with the configured sensor
>> + *
>> + * \var agc::Session::maxExposure
>> + * \brief Maximum exposure (in lines) supported with the configured sensor
>> + *
>>    * \var agc::Session::minExposureTime
>>    * \brief Minimum exposure time supported with the configured sensor
>>    *
>> @@ -223,6 +229,19 @@ LOG_DEFINE_CATEGORY(Agc)
>>    * \brief Effective lux value of the frame
>>    */
>>
>> +namespace {
>> +
>> +[[nodiscard]] uint32_t clampExposure(utils::Duration exposureTime, const agc::Session &session)
>> +{
>> +	return std::clamp<uint32_t>(
>> +		exposureTime / session.lineDuration,
>> +		session.minExposure,
>> +		session.maxExposure
>> +	);
>> +}
>> +
>> +} /* namespace */
>> +
>>   /**
>>    * \brief Load tuning data
>>    */
>> @@ -317,6 +336,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>>   	 *
>>   	 * \todo take VBLANK into account for maximum exposure time
>>   	 */
>> +	session.minExposure = minExposure;
>> +	session.maxExposure = maxExposure;
>>   	session.minExposureTime = minExposure * session.lineDuration;
>>   	session.maxExposureTime = maxExposure * session.lineDuration;
>>   	session.minAnalogueGain = minGain;
>> @@ -331,7 +352,7 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
>>   	/* Configure the default exposure and gain. */
>>   	state = {};
>>   	state.automatic.gain = session.minAnalogueGain;
>> -	state.automatic.exposure = 10ms / session.lineDuration;
>> +	state.automatic.exposure = clampExposure(10ms, session);
>>   	state.automatic.quantizationGain = 1;
>>   	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
>>   	state.manual.gain = state.automatic.gain;
>> @@ -437,7 +458,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(*exposure * 1us, session);
>>
>>   		LOG(Agc, Debug)
>>   			<< "Set exposure to " << state.manual.exposure;
>> @@ -599,7 +620,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>   			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
>>
>>   		/* Update the estimated exposure and gain. */
>> -		state.automatic.exposure = newEv.exposureTime / lineDuration;
>> +		state.automatic.exposure = clampExposure(newEv.exposureTime, session);
>>   		state.automatic.gain = newEv.analogueGain;
>>   		state.automatic.quantizationGain = newEv.quantizationGain;
>>   		state.automatic.yTarget = newEv.yTarget;
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> index 1eace12908..c11dbf80cd 100644
>> --- a/src/ipa/libipa/agc.h
>> +++ b/src/ipa/libipa/agc.h
>> @@ -49,6 +49,8 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>>   }
>>
>>   struct Session {
>> +	uint32_t minExposure;
>> +	uint32_t maxExposure;
>>   	utils::Duration minExposureTime;
>>   	utils::Duration maxExposureTime;
>>   	double minAnalogueGain;
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>> --
>> 2.55.0
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
index e16a02fdde..8a2525ec98 100644
--- a/src/ipa/libipa/agc.cpp
+++ b/src/ipa/libipa/agc.cpp
@@ -38,6 +38,12 @@  LOG_DEFINE_CATEGORY(Agc)
  * \struct agc::Session
  * \brief Session configuration for AgcAlgorithm
  *
+ * \var agc::Session::minExposure
+ * \brief Minimum exposure (in lines) supported with the configured sensor
+ *
+ * \var agc::Session::maxExposure
+ * \brief Maximum exposure (in lines) supported with the configured sensor
+ *
  * \var agc::Session::minExposureTime
  * \brief Minimum exposure time supported with the configured sensor
  *
@@ -223,6 +229,19 @@  LOG_DEFINE_CATEGORY(Agc)
  * \brief Effective lux value of the frame
  */
 
+namespace {
+
+[[nodiscard]] uint32_t clampExposure(utils::Duration exposureTime, const agc::Session &session)
+{
+	return std::clamp<uint32_t>(
+		exposureTime / session.lineDuration,
+		session.minExposure,
+		session.maxExposure
+	);
+}
+
+} /* namespace */
+
 /**
  * \brief Load tuning data
  */
@@ -317,6 +336,8 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
 	 *
 	 * \todo take VBLANK into account for maximum exposure time
 	 */
+	session.minExposure = minExposure;
+	session.maxExposure = maxExposure;
 	session.minExposureTime = minExposure * session.lineDuration;
 	session.maxExposureTime = maxExposure * session.lineDuration;
 	session.minAnalogueGain = minGain;
@@ -331,7 +352,7 @@  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons
 	/* Configure the default exposure and gain. */
 	state = {};
 	state.automatic.gain = session.minAnalogueGain;
-	state.automatic.exposure = 10ms / session.lineDuration;
+	state.automatic.exposure = clampExposure(10ms, session);
 	state.automatic.quantizationGain = 1;
 	state.automatic.yTarget = impl_.effectiveYTarget(0, 1);
 	state.manual.gain = state.automatic.gain;
@@ -437,7 +458,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(*exposure * 1us, session);
 
 		LOG(Agc, Debug)
 			<< "Set exposure to " << state.manual.exposure;
@@ -599,7 +620,7 @@  void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
 			<< ", " << newEv.quantizationGain << " and " << newEv.digitalGain;
 
 		/* Update the estimated exposure and gain. */
-		state.automatic.exposure = newEv.exposureTime / lineDuration;
+		state.automatic.exposure = clampExposure(newEv.exposureTime, session);
 		state.automatic.gain = newEv.analogueGain;
 		state.automatic.quantizationGain = newEv.quantizationGain;
 		state.automatic.yTarget = newEv.yTarget;
diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
index 1eace12908..c11dbf80cd 100644
--- a/src/ipa/libipa/agc.h
+++ b/src/ipa/libipa/agc.h
@@ -49,6 +49,8 @@  prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
 }
 
 struct Session {
+	uint32_t minExposure;
+	uint32_t maxExposure;
 	utils::Duration minExposureTime;
 	utils::Duration maxExposureTime;
 	double minAnalogueGain;