[libcamera-devel] [PATCH 05/13] pipeline: raspberrypi: rpi_stream: Set invalid buffer to id == 0

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Apr 26, 2023 at 02:10:49PM +0100, Naushir Patuck via libcamera-devel wrote:
> At present, the RPiStream buffer ids == -1 indicates an invalid value.
> As a simplification, use id == 0 to indicate an invalid value. This
> allows for better code readability.
> 
> As a consequence of this, use unsigned int for the buffer id values.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  .../pipeline/rpi/common/rpi_stream.cpp         | 10 +++++-----
>  src/libcamera/pipeline/rpi/common/rpi_stream.h | 18 +++++++++---------
>  src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp |  6 +++---
>  3 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> index 2bb10f25d6ca..3690667e9aa6 100644
> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> @@ -55,17 +55,17 @@ const BufferMap &Stream::getBuffers() const
>  	return bufferMap_;
>  }
>  
> -int Stream::getBufferId(FrameBuffer *buffer) const
> +unsigned int Stream::getBufferId(FrameBuffer *buffer) const
>  {
>  	if (importOnly_)
> -		return -1;
> +		return 0;
>  
>  	/* Find the buffer in the map, and return the buffer id. */
>  	auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),
>  			       [&buffer](auto const &p) { return p.second == buffer; });
>  
>  	if (it == bufferMap_.end())
> -		return -1;
> +		return 0;
>  
>  	return it->first;
>  }
> @@ -77,10 +77,10 @@ void Stream::setExternalBuffer(FrameBuffer *buffer)
>  
>  void Stream::removeExternalBuffer(FrameBuffer *buffer)
>  {
> -	int id = getBufferId(buffer);
> +	unsigned int id = getBufferId(buffer);
>  
>  	/* Ensure we have this buffer in the stream, and it is marked external. */
> -	ASSERT(id != -1 && (id & BufferMask::MaskExternalBuffer));
> +	ASSERT(id & BufferMask::MaskExternalBuffer);
>  	bufferMap_.erase(id);
>  }
>  
> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> index b8bd79cf1535..1aae674967e1 100644
> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h
> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> @@ -58,7 +58,7 @@ public:
>  
>  	void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  	const BufferMap &getBuffers() const;
> -	int getBufferId(FrameBuffer *buffer) const;
> +	unsigned int getBufferId(FrameBuffer *buffer) const;
>  
>  	void setExternalBuffer(FrameBuffer *buffer);
>  	void removeExternalBuffer(FrameBuffer *buffer);
> @@ -74,25 +74,25 @@ private:
>  	class IdGenerator
>  	{
>  	public:
> -		IdGenerator(int max)
> +		IdGenerator(unsigned int max)
>  			: max_(max), id_(0)
>  		{
>  		}
>  
> -		int get()
> +		unsigned int get()
>  		{
> -			int id;
> +			unsigned int id;
>  			if (!recycle_.empty()) {
>  				id = recycle_.front();
>  				recycle_.pop();
>  			} else {
> -				id = id_++;
> +				id = ++id_;
>  				ASSERT(id_ <= max_);
>  			}
>  			return id;
>  		}
>  
> -		void release(int id)
> +		void release(unsigned int id)
>  		{
>  			recycle_.push(id);
>  		}
> @@ -104,9 +104,9 @@ private:
>  		}
>  
>  	private:
> -		int max_;
> -		int id_;
> -		std::queue<int> recycle_;
> +		unsigned int max_;
> +		unsigned int id_;
> +		std::queue<unsigned int> recycle_;
>  	};
>  
>  	void clearBuffers();
> diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> index cfde7f2e2229..e54bf1ef2c17 100644
> --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> @@ -1210,7 +1210,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  			continue;
>  
>  		FrameBuffer *buffer = request->findBuffer(stream);
> -		if (buffer && stream->getBufferId(buffer) == -1) {
> +		if (buffer && !stream->getBufferId(buffer)) {
>  			/*
>  			 * This buffer is not recognised, so it must have been allocated
>  			 * outside the v4l2 device. Store it in the stream buffer list
> @@ -2042,7 +2042,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  
>  	for (RPi::Stream &s : unicam_) {
>  		index = s.getBufferId(buffer);
> -		if (index != -1) {
> +		if (index) {
>  			stream = &s;
>  			break;
>  		}
> @@ -2098,7 +2098,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  
>  	for (RPi::Stream &s : isp_) {
>  		index = s.getBufferId(buffer);
> -		if (index != -1) {
> +		if (index) {
>  			stream = &s;
>  			break;
>  		}

-- 
Regards,

Laurent Pinchart