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

[Jacopo Mondi]

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 ?

>  }
>
>  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
>