[RFC,v1,1/8] ipa: libipa: agc: Keep frame duration limits ordered

28115 diff mbox series

The frame duration limits from controls are already clamped, but that does
not prevent a situation where min > max, so use the just acquired min frame
duration as the lower bound when clamping the max value in order to avoid
this possibility.

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

Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-27 12:41:01)
> The frame duration limits from controls are already clamped, but that does
> not prevent a situation where min > max, so use the just acquired min frame
> duration as the lower bound when clamping the max value in order to avoid
> this possibility.

It took me the next patch to understand why this makes sense. I had
trouble understanding the commit message because the calculation of
state.MinFrameDuration is not in the context. What about:

The frame duration limits from controls get clamped to the session
limits, but that does not prevent a situation where min > max. Use the
just computed session.minFrameDuration as the lower bound when clamping
the max value in order to avoid this possibility.

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

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

Best regards,
Stefan

> ---
>  src/ipa/libipa/agc.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 2c1d2169f2..f9edd9295a 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -642,7 +642,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
>  
>                 state.maxFrameDuration = std::clamp<utils::Duration>(
>                         std::chrono::microseconds((*frameDurationLimits).back()),
> -                       session.minFrameDuration, session.maxFrameDuration);
> +                       state.minFrameDuration, session.maxFrameDuration);
>         }
>         frameContext.minFrameDuration = state.minFrameDuration;
>         frameContext.maxFrameDuration = state.maxFrameDuration;
> -- 
> 2.55.0
>

2026. 08. 27. 15:13 keltezéssel, Stefan Klug írta:
> Hi Barnabás,
> 
> Quoting Barnabás Pőcze (2026-08-27 12:41:01)
>> The frame duration limits from controls are already clamped, but that does
>> not prevent a situation where min > max, so use the just acquired min frame
>> duration as the lower bound when clamping the max value in order to avoid
>> this possibility.
> 
> It took me the next patch to understand why this makes sense. I had
> trouble understanding the commit message because the calculation of
> state.MinFrameDuration is not in the context. What about:
> 
> The frame duration limits from controls get clamped to the session
> limits, but that does not prevent a situation where min > max. Use the
> just computed session.minFrameDuration as the lower bound when clamping

You mean `state.minFrameDuration` ? I have adjusted the message with that.


> the max value in order to avoid this possibility.
> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> Best regards,
> Stefan
> 
>> ---
>>   src/ipa/libipa/agc.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index 2c1d2169f2..f9edd9295a 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -642,7 +642,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
>>   
>>                  state.maxFrameDuration = std::clamp<utils::Duration>(
>>                          std::chrono::microseconds((*frameDurationLimits).back()),
>> -                       session.minFrameDuration, session.maxFrameDuration);
>> +                       state.minFrameDuration, session.maxFrameDuration);
>>          }
>>          frameContext.minFrameDuration = state.minFrameDuration;
>>          frameContext.maxFrameDuration = state.maxFrameDuration;
>> -- 
>> 2.55.0
>>

Hi Barnabás

On Thu, Aug 27, 2026 at 03:13:16PM +0200, Stefan Klug wrote:
> Hi Barnabás,
>
> Quoting Barnabás Pőcze (2026-08-27 12:41:01)
> > The frame duration limits from controls are already clamped, but that does
> > not prevent a situation where min > max, so use the just acquired min frame
> > duration as the lower bound when clamping the max value in order to avoid
> > this possibility.
>
> It took me the next patch to understand why this makes sense. I had
> trouble understanding the commit message because the calculation of
> state.MinFrameDuration is not in the context. What about:
>
> The frame duration limits from controls get clamped to the session
> limits, but that does not prevent a situation where min > max. Use the
> just computed session.minFrameDuration as the lower bound when clamping
> the max value in order to avoid this possibility.

It's actually the just computed "state.minFrameDuration" not session

But I found the proposed commit message clearer

>
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

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

>
> Best regards,
> Stefan
>
> > ---
> >  src/ipa/libipa/agc.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> > index 2c1d2169f2..f9edd9295a 100644
> > --- a/src/ipa/libipa/agc.cpp
> > +++ b/src/ipa/libipa/agc.cpp
> > @@ -642,7 +642,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s
> >
> >                 state.maxFrameDuration = std::clamp<utils::Duration>(
> >                         std::chrono::microseconds((*frameDurationLimits).back()),
> > -                       session.minFrameDuration, session.maxFrameDuration);
> > +                       state.minFrameDuration, session.maxFrameDuration);
> >         }
> >         frameContext.minFrameDuration = state.minFrameDuration;
> >         frameContext.maxFrameDuration = state.maxFrameDuration;
> > --
> > 2.55.0
> >