[PATCH 4/4] ipa: rkisp1: Initialize FrameContext.agc.meteringMode
Stefan Klug
stefan.klug at ideasonboard.com
Tue Oct 29 09:55:18 CET 2024
Hi Jacopo,
On Tue, Oct 29, 2024 at 08:49:26AM +0100, Jacopo Mondi wrote:
> Hi Stefan
>
> On Mon, Oct 28, 2024 at 07:12:57PM +0100, Stefan Klug wrote:
> > 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.
> >
>
> As the name suggests, per-frame settings are indeed stored in the
> frame context, but the active state keeps track of the algorithm's
> state. In example, the AGC algo for RkISP1 stores in the active state
> the auto/manual mode and the associated exposure time and gain.
>
> As I see it, and might be my partial understanding of algos, the
> FrameContext can be initialized using the active state values, then:
>
> - when alloc()-ed during a queueRequest values coming from the application
> ControlList will override both the FrameContext and the ActiveState as
> it happens today
>
> - when get() without an alloc (Request underrun) the values it has
> been initialized to will continue to be in use and the algorithms
> can keep operating without a user Request
>
> What I'm not sure about is, in case of alloc(), if we need the
> initialization or not, as the newly alloc()-ed FrameContext will be
> passed through Algo::queueRequest(), so values copied from the
> ActiveState will be overwritten anyway.
I wrote the following paragraph last. Maybe you should read the rest
first :-)
What about renaming Algorithm::queueRequest to
Algorithm::initFrameContext which gets called once in both code paths
(Either after/in alloc() or after/in get() without prior alloc()) For
the get() without alloc we just pass an empty ControlsList.
>
> > >
> > > > 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
>
> That shouldn't happen. What have I missed ?
>
> In my understanding, following your example, we have
>
> FCQ:
> frame#0 frame#1
> +--------+--------+
> |auto = f|auto = t|
> |blue = 4| |
> +--------+--------+
>
> They both get alloc()-ed and passed through Awb::queueRequest() in
> IPA::queueRequest(0) and IPA::queueRequest(1)
>
> The call to Awb::queueRequest(1) will
>
> void Awb::queueRequest(IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> IPAFrameContext &frameContext,
> const ControlList &controls)
> {
> auto &awb = context.activeState.awb;
>
> const auto &awbEnable = controls.get(controls::AwbEnable);
> if (awbEnable && *awbEnable != awb.autoEnabled) {
> awb.autoEnabled = *awbEnable;
>
> LOG(RkISP1Awb, Debug)
> << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> }
>
> ...
>
> }
>
> So now ActiveContext contains
>
> awb.autoEnabled = true
>
> Now a new frame gets produced, and no Request has been queued, so we get(2)
> without a preceding alloc(2).
>
> We call get(2) and hence we target FCQ[0] which has already been alloc()
> to store frame#0 settings
>
> FrameContext &get(uint32_t frame, const ActiveState &activeState)
> {
> FrameContext &frameContext = contexts_[frame % contexts_.size()];
>
> if (frame < frameContext.frame)
> /*
> * We don't get here, frame = 2,
> * frameContext.frame = 0
> */
>
>
> if (frame == 0 && !frameContext.initialised)
> /* We don't get here, frame = 2 */
>
> if (frame == frameContext.frame)
> /*
> * We don't get here, frame = 2,
> * frameContext.frame = 0
> */
>
> /*
> * So we get here, where we WARN and re-initialize the
> * frame context
> */
> LOG(FCQueue, Warning)
> << "Obtained an uninitialised FrameContext for " << frame;
>
> frameContext.init(frame, activeState);
>
> return frameContext;
> }
>
> The frameContext is re-initialized with the current activeState values
> so with autoEnabled=true and passed to process().
Here is the catch (or the piece I can't see). Who does the assignment
frameContext.awb.autoEnable = activeState.awb.autoEnable?
It has to be inside the frameContext.init() function. Isn't that the
place where we started off with the discussion? That we could either
hardcode it inside IPAFrameContext::init() if the frame context has
enough information on what to initialize from activeState or forward
that to the corresponding algorithm and implement a
Algorithm::initFrameContext(fc, activeState).
>
> Now, how do we recover from a Request underrun is what we have to
> design, but assuming no futher Request gets queued, I don't see the
> algo oscillating between auto and manual.gain.blue = 4.
>
> What am I missing ?
I think we are on the same page. The only difference I see is that I
tend to move things into the agorithms to keep all that logic in one
place and I believe you see the algorithms more as a pluggable formula
which operate on the state that is kept (and known) in frame context and
therefore frame context is able to initialize it.
>
> > 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...
>
> Maybe I am.
>
> The above is my interpretation of how thing should work. If we agree
> but you see a different behaviour, then we might have a bug somewhere
> (which I'm not seeing).
I think we agree in the overall picture. Maybe one approach to solve
this is to further fill the FrameContext::init() to see how much
algorithm specific knowledge we need there. This boils down to copying
all assignments to frameContext from the Algorihm::queueRequest()
functions into the FrameContext::init() or try to rename
Algorithm::queueRequest as proposed above.
Best regards,
Stefan
>
> >
> > Cheers,
> > Stefan
>
> Thanks!
>
> >
> > >
> > > >
> > > > I hope it is somehow understandable.
> > >
> > > Thank you!
> > > j
> > >
> > > >
> > > > Cheers,
> > > > Stefan
> > > >
> > > > > }
> > > > >
> > > > > /**
> > > > > --
> > > > > 2.47.0
> > > > >
More information about the libcamera-devel
mailing list