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

28114 diff mbox series

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(-)

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
>

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
>>