[PATCH 4/4] ipa: rkisp1: Initialize FrameContext.agc.meteringMode
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 29 08:49:26 CET 2024
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.
> >
> > > 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().
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 ?
> 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).
>
> 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