[libcamera-devel,v1,4/4] pipeline: rpi: Simplify buffer id generation
diff mbox series

Message ID 20230721093759.27700-5-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: External buffer handling
Related show

Commit Message

Naushir Patuck July 21, 2023, 9:37 a.m. UTC 
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(-)

Comments

Jacopo Mondi July 24, 2023, 7:50 a.m. UTC | #1 
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
>
Naushir Patuck July 25, 2023, 8:01 a.m. UTC | #2 
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
> >

Patch
diff mbox series 

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_;