[libcamera-devel] [PATCH v3 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 20 18:49:27 CEST 2022
Hi Umang
On Mon, Jun 20, 2022 at 10:05:53PM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 6/20/22 21:42, Jacopo Mondi wrote:
> > 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 ?
>
>
> Once that's merged, I can drop it.
>
> >
> > > > 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.
>
>
> It's the best assumption I can work with right now. The code accordingly to
> me does the right thing given the assumption.
>
> I am working with assumption of "there are no frame drops" and you are
> reviewing this from "there are frame drops"
>
> Sure, we have two different angle of attack.
>
> Should I drop all this work and work on frame drops instead, Kieran what you
> do think? Should I start the discussion from day 1 of the Codecamp? Happy to
> help.
>
> >
> > 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.
>
>
> Correct.
>
> >
> > 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
>
>
> Cool, frame drops again ;-)
>
> >
> > 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
>
>
> btw, 46 % 16 == 14
The text got cut.
It was
46 % 16 == 30 % 16
>
> So 46th frame will get 14th slot.
>
> 45th is dropped, so it won't get consumed, yes, so completeFrame(45) will
> not be called
>
> After completeFrame (44) => oldestFC = 44, then directly
> completeFrame(46) => oldestFC_ = 46
>
> Hence the slot for 45th is never filled and never retrieved.
>
> Can it trigger isFull_ = true? Yes. an empty slot can trigger that until a
> completeFrame(46) is called upon.
>
> Should we handle it, yes? How ? Frame drop handling. Oops I am running in
> circles now ;-)
>
> >
> > > > 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.
>
>
> Any other opinions ?
>
> >
> > > > 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 ?
>
>
> The latter. Frame contexts can be created and pre-filled by the algorithms
> much earlier than the frame is actually queued up.
>
> >
> > > 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