[libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use an unordered_map for the stream buffer list
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 18 15:17:43 CEST 2020
Hi Naush,
On 18/09/2020 14:00, Naushir Patuck wrote:
> 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:
Aha- that's what I'd missed, it *is* initialised ;-)
I'm sorry for the noise.
>
> 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?
Yup - I had completely missed that the id_ was being intialised by the
stream, and somehow had it in my head that it was using a default
initialiser.
Thanks,
This still stands,
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
So I'll let Niklas take this on from here!
--
Kieran
> 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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list