@@ -76,7 +76,7 @@ struct IPAActiveState {
} toneMapping;
};
-struct IPAFrameContext : public FrameContext {
+struct IPAFrameContext {
struct {
uint32_t exposure;
double gain;
@@ -20,28 +20,10 @@ namespace ipa {
* \brief Queue of per-frame contexts
*/
-/**
- * \struct FrameContext
- * \brief Context for a frame
- *
- * The frame context stores data specific to a single frame processed by the
- * IPA module. Each frame processed by the IPA module has a context associated
- * with it, accessible through the Frame Context Queue.
- *
- * Fields in the frame context should reflect values and controls associated
- * with the specific frame as requested by the application, and as configured by
- * the hardware. Fields can be read by algorithms to determine if they should
- * update any specific action for this frame, and finally to update the metadata
- * control lists when the frame is fully completed.
- *
- * \var FrameContext::frame
- * \brief The frame number
- */
-
/**
* \class FCQueue
* \brief A support class for managing FrameContext instances in IPA modules
- * \tparam FrameContext The IPA module-specific FrameContext derived class type
+ * \tparam FrameContext The IPA module-specific frame context type
*
* Along with the Module and Algorithm classes, the frame context queue is a
* core component of the libipa infrastructure. It stores per-frame contexts
@@ -83,16 +65,12 @@ namespace ipa {
* allowed to overflow, which must be ensured by pipeline handlers never
* queuing more in-flight requests to the IPA module than the queue size. If an
* overflow condition is detected, the queue will log a fatal error.
- *
- * IPA module-specific frame context implementations shall inherit from the
- * FrameContext base class to support the minimum required features for a
- * FrameContext.
*/
/**
- * \fn FCQueue::FCQueue(unsigned int size)
+ * \fn FCQueue::FCQueue(std::size_t capacity)
* \brief Construct a frame contexts queue of a specified size
- * \param[in] size The number of contexts in the queue
+ * \param[in] capacity The number of contexts in the queue
*/
/**
@@ -116,7 +94,8 @@ namespace ipa {
* initialised already, and returned to the caller.
*
* If the FrameContext was already initialized for this \a frame, a warning will
- * be reported and the previously initialized FrameContext is returned.
+ * be reported and the previously initialized FrameContext is returned. Otherwise,
+ * \a frame must be greater than the last allocated frame number.
*
* Frame contexts are expected to be initialised when a Request is first passed
* to the IPA module in IPAModule::queueRequest().
@@ -129,8 +108,8 @@ namespace ipa {
* \brief Obtain the FrameContext for the \a frame
* \param[in] frame The frame context sequence number
*
- * If the FrameContext is not correctly initialised for the \a frame, it will be
- * initialised.
+ * If the FrameContext is not correctly initialised for the \a frame, initialisation
+ * will be tried as if by calling \a alloc(frame). Note, that this may fail, and abort.
*
* \return A reference to the FrameContext for sequence \a frame
*/
@@ -7,8 +7,11 @@
#pragma once
+#include <algorithm>
+#include <memory>
+#include <optional>
#include <stdint.h>
-#include <vector>
+#include <type_traits>
#include <libcamera/base/log.h>
@@ -18,118 +21,129 @@ LOG_DECLARE_CATEGORY(FCQueue)
namespace ipa {
-template<typename FrameContext>
-class FCQueue;
-
-struct FrameContext {
-private:
- template<typename T> friend class FCQueue;
- uint32_t frame;
- bool initialised = false;
-};
-
-template<typename FrameContext>
+template<typename T>
class FCQueue
{
+ static_assert(std::is_default_constructible_v<T>);
+
public:
- FCQueue(unsigned int size)
- : contexts_(size)
+ FCQueue(std::size_t capacity)
+ : entries_(std::make_unique<std::optional<Entry>[]>(capacity)),
+ capacity_(capacity)
{
+ ASSERT(capacity > 0);
}
void clear()
{
- for (FrameContext &ctx : contexts_) {
- ctx.initialised = false;
- ctx.frame = 0;
- }
+ next_ = 0;
+ lastFrame_.reset();
+
+ for (size_t i = 0; i < capacity_; i++)
+ entries_[i].reset();
}
- FrameContext &alloc(const uint32_t frame)
+ T &get(uint32_t frame)
{
- FrameContext &frameContext = contexts_[frame % contexts_.size()];
+ LOG(FCQueue, Debug) << "get(" << frame << ")";
- /*
- * Do not re-initialise if a get() call has already fetched this
- * frame context to preseve the context.
- *
- * \todo If the the sequence number of the context to initialise
- * is smaller than the sequence number of the queue slot to use,
- * it means that we had a serious request underrun and more
- * frames than the queue size has been produced since the last
- * time the application has queued a request. Does this deserve
- * an error condition ?
- */
- if (frame != 0 && frame <= frameContext.frame)
- LOG(FCQueue, Warning)
- << "Frame " << frame << " already initialised";
- else
- init(frameContext, frame);
+ if (auto *d = find(frame))
+ return *d;
+
+ LOG(FCQueue, Warning)
+ << "Frame " << frame << " not found, trying to allocate";
- return frameContext;
+ return allocNext(frame);
}
- FrameContext &get(uint32_t frame)
+ T &alloc(uint32_t frame)
{
- FrameContext &frameContext = contexts_[frame % contexts_.size()];
+ LOG(FCQueue, Debug) << "alloc(" << frame << ")";
+
+ if (frame <= lastFrame_) {
+ if (auto *d = find(frame)) {
+ LOG(FCQueue, Warning)
+ << "Frame " << frame << " already initialised";
+ return *d;
+ }
+ }
- /*
- * If the IPA algorithms try to access a frame context slot which
- * has been already overwritten by a newer context, it means the
- * frame context queue has overflowed and the desired context
- * has been forever lost. The pipeline handler shall avoid
- * queueing more requests to the IPA than the frame context
- * queue size.
- */
- if (frame < frameContext.frame)
- LOG(FCQueue, Fatal) << "Frame context for " << frame
- << " has been overwritten by "
- << frameContext.frame;
+ return allocNext(frame);
+ }
- if (frame == 0 && !frameContext.initialised) {
- /*
- * If the IPA calls get() at start() time it will get an
- * un-intialized FrameContext as the below "frame ==
- * frameContext.frame" check will return success because
- * FrameContexts are zeroed at creation time.
- *
- * Make sure the FrameContext gets initialised if get()
- * is called before alloc() by the IPA for frame#0.
- */
- init(frameContext, frame);
+private:
+ LIBCAMERA_DISABLE_COPY_AND_MOVE(FCQueue)
- return frameContext;
+ struct Entry {
+ uint32_t frame;
+ T data;
+
+ Entry(uint32_t f)
+ : frame(f), data()
+ {
}
+ };
+
+ T *find(uint32_t frame)
+ {
+ const auto findInRange = [&](auto first, auto last) -> T * {
+ auto it = std::partition_point(first, last, [&](const auto &e) {
+ ASSERT(e);
+ return e->frame < frame;
+ });
+ if (it != last && (*it)->frame == frame)
+ return &(*it)->data;
+
+ return nullptr;
+ };
- if (frame == frameContext.frame)
- return frameContext;
+ const auto first = entries_.get();
+ const auto mid = first + next_;
+ const auto last = first + capacity_;
/*
- * The frame context has been retrieved before it was
- * initialised through the initialise() call. This indicates an
- * algorithm attempted to access a Frame context before it was
- * queued to the IPA. Controls applied for this request may be
- * left unhandled.
- *
- * \todo Set an error flag for per-frame control errors.
+ * Search the more recent half: [0; next),
+ * the optionals in this range must always be non-empty.
*/
- LOG(FCQueue, Warning)
- << "Obtained an uninitialised FrameContext for " << frame;
+ if (auto *d = findInRange(first, mid))
+ return d;
- init(frameContext, frame);
+ /* Search the less recent half: [next_; capacity_) */
+ if (mid != last && mid->has_value()) {
+ /*
+ * If `next_` has wrapped around at least once, then all the optionals
+ * in [next_; capacity_) are non-empty. So if `*mid` is not empty,
+ * then all of them should be non-empty.
+ */
+ if (auto *d = findInRange(mid, last))
+ return d;
+ }
- return frameContext;
+ return nullptr;
}
-private:
- void init(FrameContext &frameContext, const uint32_t frame)
+ T &allocNext(uint32_t frame)
{
- frameContext = {};
- frameContext.frame = frame;
- frameContext.initialised = true;
+ if (!(lastFrame_ < frame)) {
+ LOG(FCQueue, Fatal)
+ << "Tried to allocate frame context for frame " << frame
+ << " after having already allocated one for a later frame "
+ << *lastFrame_;
+ }
+
+ auto &e = entries_[next_].emplace(frame);
+ LOG(FCQueue, Debug) << "frame " << frame << " slot " << next_;
+
+ next_ = (next_ + 1) % capacity_;
+ lastFrame_ = frame;
+
+ return e.data;
}
- std::vector<FrameContext> contexts_;
+ std::unique_ptr<std::optional<Entry>[]> entries_;
+ std::size_t capacity_;
+ std::size_t next_ = 0;
+ std::optional<uint32_t> lastFrame_;
};
} /* namespace ipa */
@@ -60,7 +60,7 @@ struct IPAActiveState {
} awb;
};
-struct IPAFrameContext : public FrameContext {
+struct IPAFrameContext {
struct {
uint32_t exposure;
double sensorGain;
@@ -153,7 +153,7 @@ struct IPAActiveState {
} lsc;
};
-struct IPAFrameContext : public FrameContext {
+struct IPAFrameContext {
struct {
uint32_t exposure;
double gain;
@@ -63,7 +63,7 @@ struct IPAActiveState {
} knobs;
};
-struct IPAFrameContext : public FrameContext {
+struct IPAFrameContext {
Matrix<float, 3, 3> ccm;
struct {
At the moment non-contiguous frame numbers are not really supported. They may cause assertion failures. Consider the situation where an `FCQueue` with a capacity of 32 is created, and then `alloc(45)` and `alloc(77)` are called. In this case, since 45 == 77 (mod 32), the same slot (13) is reused for both. A subsequent `get(45)` call would then log a fatal error as it detects that the slot has been overwritten. This issue affects the rkisp1 pipeline handler in particular as it does not use the request sequence number but instead manages a separate counter, which can have jumps when adjusted in `PipelineHandlerRkISP1::statBufferReady()`. It is easily visible in non-optimized sanitized builds with significant amount of log output. Other pipeline handlers employing `FCQueue` use the request sequence number, which is always incremented by 1, with no jumps. Address this by storing the sequence number next to the data, and using binary search instead of direct indexing. As a consequence of this, `get(x)` can no longer initialize the item in every case if `alloc(x)` has not been called, thus it has to abort to provide the same interface. The `FrameContext` type is also removed, no inheritance is necessary anymore. Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/250 Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/ipu3/ipa_context.h | 2 +- src/ipa/libipa/fc_queue.cpp | 35 ++----- src/ipa/libipa/fc_queue.h | 178 ++++++++++++++++++--------------- src/ipa/mali-c55/ipa_context.h | 2 +- src/ipa/rkisp1/ipa_context.h | 2 +- src/ipa/simple/ipa_context.h | 2 +- 6 files changed, 107 insertions(+), 114 deletions(-)