[libcamera-devel] [PATCH v3 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 20 18:12:59 CEST 2022
Hi Umang,
On Mon, Jun 20, 2022 at 07:19:34PM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 6/20/22 16:03, Jacopo Mondi wrote:
> > Hi Umang
> >
[snip]
> > > + /* Check if the queue is full to avoid over-queueing later */
> > > + if (oldestFC_ && latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
> > Doesn't this work even with !oldestFC_, ie can you remove the first
>
>
> It has to do with the fact frame numbers doesn't start from '0' like in CTS
> (back to back test runs)
>
> So on every start() we call FCQueue::reset() and oldestFC_ gets assigned
> again to '0'
>
> When the "first" .get() happens, the incoming frame number maybe 244 so the
> check
>
> + if (latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
>
> will be true and hence isFull_ gets set on first frame itself.
>
> When the first frame (#244) gets completed, oldestFC_ will be set from 0 to
> 244 directly,
> so from there it will do the right thing
>
> So, for now we need the initial part of the check as well - until Kieran's
> patch makes sure
> every frame starts from '0' I believe.
>
I think we should operate under the assumption that every streaming
session (start/stop) re-init the frame and request sequences from 0.
It will simplify the work here greatly, and if I'm not mistaken is not
that far away ?
> > part of the condition. Also, I think you do >= kMaxFrameContexts - 1
> > to prepare the flag for the 'next' frame, however as we use the frame
>
>
> what's wrong with that? You set the last IPAFrameContext slot in the FCQueue
> - see whether FCQueue full - and set the flag there itself.
>
It's "wrong" the fact you evaluate the error condition at the
beginning and you updated it last assuming the next frame will be
sequential.
Let's look again at the code in full, as there are too many comments
in this thread :)
+IPAFrameContext *FCQueue::get(uint32_t frame)
+{
...
+ if (isFull_) {
+ LOG(IPAIPU3, Warning)
+ << "Cannot create frame context for frame " << frame
+ << " since FCQueue is full";
+
+ return nullptr;
+ }
+
+ latestFC_ = frame;
+
+ /* Check if the queue is full to avoid over-queueing later */
+ if (oldestFC_ && latestFC_ - oldestFC_ >= size_ - 1)
+ isFull_ = true;
+
+ return &this->at(latestFC_ % size_);
}
You're here as you're asked to create a new frame context. The error
condition is to return nullptr if there's no space to create a new
one.
Let's assume:
kMaxFrameContexts = 16
latestFC_ = 43
oldestFC_ = 30
isFull_ = false;
get(44):
isFull_ = false;
latestFC_ = 44;
44 - 30 < 15: isFull_ = false;
return queue[44 % 16];
frame 45 dropped
get(46):
isFull_ = false;
latestFC_ = 46;
46 - 30 >= 15: isFull_ = true;
return queue[46 % 16]; <--- THIS SLOT HAS NOT BEEN
CONSUMED YET. 46 % 16 == 30
> > id to identify the slot position and there's no guarantee that the
> > 'next' frame id will be + 1 (because of drops). This makes me think
>
>
> The gap / drop handling will probably halt this entire development.
>
> Yes, we need to handle drops and it will requiring FCQueue adjustments, but
> I cannot develop once we some sort of handling policy or
> discussion/mechanisms plan.
>
We debated it already, I know, but I think drops should be handled from the
beginning otherwise we'll have to implement this again. Happy to have
more opinions on this.
> > checking before with a function that takes the frame number to insert
> > is better.
> >
> > > + isFull_ = true;
> > >
> > > - if (frame != frameContext.frame) {
> > > + return &this->at(latestFC_ % kMaxFrameContexts);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * \brief Notifies the FCQueue that a frame has been completed
> > > + * \param[in] frame The completed frame number
> > > + */
> > > +void FCQueue::completeFrame(uint32_t frame)
> > > +{
> > > + if (oldestFC_ != frame)
> > > LOG(IPAIPU3, Warning)
> > > - << "Got wrong frame context for frame" << frame
> > > - << " or frame context doesn't exist yet";
> > > + << "Frame " << frame << " completed out-of-sync?";
> > > +
> > > + if (frame > latestFC_) {
> > This mean something is very wrong. Is it worth an ASSERT() ?
>
>
> could be upgraded.
>
> >
> > > + LOG(IPAIPU3, Error)
> > > + << "Completing a frame " << frame
> > > + << " not present in the queue is disallowed";
> > > + return;
> > > }
> > >
> > > - return &frameContext;
> > > + /**
> > > + * \todo If there are 'gaps' in FCQueue due to frame drops, the below
> > > + * oldestFC_ = frame + 1 will be be wrong.
> > > + * We need to set oldestFC_ to the next valid entry, jumping the drop
> > > + * gap slots.
> > > + */
> > > + oldestFC_ = frame + 1;
> > This looks like
> >
> >
> > FCQueue = [...] [frame] [gap] [frame+1] [...]
> >
> > gap is a leftover from a previous iteration so it will certainly have
> > an id < frame, right ? Can't we inspect the 'next' positions until we
> > don't find an id > frame ?
>
>
> That's what is being biting us here. We can't really know if the next slot
> is a valid frame context or not, or just a 'gap'
>
> If we have a mechanism to know if the next slot is valid IPAFrameContext or
> just a gap, I could have jumped the oldestFC_ to point to next "valid" one.
>
Such mechanism (if necessary at all, I think having slots initialized to 0
and check for next.frame > frame is enough) should be part of the
design of this class, otherwise it will have to be reimplemented on
top, leading possibly to a rewrite when we'll find out it's a real
issue.
> > This now rises the question what happens when we have an underrun, ie
> > the just completed frame is the latest one in the queue.
> >
> > I think you're current code doesn't handle that and sets oldestFC_
> > past latestFC_ unconditionally. Should we not advance oldestFC_ ? The
> > disance between latestFC_ and oldestFC_ will be 0, which I guess is
> > what we want.
>
>
> We need a discussion here.
>
> If algorithms keeps computing values for next frames, the slots will get
> filled up so the latestFC_ will be advancing I believe.
>
They will however ask for existing context, or will they keep creating
new ones if there are no requests from the application ?
> When application doesn't queue requests enough, we will face underrun so
> oldestFC_ will stop at a point will the last request is processed by the IPA
> (and the last call to completeFrame)
>
> So it might very well happen practically, that distance between latestFC_
> and oldestFC_ doesn't go to zero and the queue reports full.
>
> I think we need to differentiate between IPAFrameContexts (whether it was
> been created by a Algorithm's look-ahead .get() call or a queueRequest()
> call) to really handle advance corner-cases.
>
> I also think all this handling should go on top instead of trying to handle
> it in this series, when we hook up "look-ahead" computation of the
> Algorithms.
>
Possibly, I'm just a bit afraid doing this once we'll find issues it
will take more time to identify the issue correctly, fix it, redesign
etc than thinking about it right now. That's what I wanted a unit test
for, mostly.
> >
> > Could something like this work ?
> >
> > for (unsigned int i = 0; i < kMaxFrameContexts; ++i) {
> > const IPAFrameContext &next = this->at(frame + i % kMaxFrameContexts);
> >
> > if (next.frame > frame || next.frame == latestFC_) {
> > oldestFC_ = next.frame;
> > break;
> > }
> > }
> >
> > I wish we could unit-test this in detail to catch the corner cases.
> >
> > > +
> > > + if (isFull_)
> > > + isFull_ = false;
> > > }
> > >
> > > /**
> > > @@ -262,12 +332,23 @@ void FCQueue::setSize(uint32_t size)
> > > resize(size);
> > > }
> > >
> > > +/**
> > > + * \fn FCQueue::isFull()
> > > + * \brief Checks whether the frame context queue is full
> > > + * \return True of FCQueue is full, False otherwise
> > > + */
> > This is never used afaict. When do you envision users of the queue to
> > use this function ?
>
>
> I use it for debugging, knowing when the queue is full might be helpful.
>
> Not sure if I can give you a proper use case right now.
>
I would have kept it out if it has not users. But it's a small
function, so...
> >
> > > +
> > > /**
> > > * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
> > > */
> > > void FCQueue::reset()
> > > {
> > > clear();
> > > +
> > > + isFull_ = false;
> > > +
> > > + latestFC_ = 0;
> > > + oldestFC_ = 0;
> > > }
> > >
> > > } /* namespace ipa::ipu3 */
> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > index e487d34b..52a0b5e7 100644
> > > --- a/src/ipa/ipu3/ipa_context.h
> > > +++ b/src/ipa/ipu3/ipa_context.h
> > > @@ -89,15 +89,23 @@ struct IPAFrameContext {
> > > ControlList frameControls;
> > > };
> > >
> > > -class FCQueue : public std::vector<IPAFrameContext>
> > > +class FCQueue : private std::vector<IPAFrameContext>
> > > {
> > > public:
> > > FCQueue();
> > >
> > > + void completeFrame(uint32_t frame);
> > > void reset();
> > > void setSize(uint32_t size);
> > >
> > > IPAFrameContext *get(uint32_t frame);
> > > + bool isFull() { return isFull_; }
> > > +
> > > +private:
> > > + uint32_t latestFC_;
> > > + uint32_t oldestFC_;
> > > +
> > > + bool isFull_;
> > > };
> > >
> > > struct IPAContext {
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index d770c254..15070fa0 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -334,6 +334,8 @@ int IPAIPU3::init(const IPASettings &settings,
> > > */
> > > int IPAIPU3::start()
> > > {
> > > + context_.frameContexts.reset();
> > > +
> > > /*
> > > * Set the sensors V4L2 controls before the first frame to ensure that
> > > * we have an expected and known configuration from the start.
> > > @@ -605,6 +607,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > */
> > >
> > > metadataReady.emit(frame, ctrls);
> > > +
> > > + context_.frameContexts.completeFrame(frame);
> > > }
> > >
> > > /**
> > > @@ -618,7 +622,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > {
> > > /* \todo Start processing for 'frame' based on 'controls'. */
> > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > + *context_.frameContexts.get(frame) = { frame, controls };
> > > }
> > >
> > > /**
> > > --
> > > 2.31.1
> > >
More information about the libcamera-devel
mailing list