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

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

Commit Message

Naushir Patuck July 25, 2023, 8:55 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          | 44 ++-----------------
 2 files changed, 6 insertions(+), 47 deletions(-)

Comments

Jacopo Mondi July 27, 2023, 9:34 a.m. UTC | #1 
Hi Naush

On Tue, Jul 25, 2023 at 09:55:40AM +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>

Looks good!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  .../pipeline/rpi/common/rpi_stream.cpp        |  9 ++--
>  .../pipeline/rpi/common/rpi_stream.h          | 44 ++-----------------
>  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
>
Kieran Bingham Sept. 4, 2023, 10:12 a.m. UTC | #2 
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@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@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
>

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