[libcamera-devel] [PATCH v2 4/4] pipeline: rpi: Simplify buffer id generation

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:40)
> 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 at raspberrypi.com>
> ---
>  .../pipeline/rpi/common/rpi_stream.cpp        |  9 ++--
>  .../pipeline/rpi/common/rpi_stream.h          | 44 ++-----------------

Diffstat looks like a win!

There are no bugs in code that doesn't exist, right? ;-)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  2 files changed, 6 insertions(+), 47 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> index e1858c731f57..d1310635c091 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..889b499782a4 100644
> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h
> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> @@ -54,13 +54,13 @@ public:
>         using StreamFlags = Flags<StreamFlag>;
>  
>         Stream()
> -               : flags_(StreamFlag::None), id_(BufferMask::MaskID)
> +               : flags_(StreamFlag::None), id_(0)
>         {
>         }
>  
>         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_;
> -- 
> 2.34.1
>