[RFC,v1,4/8] ipa: libipa: agc: Take parameters from active state for calculation
diff mbox series

Message ID 20260827104108.1432632-5-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • ipa: libipa: agc: Take exposure margin into account
Related show

Commit Message

Barnabás Pőcze Aug. 27, 2026, 10:41 a.m. UTC 
The `frameContext` is of an already completed frame, its exposure/gain/etc
is of no real concern for future frames, so take the most recent settings
from the active state. This is still not ideal, but better than the
previous status quo.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Stefan Klug Aug. 27, 2026, 1:34 p.m. UTC | #1 
Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-27 12:41:04)
> The `frameContext` is of an already completed frame, its exposure/gain/etc
> is of no real concern for future frames, so take the most recent settings
> from the active state. This is still not ideal, but better than the
> previous status quo.

I agree with you here. And I agree that it is still not correct. Ideally
we would apply the limits and do the exposure splitting in prepare().
But then the documentation says that prepare should run as fast as
possible which maybe forgis the histogram calculations...

For now we can do that.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index b969118063..34379c0d61 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -754,24 +754,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>         double minAnalogueGain;
>         double maxAnalogueGain;
>  
> -       /* \todo This uses the configuration from an already completed frame. */
> -
> -       if (frameContext.autoExposureEnabled) {
> +       if (state.autoExposureEnabled) {
>                 minExposureTime = session.minExposureTime;
> -               maxExposureTime = std::clamp(frameContext.maxFrameDuration,
> +               maxExposureTime = std::clamp(state.maxFrameDuration,
>                                              session.minExposureTime,
>                                              session.maxExposureTime);
>         } else {
> -               minExposureTime = lineDuration * frameContext.exposure;
> +               minExposureTime = lineDuration * state.manual.exposure;
>                 maxExposureTime = minExposureTime;
>         }
>  
> -       if (frameContext.autoGainEnabled) {
> +       if (state.autoGainEnabled) {
>                 minAnalogueGain = session.minAnalogueGain;
>                 maxAnalogueGain = session.maxAnalogueGain;
>         } else {
> -               minAnalogueGain = frameContext.gain;
> -               maxAnalogueGain = frameContext.gain;
> +               minAnalogueGain = state.manual.gain;
> +               maxAnalogueGain = state.manual.gain;
>         }
>  
>         std::visit(utils::overloaded{
> @@ -817,10 +815,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>                                 .traits = params->traits,
>                                 .yHist = params->yHist,
>                                 .effectiveExposureValue = effectiveExposureValue,
> -                               .constraintModeIndex = frameContext.constraintMode,
> -                               .exposureModeIndex = frameContext.exposureMode,
> +                               .constraintModeIndex = state.constraintMode,
> +                               .exposureModeIndex = state.exposureMode,
>                                 .lux = params->lux,
> -                               .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> +                               .exposureCompensation = std::pow(2.0, state.exposureValue),
>                         });
>  
>                         /* Update the estimated exposure and gain. */
> -- 
> 2.55.0
>
Barnabás Pőcze Aug. 27, 2026, 1:42 p.m. UTC | #2 
2026. 08. 27. 15:34 keltezéssel, Stefan Klug írta:
> Hi Barnabás,
> 
> Quoting Barnabás Pőcze (2026-08-27 12:41:04)
>> The `frameContext` is of an already completed frame, its exposure/gain/etc
>> is of no real concern for future frames, so take the most recent settings
>> from the active state. This is still not ideal, but better than the
>> previous status quo.
> 
> I agree with you here. And I agree that it is still not correct. Ideally
> we would apply the limits and do the exposure splitting in prepare().
> But then the documentation says that prepare should run as fast as
> possible which maybe forgis the histogram calculations...

Yes, and to do that, it would also need to save the statistics of the previous frame.
So it didn't seem like a trivial change.


> 
> For now we can do that.
> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp | 20 +++++++++-----------
>>   1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index b969118063..34379c0d61 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -754,24 +754,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>          double minAnalogueGain;
>>          double maxAnalogueGain;
>>   
>> -       /* \todo This uses the configuration from an already completed frame. */
>> -
>> -       if (frameContext.autoExposureEnabled) {
>> +       if (state.autoExposureEnabled) {
>>                  minExposureTime = session.minExposureTime;
>> -               maxExposureTime = std::clamp(frameContext.maxFrameDuration,
>> +               maxExposureTime = std::clamp(state.maxFrameDuration,
>>                                               session.minExposureTime,
>>                                               session.maxExposureTime);
>>          } else {
>> -               minExposureTime = lineDuration * frameContext.exposure;
>> +               minExposureTime = lineDuration * state.manual.exposure;
>>                  maxExposureTime = minExposureTime;
>>          }
>>   
>> -       if (frameContext.autoGainEnabled) {
>> +       if (state.autoGainEnabled) {
>>                  minAnalogueGain = session.minAnalogueGain;
>>                  maxAnalogueGain = session.maxAnalogueGain;
>>          } else {
>> -               minAnalogueGain = frameContext.gain;
>> -               maxAnalogueGain = frameContext.gain;
>> +               minAnalogueGain = state.manual.gain;
>> +               maxAnalogueGain = state.manual.gain;
>>          }
>>   
>>          std::visit(utils::overloaded{
>> @@ -817,10 +815,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>                                  .traits = params->traits,
>>                                  .yHist = params->yHist,
>>                                  .effectiveExposureValue = effectiveExposureValue,
>> -                               .constraintModeIndex = frameContext.constraintMode,
>> -                               .exposureModeIndex = frameContext.exposureMode,
>> +                               .constraintModeIndex = state.constraintMode,
>> +                               .exposureModeIndex = state.exposureMode,
>>                                  .lux = params->lux,
>> -                               .exposureCompensation = std::pow(2.0, frameContext.exposureValue),
>> +                               .exposureCompensation = std::pow(2.0, state.exposureValue),
>>                          });
>>   
>>                          /* Update the estimated exposure and gain. */
>> -- 
>> 2.55.0
>>
Jacopo Mondi Aug. 27, 2026, 3:19 p.m. UTC | #3 
Hi Barnabás

On Thu, Aug 27, 2026 at 12:41:04PM +0200, Barnabás Pőcze wrote:
> The `frameContext` is of an already completed frame, its exposure/gain/etc
> is of no real concern for future frames, so take the most recent settings
> from the active state. This is still not ideal, but better than the
> previous status quo.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

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

> ---
>  src/ipa/libipa/agc.cpp | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index b969118063..34379c0d61 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -754,24 +754,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  	double minAnalogueGain;
>  	double maxAnalogueGain;
>
> -	/* \todo This uses the configuration from an already completed frame. */
> -
> -	if (frameContext.autoExposureEnabled) {
> +	if (state.autoExposureEnabled) {
>  		minExposureTime = session.minExposureTime;
> -		maxExposureTime = std::clamp(frameContext.maxFrameDuration,
> +		maxExposureTime = std::clamp(state.maxFrameDuration,
>  					     session.minExposureTime,
>  					     session.maxExposureTime);
>  	} else {
> -		minExposureTime = lineDuration * frameContext.exposure;
> +		minExposureTime = lineDuration * state.manual.exposure;
>  		maxExposureTime = minExposureTime;
>  	}
>
> -	if (frameContext.autoGainEnabled) {
> +	if (state.autoGainEnabled) {
>  		minAnalogueGain = session.minAnalogueGain;
>  		maxAnalogueGain = session.maxAnalogueGain;
>  	} else {
> -		minAnalogueGain = frameContext.gain;
> -		maxAnalogueGain = frameContext.gain;
> +		minAnalogueGain = state.manual.gain;
> +		maxAnalogueGain = state.manual.gain;
>  	}
>
>  	std::visit(utils::overloaded{
> @@ -817,10 +815,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  				.traits = params->traits,
>  				.yHist = params->yHist,
>  				.effectiveExposureValue = effectiveExposureValue,
> -				.constraintModeIndex = frameContext.constraintMode,
> -				.exposureModeIndex = frameContext.exposureMode,
> +				.constraintModeIndex = state.constraintMode,
> +				.exposureModeIndex = state.exposureMode,
>  				.lux = params->lux,
> -				.exposureCompensation = std::pow(2.0, frameContext.exposureValue),
> +				.exposureCompensation = std::pow(2.0, state.exposureValue),
>  			});
>
>  			/* Update the estimated exposure and gain. */
> --
> 2.55.0
>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
index b969118063..34379c0d61 100644
--- a/src/ipa/libipa/agc.cpp
+++ b/src/ipa/libipa/agc.cpp
@@ -754,24 +754,22 @@  void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
 	double minAnalogueGain;
 	double maxAnalogueGain;
 
-	/* \todo This uses the configuration from an already completed frame. */
-
-	if (frameContext.autoExposureEnabled) {
+	if (state.autoExposureEnabled) {
 		minExposureTime = session.minExposureTime;
-		maxExposureTime = std::clamp(frameContext.maxFrameDuration,
+		maxExposureTime = std::clamp(state.maxFrameDuration,
 					     session.minExposureTime,
 					     session.maxExposureTime);
 	} else {
-		minExposureTime = lineDuration * frameContext.exposure;
+		minExposureTime = lineDuration * state.manual.exposure;
 		maxExposureTime = minExposureTime;
 	}
 
-	if (frameContext.autoGainEnabled) {
+	if (state.autoGainEnabled) {
 		minAnalogueGain = session.minAnalogueGain;
 		maxAnalogueGain = session.maxAnalogueGain;
 	} else {
-		minAnalogueGain = frameContext.gain;
-		maxAnalogueGain = frameContext.gain;
+		minAnalogueGain = state.manual.gain;
+		maxAnalogueGain = state.manual.gain;
 	}
 
 	std::visit(utils::overloaded{
@@ -817,10 +815,10 @@  void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
 				.traits = params->traits,
 				.yHist = params->yHist,
 				.effectiveExposureValue = effectiveExposureValue,
-				.constraintModeIndex = frameContext.constraintMode,
-				.exposureModeIndex = frameContext.exposureMode,
+				.constraintModeIndex = state.constraintMode,
+				.exposureModeIndex = state.exposureMode,
 				.lux = params->lux,
-				.exposureCompensation = std::pow(2.0, frameContext.exposureValue),
+				.exposureCompensation = std::pow(2.0, state.exposureValue),
 			});
 
 			/* Update the estimated exposure and gain. */