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

[Kieran Bingham]

Quoting Stefan Klug (2025-03-25 13:57:26)
> 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 at 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.

That's fine with me:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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