[libcamera-devel] [PATCH v3 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 20 12:33:20 CEST 2022
Hi Umang
On Mon, Jun 20, 2022 at 02:10:22AM +0530, Umang Jain via libcamera-devel wrote:
> Extend the FCQueue::get() to return a IPAFrameContext for a
> non-existent frame. The .get() should be able to figure out if a
> existent frame context is asked for, or to create a new frame
> context and return its IPAFrameContext.
>
> We specifically want to have access to the FCQueue through the
> .get() public function hence the class FCQueue needs to be now
> privately inherited from std::vector<>.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> src/ipa/ipu3/ipa_context.cpp | 97 +++++++++++++++++++++++++++++++++---
> src/ipa/ipu3/ipa_context.h | 10 +++-
> src/ipa/ipu3/ipu3.cpp | 6 ++-
> 3 files changed, 103 insertions(+), 10 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 36d20b1b..60c58b97 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -222,15 +222,34 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> * \brief A FIFO circular queue holding IPAFrameContext(s)
> *
> * FCQueue holds all the IPAFrameContext(s) related to frames required
> - * to be processed by the IPA at a given point.
> + * to be processed by the IPA at a given point. An IPAFrameContext is created
> + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to
> + * FCQueue::get() with the same frame number shall return the IPAFrameContext
> + * previously created until the frame is marked as complete through
> + * FCQueue::completeFrame().
> + */
> +
> +/**
> + * \var FCQueue::oldestFC_
> + * \brief Frame number of the oldest IPAFrameContext in the FCQueue
> + */
> +
> +/**
> + * \var FCQueue::latestFC_
> + * \brief Frame number of the latest IPAFrameContext in the FCQueue
> + */
> +
Let's open the bikeshedding on names.
head and tail confused me at the beginning but I think they're more
appropriate. Other ideas ?
> +/**
> + * \var FCQueue::isFull_
> + * \brief Flag set when the FCQueue is full
> */
>
> /**
> * \brief FCQueue constructor
> */
> FCQueue::FCQueue()
> + : latestFC_(0), oldestFC_(0), isFull_(false)
> {
> - reset();
> }
>
> /**
> @@ -238,19 +257,70 @@ FCQueue::FCQueue()
> * \param[in] frame Frame number for which the IPAFrameContext needs to
> * retrieved
> *
> - * \return Pointer to the IPAFrameContext
> + * This function returns the IPAFrameContext for the desired frame. If the
> + * frame context does not exist in the queue, a new slot for the corresponding
> + * \a frame is created and returned. It is the responsibility of the caller
> + * to fill the correct IPAFrameContext parameters of newly returned
> + * IPAFrameContext.
> + *
> + * \return Pointer to the IPAFrameContext or nullptr if frame context couldn't
> + * be created
> */
> IPAFrameContext *FCQueue::get(uint32_t frame)
> {
> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> + if (latestFC_ && frame <= latestFC_) {
> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> + if (frame != frameContext.frame)
> + LOG(IPAIPU3, Warning)
> + << "Got wrong frame context for frame " << frame;
> +
> + return &frameContext;
> + } else {
You can save one indendation level by dropping the else
> + if (isFull_) {
You know, I would be tempted to turn this into a function
bool FCQueue::full(unsigned int frame)
{
isFull_ = (frame - oldestFC_) >= kMaxFrameContexts;
return isFull_;
}
Which does the right calculations and sets the internal flag (if you
need to expose a public isFull() method), so that you only have to
update the counters on get() and complete()
> + LOG(IPAIPU3, Warning)
I was about to suggest to use Error, as the caller should really check
the returned value and this could potentially cause a segfault, but
maybe warning is enough ?
> + << "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_ >= kMaxFrameContexts - 1)
Doesn't this work even with !oldestFC_, ie can you remove the first
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
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
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() ?
> + 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 ?
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.
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 ?
> +
> /**
> * \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