[libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use an unordered_map for the stream buffer list
Naushir Patuck
naush at raspberrypi.com
Fri Sep 18 15:00:52 CEST 2020
Hi Kieran,
Thank you for your review.
On Fri, 18 Sep 2020 at 13:53, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 18/09/2020 10:42, Naushir Patuck wrote:
> > By using a map container, we can easily insert/remove buffers from the
> > buffer list. This will be required to track buffers allocated externally
> > and passed to the pipeline handler through a Request.
> >
> > Replace the buffer index tracking with an id generated internally by the
> > stream object.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------
> > .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++-----
> > .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++--
> > 3 files changed, 82 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 7d91188c..51544233 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > {
> > RPiCameraData *data = cameraData(camera);
> > - unsigned int index;
> > int ret;
> >
> > /*
> > @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > * This will allow us to identify buffers passed between the pipeline
> > * handler and the IPA.
> > */
> > - index = 0;
> > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,
> > - .planes = b->planes() });
> > - index++;
> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first,
> > + .planes = b.second->planes() });
> > }
> >
> > - index = 0;
> > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> > - .planes = b->planes() });
> > - index++;
> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first,
> > + .planes = b.second->planes() });
> > }
> >
> > data->ipa_->mapBuffers(data->ipaBuffers_);
> > @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > return;
> >
> > for (RPi::RPiStream &s : unicam_) {
> > - index = s.getBufferIndex(buffer);
> > + index = s.getBufferId(buffer);
> > if (index != -1) {
> > stream = &s;
> > break;
> > @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> > return;
> >
> > LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> > - << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer)
> > + << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer)
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > /* The ISP input buffer gets re-queued into Unicam. */
> > @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > return;
> >
> > for (RPi::RPiStream &s : isp_) {
> > - index = s.getBufferIndex(buffer);
> > + index = s.getBufferId(buffer);
> > if (index != -1) {
> > stream = &s;
> > break;
> > @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline()
> > /* Set our state to say the pipeline is active. */
> > state_ = State::Busy;
> >
> > - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);
> > - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);
> > + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);
> > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> >
> > LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:"
> > - << " Bayer buffer id: " << bayerIndex
> > - << " Embedded buffer id: " << embeddedIndex;
> > + << " Bayer buffer id: " << bayerId
> > + << " Embedded buffer id: " << embeddedId;
> >
> > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > - RPiIpaMask::BAYER_DATA | bayerIndex };
> > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId,
> > + RPiIpaMask::BAYER_DATA | bayerId };
> > ipa_->processEvent(op);
> > }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 80170d64..aee0aa2d 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const
> >
> > void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > {
> > - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> > - [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> > + for (auto const &buffer : *buffers)
> > + bufferMap_.emplace(id_.get(), buffer.get());
> > }
> >
> > -const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> > +const BufferMap &RPiStream::getBuffers() const
> > {
> > - return bufferList_;
> > + return bufferMap_;
> > }
> >
> > -int RPiStream::getBufferIndex(FrameBuffer *buffer) const
> > +int RPiStream::getBufferId(FrameBuffer *buffer) const
> > {
> > if (importOnly_)
> > return -1;
> >
> > - /* Find the buffer in the list, and return the index position. */
> > - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);
> > - if (it != bufferList_.end())
> > - return std::distance(bufferList_.begin(), it);
> > + /* Find the buffer in the map, and return the buffer id. */
> > + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),
> > + [&buffer](auto const &p) { return p.second == buffer; });
> >
> > - return -1;
> > + if (it == bufferMap_.end())
> > + return -1;
> > +
> > + return it->first;
> > }
> >
> > int RPiStream::prepareBuffers(unsigned int count)
> > @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count)
> > }
> >
> > /* We must import all internal/external exported buffers. */
> > - count = bufferList_.size();
> > + count = bufferMap_.size();
> > }
> >
> > return dev_->importBuffers(count);
> > @@ -139,6 +141,9 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
> > /* Push this buffer back into the queue to be used again. */
> > availableBuffers_.push(buffer);
> >
> > + /* Allow the buffer id to be reused. */
> > + id_.release(getBufferId(buffer));
> > +
> > /*
> > * Do we have any Request buffers that are waiting to be queued?
> > * If so, do it now as availableBuffers_ will not be empty.
> > @@ -196,12 +201,13 @@ void RPiStream::clearBuffers()
> > availableBuffers_ = std::queue<FrameBuffer *>{};
> > requestBuffers_ = std::queue<FrameBuffer *>{};
> > internalBuffers_.clear();
> > - bufferList_.clear();
> > + bufferMap_.clear();
> > + id_.reset();
> > }
> >
> > int RPiStream::queueToDevice(FrameBuffer *buffer)
> > {
> > - LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer)
> > + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer)
> > << " for " << name_;
> >
> > int ret = dev_->queueBuffer(buffer);
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > index 959e9147..df986367 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -9,8 +9,10 @@
> >
> > #include <queue>
> > #include <string>
> > +#include <unordered_map>
> > #include <vector>
> >
> > +#include <libcamera/ipa/raspberrypi.h>
> > #include <libcamera/stream.h>
> >
> > #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -19,6 +21,8 @@ namespace libcamera {
> >
> > namespace RPi {
> >
> > +using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>;
> > +
> > /*
> > * Device stream abstraction for either an internal or external stream.
> > * Used for both Unicam and the ISP.
> > @@ -27,12 +31,13 @@ class RPiStream : public Stream
> > {
> > public:
> > RPiStream()
> > + : id_(RPiIpaMask::ID)
> > {
> > }
> >
> > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> > : external_(false), importOnly_(importOnly), name_(name),
> > - dev_(std::make_unique<V4L2VideoDevice>(dev))
> > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiIpaMask::ID)
> > {
> > }
> >
> > @@ -45,8 +50,8 @@ public:
> > bool isExternal() const;
> >
> > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > - const std::vector<FrameBuffer *> &getBuffers() const;
> > - int getBufferIndex(FrameBuffer *buffer) const;
> > + const BufferMap &getBuffers() const;
> > + int getBufferId(FrameBuffer *buffer) const;
> >
> > int prepareBuffers(unsigned int count);
> > int queueBuffer(FrameBuffer *buffer);
> > @@ -56,6 +61,44 @@ public:
> > void releaseBuffers();
> >
> > private:
> > + class IdGenerator
> > + {
> > + public:
> > + IdGenerator(int max)
>
> Should this provide a default value for max?
>
> Perhaps int max = 32?
>
> (See below).
>
> > + : max_(max), id_(0)
> > + {
> > + }
> > +
> > + int get()
> > + {
> > + int id;
> > + if (!recycle_.empty()) {
> > + id = recycle_.front();
> > + recycle_.pop();
> > + } else {
> > + id = id_++;
> > + ASSERT(id_ <= max_);
> > + }
> > + return id;
> > + }
> > +
> > + void release(int id)
> > + {
> > + recycle_.push(id);
> > + }
> > +
> > + void reset()
> > + {
> > + id_ = 0;
> > + recycle_ = {};
> > + }
> > +
> > + private:
> > + int max_;
> > + int id_;
> > + std::queue<int> recycle_;
> > + };
> > +
> > void clearBuffers();
> > int queueToDevice(FrameBuffer *buffer);
> >
> > @@ -74,8 +117,11 @@ private:
> > /* The actual device stream. */
> > std::unique_ptr<V4L2VideoDevice> dev_;
> >
> > + /* Tracks a unique id key for the bufferMap_ */
> > + IdGenerator id_;
>
> The only constructor for IdGenerator takes an int 'max' value, to assign
> to max_ which has the assertion check.
>
> That sounds useful to me for validation, but I can't see how this works
> (I'm probably missing something).
>
> With no default parameter, and nothing declared in the class definition
> as a default value, what is max_ set to ?
> (Some how I assume it would be zero?, but I would expect the compiler to
> complain too)
Unless I misunderstood your comment above, the IdGenerator class is
initialised in the RPiStream contstructors:
RPiStream()
: id_(RPiBufferMask::ID)
{
}
RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
: external_(false), importOnly_(importOnly), name_(name),
dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPiBufferMask::ID)
{
}
IdGenerator::max_ takes the value RPiBufferMask::ID (0xffff) in this
case. Given no default value is used, the compiler ought to shout
loudly if I were to construct an IdGenerator instance without a max
parameter. Does that answer your query?
Regards,
Naush
>
> Actually, I think what probably happens is it uses a 'default
> constructor' and leaves the id_ and max_ values uninitialised, so it's
> probably works by chance ?
>
> Other than that, I think this is neat. A quick and fast solution to the
> problem, and we never expect recycle queue to grow beyond the maximum
> quantity of unique buffers running in the system at any one time.
>
> With the max_ issue cleared:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> There shouldn't be any need to resend the whole series for this.
> If it does need fixing, either send an 8.1 of just this patch, or we can
> fix while applying perhaps, if it's just providing the default value.
> --
> Kieran
>
>
>
>
>
> > +
> > /* All frame buffers associated with this device stream. */
> > - std::vector<FrameBuffer *> bufferList_;
> > + BufferMap bufferMap_;
> >
> > /*
> > * List of frame buffers that we can use if none have been provided by
> >
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list