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

[Jacopo Mondi]

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.

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

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

>
> I hope it is somehow understandable.

Thank you!
  j

>
> Cheers,
> Stefan
>
> >  }
> >
> >  /**
> > --
> > 2.47.0
> >