[2/2] ipa: rkisp1: agc: Set measurement window to full frame

22977 diff mbox series

With the availability of metering modes and the corresponding weights,
there is a flexible way of defining the area that gets taken into
account when AEGC is calculated. There is no need to reduce that window
to an arbitrary region anymore. If need arises we can make this
parameter user configurable or add a control for it.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Hi Stefan

On 19/03/2025 16:01, Stefan Klug wrote:
> With the availability of metering modes and the corresponding weights,
> there is a flexible way of defining the area that gets taken into
> account when AEGC is calculated. There is no need to reduce that window
> to an arbitrary region anymore. If need arises we can make this
> parameter user configurable or add a control for it.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>


Makes sense to me:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 101cad5d0893..b3ac9400b74f 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -193,14 +193,10 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>   	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
>   	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
>   
> -	/*
> -	 * Define the measurement window for AGC as a centered rectangle
> -	 * covering 3/4 of the image width and height.
> -	 */
> -	context.configuration.agc.measureWindow.h_offs = configInfo.outputSize.width / 8;
> -	context.configuration.agc.measureWindow.v_offs = configInfo.outputSize.height / 8;
> -	context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> -	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> +	context.configuration.agc.measureWindow.h_offs = 0;
> +	context.configuration.agc.measureWindow.v_offs = 0;
> +	context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> +	context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
>   
>   	setLimits(context.configuration.sensor.minExposureTime,
>   		  context.configuration.sensor.maxExposureTime,

Hi Kieran,

Thank you for the review. 

On Tue, Mar 25, 2025 at 10:19:54AM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-19 16:01:37)
> > With the availability of metering modes and the corresponding weights,
> > there is a flexible way of defining the area that gets taken into
> > account when AEGC is calculated. There is no need to reduce that window
> > to an arbitrary region anymore. If need arises we can make this
> > parameter user configurable or add a control for it.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 101cad5d0893..b3ac9400b74f 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -193,14 +193,10 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> >         context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> >  
> > -       /*
> > -        * Define the measurement window for AGC as a centered rectangle
> > -        * covering 3/4 of the image width and height.
> > -        */
> 
> Do the existing weights already match this? Where are the weights
> currently coming from for this ? Are they filled in from the tuning file
> generator ? or is there a default somewhere?

Yes, the weights come from the tuning file generator. This code here
dates back to 2022 8917c9e7ba64 ("ipa: rkisp1: agc: Add a
histogram-based gain") and I believe the intention was to add a bit of a
spot measurement. We should have changed it when the metering modes were
added. You are right, the behavior of existing cameras with existing
tuning files change a bit. So we should strive for updated tuning files
in the near future. We could add the default metering modes to all
existing tuning files in a follow up patch.

Cheers,
Stefan

> 
> > -       context.configuration.agc.measureWindow.h_offs = configInfo.outputSize.width / 8;
> > -       context.configuration.agc.measureWindow.v_offs = configInfo.outputSize.height / 8;
> > -       context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> > -       context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> > +       context.configuration.agc.measureWindow.h_offs = 0;
> > +       context.configuration.agc.measureWindow.v_offs = 0;
> > +       context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> > +       context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
> >  
> >         setLimits(context.configuration.sensor.minExposureTime,
> >                   context.configuration.sensor.maxExposureTime,
> > -- 
> > 2.43.0
> >