Message ID | 20230721093759.27700-5-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Naush On Fri, Jul 21, 2023 at 10:37:59AM +0100, Naushir Patuck via libcamera-devel wrote: > Replace the buffer id generation in RPi::Stream with a simple integer > counter since ids don't get recycled any more. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/rpi/common/rpi_stream.cpp | 9 ++-- > .../pipeline/rpi/common/rpi_stream.h | 42 +------------------ > 2 files changed, 5 insertions(+), 46 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index e24531e171c8..d32163d3fc0f 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -53,7 +53,7 @@ void Stream::resetBuffers() > void Stream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > for (auto const &buffer : *buffers) > - bufferMap_.emplace(id_.get(), buffer.get()); > + bufferMap_.emplace(++id_, buffer.get()); This make id_ start from 1. Is 0 a special reserved value ? > } > > const BufferMap &Stream::getBuffers() const > @@ -78,7 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const > > void Stream::setExportedBuffer(FrameBuffer *buffer) > { > - bufferMap_.emplace(id_.get(), buffer); > + bufferMap_.emplace(++id_, buffer); > } > > int Stream::prepareBuffers(unsigned int count) > @@ -149,9 +149,6 @@ void Stream::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. > @@ -210,7 +207,7 @@ void Stream::clearBuffers() > requestBuffers_ = std::queue<FrameBuffer *>{}; > internalBuffers_.clear(); > bufferMap_.clear(); > - id_.reset(); > + id_ = 0; > } > > int Stream::queueToDevice(FrameBuffer *buffer) > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index d1289c4679b9..cc5d3f4afad4 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -60,7 +60,7 @@ public: > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > : flags_(flags), name_(name), > - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID) > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0) Should you change the other constructor too ? Stream() : flags_(StreamFlag::None), id_(BufferMask::MaskID) { } Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) : flags_(flags), name_(name), dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0) { } It also makes me wonder if you still need the BufferMask enum.. > { > } > > @@ -86,44 +86,6 @@ public: > void releaseBuffers(); > > private: > - class IdGenerator > - { > - public: > - IdGenerator(unsigned int max) > - : max_(max), id_(0) > - { > - } > - > - unsigned int get() > - { > - unsigned int id; > - if (!recycle_.empty()) { > - id = recycle_.front(); > - recycle_.pop(); > - } else { > - id = ++id_; > - ASSERT(id_ <= max_); > - } > - return id; > - } > - > - void release(unsigned int id) > - { > - recycle_.push(id); > - } > - > - void reset() > - { > - id_ = 0; > - recycle_ = {}; > - } > - > - private: > - unsigned int max_; > - unsigned int id_; > - std::queue<unsigned int> recycle_; > - }; > - > void clearBuffers(); > int queueToDevice(FrameBuffer *buffer); > > @@ -136,7 +98,7 @@ private: > std::unique_ptr<V4L2VideoDevice> dev_; > > /* Tracks a unique id key for the bufferMap_ */ > - IdGenerator id_; > + unsigned int id_; > > /* All frame buffers associated with this device stream. */ > BufferMap bufferMap_; > -- > 2.34.1 >
Hi Jacopo, Thank you for your review! On Mon, 24 Jul 2023 at 08:50, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Jul 21, 2023 at 10:37:59AM +0100, Naushir Patuck via libcamera-devel wrote: > > Replace the buffer id generation in RPi::Stream with a simple integer > > counter since ids don't get recycled any more. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/rpi/common/rpi_stream.cpp | 9 ++-- > > .../pipeline/rpi/common/rpi_stream.h | 42 +------------------ > > 2 files changed, 5 insertions(+), 46 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > index e24531e171c8..d32163d3fc0f 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > @@ -53,7 +53,7 @@ void Stream::resetBuffers() > > void Stream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > for (auto const &buffer : *buffers) > > - bufferMap_.emplace(id_.get(), buffer.get()); > > + bufferMap_.emplace(++id_, buffer.get()); > > This make id_ start from 1. Is 0 a special reserved value ? The 0 value is reserved for invalid id, so id_ does start from 1. > > > } > > > > const BufferMap &Stream::getBuffers() const > > @@ -78,7 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const > > > > void Stream::setExportedBuffer(FrameBuffer *buffer) > > { > > - bufferMap_.emplace(id_.get(), buffer); > > + bufferMap_.emplace(++id_, buffer); > > } > > > > int Stream::prepareBuffers(unsigned int count) > > @@ -149,9 +149,6 @@ void Stream::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. > > @@ -210,7 +207,7 @@ void Stream::clearBuffers() > > requestBuffers_ = std::queue<FrameBuffer *>{}; > > internalBuffers_.clear(); > > bufferMap_.clear(); > > - id_.reset(); > > + id_ = 0; > > } > > > > int Stream::queueToDevice(FrameBuffer *buffer) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > index d1289c4679b9..cc5d3f4afad4 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > @@ -60,7 +60,7 @@ public: > > > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > > : flags_(flags), name_(name), > > - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID) > > + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0) > > Should you change the other constructor too ? Yes - it's inconsequential, but it would be correct to change it as well. > > Stream() > : flags_(StreamFlag::None), id_(BufferMask::MaskID) > { > } > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > : flags_(flags), name_(name), > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0) > { > } > > It also makes me wonder if you still need the BufferMask enum.. I do still need this enum as it differentiates between stats and embedded data buffer Ids that get passed from PH to the IPA. Buffer Ids are unique in a a single stream, but can be the same across different streams... if that makes sense? Regards, Naush > > > { > > } > > > > @@ -86,44 +86,6 @@ public: > > void releaseBuffers(); > > > > private: > > - class IdGenerator > > - { > > - public: > > - IdGenerator(unsigned int max) > > - : max_(max), id_(0) > > - { > > - } > > - > > - unsigned int get() > > - { > > - unsigned int id; > > - if (!recycle_.empty()) { > > - id = recycle_.front(); > > - recycle_.pop(); > > - } else { > > - id = ++id_; > > - ASSERT(id_ <= max_); > > - } > > - return id; > > - } > > - > > - void release(unsigned int id) > > - { > > - recycle_.push(id); > > - } > > - > > - void reset() > > - { > > - id_ = 0; > > - recycle_ = {}; > > - } > > - > > - private: > > - unsigned int max_; > > - unsigned int id_; > > - std::queue<unsigned int> recycle_; > > - }; > > - > > void clearBuffers(); > > int queueToDevice(FrameBuffer *buffer); > > > > @@ -136,7 +98,7 @@ private: > > std::unique_ptr<V4L2VideoDevice> dev_; > > > > /* Tracks a unique id key for the bufferMap_ */ > > - IdGenerator id_; > > + unsigned int id_; > > > > /* All frame buffers associated with this device stream. */ > > BufferMap bufferMap_; > > -- > > 2.34.1 > >
diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp index e24531e171c8..d32163d3fc0f 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp @@ -53,7 +53,7 @@ void Stream::resetBuffers() void Stream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers) { for (auto const &buffer : *buffers) - bufferMap_.emplace(id_.get(), buffer.get()); + bufferMap_.emplace(++id_, buffer.get()); } const BufferMap &Stream::getBuffers() const @@ -78,7 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const void Stream::setExportedBuffer(FrameBuffer *buffer) { - bufferMap_.emplace(id_.get(), buffer); + bufferMap_.emplace(++id_, buffer); } int Stream::prepareBuffers(unsigned int count) @@ -149,9 +149,6 @@ void Stream::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. @@ -210,7 +207,7 @@ void Stream::clearBuffers() requestBuffers_ = std::queue<FrameBuffer *>{}; internalBuffers_.clear(); bufferMap_.clear(); - id_.reset(); + id_ = 0; } int Stream::queueToDevice(FrameBuffer *buffer) diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h index d1289c4679b9..cc5d3f4afad4 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h @@ -60,7 +60,7 @@ public: Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) : flags_(flags), name_(name), - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID) + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0) { } @@ -86,44 +86,6 @@ public: void releaseBuffers(); private: - class IdGenerator - { - public: - IdGenerator(unsigned int max) - : max_(max), id_(0) - { - } - - unsigned int get() - { - unsigned int id; - if (!recycle_.empty()) { - id = recycle_.front(); - recycle_.pop(); - } else { - id = ++id_; - ASSERT(id_ <= max_); - } - return id; - } - - void release(unsigned int id) - { - recycle_.push(id); - } - - void reset() - { - id_ = 0; - recycle_ = {}; - } - - private: - unsigned int max_; - unsigned int id_; - std::queue<unsigned int> recycle_; - }; - void clearBuffers(); int queueToDevice(FrameBuffer *buffer); @@ -136,7 +98,7 @@ private: std::unique_ptr<V4L2VideoDevice> dev_; /* Tracks a unique id key for the bufferMap_ */ - IdGenerator id_; + unsigned int id_; /* All frame buffers associated with this device stream. */ BufferMap bufferMap_;
Replace the buffer id generation in RPi::Stream with a simple integer counter since ids don't get recycled any more. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/rpi/common/rpi_stream.cpp | 9 ++-- .../pipeline/rpi/common/rpi_stream.h | 42 +------------------ 2 files changed, 5 insertions(+), 46 deletions(-)