[libcamera-devel] [PATCH v3 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 20 18:35:53 CEST 2022
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
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