[PATCH 4/4] ipa: rkisp1: Initialize FrameContext.agc.meteringMode

[Stefan Klug]

Hi Jacopo,

On Mon, Oct 28, 2024 at 06:14:42PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Oct 28, 2024 at 05:47:28PM +0100, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:
> > > The RkISP1 AGC algorithms assumes the metering mode to be
> > > "MeteringMatrix" and pre-fill an array of weights associated
> > > with it stored in meteringModes_[MeteringMatrix] when intializing
> > > the algorithm in parseMeteringModes().
> > >
> > > It laters fetches the weights when computing parameters using the
> > > FrameContext.agc.meteringMode as index of the meteringModes_ array.
> > >
> > > When a Request gets queued to the algorithm, the
> > > FrameContext.agc.meteringMode index is populated from the algorithm's
> > > active state, and optionally updated if the application has specified
> > > a different metering mode.
> > >
> > > In case of Request underrun however, a non-initialized FrameContext
> > > gets provided to the algorithm, causing an (unhandled) exception when
> > > accessing the meteringModes_ array.
> > >
> > > Fix this by intializing the AGC metering mode to a supported value
> > > coming from the ActiveState in IPAFrameContext::init().
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 2 ++
> > >  src/ipa/rkisp1/ipa_context.cpp    | 5 +++++
> > >  2 files changed, 7 insertions(+)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 17d074d9c03e..dd7e9468741e 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >  		static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > >  	context.activeState.agc.exposureMode =
> > >  		static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first);
> > > +
> > > +	/* Use the metering matrix mode by default. */
> > >  	context.activeState.agc.meteringMode =
> > >  		static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first);
> > >
> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > index 2dad42b3154f..e88609137345 100644
> > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,
> > >  			   const ActiveState &activeState)
> > >  {
> > >  	FrameContext::init(frameNum, activeState);
> > > +
> > > +	const IPAActiveState *rkisp1ActiveState =
> > > +		reinterpret_cast<const IPAActiveState *>(&activeState);
> > > +
> > > +	agc.meteringMode = rkisp1ActiveState->agc.meteringMode;
> >
> > I don't particularly like that the IPAFrameContext needs that knowledge
> > which should be internal to the agc algorithm. We could add a
> 
> You know, my feeling was the opposite. Algorithms should make less
> assumptions about their internals implementation details but rely on a
> more globally shared state, kept in the FrameContext and the
> ActiveState. This would make easy to "swap" algoroithms.
> 
> After all, this already shows the AGC algo depends on a precise
> ordering of operations which if not respected results in a run-time
> exception.
> 
> > initContext() function to the algorithm base class. But then we need to
> > loop over all algorithms here, which doesn't feel efficient for a single
> > case. But on the other hand if we assume that request underruns become
> 
> I considered that but it seemed rather non-efficient.

Agreed, but I'm not sure if we can completely avoid it. Maybe with some
more restructuring...

> 
> > more common, then the other algorithms need to be able to continue with
> > current data without a prior queueRequest() call. While writing this,
> 
> Isn't the "current data" exactly the active state ?

The active state is the current measuered/stats data. But the current manual
(user supplied) data is stored in the frame context.

> 
> > yes I think we should forward that to every algorithm. Otherwise we
> > might end up with strange effects because the algorithms use the data
> > from the frame context that was reused without proper initialization.
> 
> I might be missing how a FrameContext could be "reused" and "without
> proper initialization". I understand a FrameContext might not
> initialized, or not correctly re-initalized but if that happens is an
> implementation issue, isn't it ?

Let's see if I can get this straight. For reference I take the awb algo.
Let's assume the following gets queued:

Frame 1: awb.autoEnable = false, gain.blue = 4
Frame 2: awb.autoEnable = true
... no more requests get queued ...

Let's further assume the FCQueue has a length of 2. Now no more requests
get queued. But process() still get's called because the internal
machinery is now ticked by the sensor producing images. That results in
a cyclic reuse of the two frame comtexts. Now the blue gain gets
constantly toggled between 4 and the automatic value. As the stats
calculation depends on the gains there will be unexpected effects.

I expect there are more cases like this and I'm not sure if we should
handle them inside FrameContext::init()

Maybe I'm missing something obvious...

Cheers,
Stefan

> 
> >
> > I hope it is somehow understandable.
> 
> Thank you!
>   j
> 
> >
> > Cheers,
> > Stefan
> >
> > >  }
> > >
> > >  /**
> > > --
> > > 2.47.0
> > >