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

[Naushir Patuck]

Hi Jacopo,

Thank you for your review!

On Mon, 24 Jul 2023 at 08:50, Jacopo Mondi
<jacopo.mondi at 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 at 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
> >