[libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator: Get and return buffers

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:
> Add to the FrameBufferAllocator class two methods to get and
> return buffers from the pool of buffers allocated for a Stream.
> 
> The two methods return pointer to the allocated buffers without
> transferring ownership to the caller.

I'm sorry I don't like this patch. If it was an interface that was 
internal I would be more OK with this change. The FrameBufferAllocator 
is facing applications and this change introduces a duality between the 
new getBuffer() and the existing buffers() functionality.

I think it creates complexity in this user-facing API which is not 
really needed. How about creating a HALFrameBufferAllocator that is 
local to the HAL and inherits from FrameBufferAllocator while adding the 
features you want?

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/framebuffer_allocator.h |  5 ++
>  src/libcamera/framebuffer_allocator.cpp   | 60 +++++++++++++++++++++--
>  2 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> index 2a4d538a0cb2..1f3f10d4ec03 100644
> --- a/include/libcamera/framebuffer_allocator.h
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -9,6 +9,7 @@
>  
>  #include <map>
>  #include <memory>
> +#include <queue>
>  #include <vector>
>  
>  namespace libcamera {
> @@ -33,9 +34,13 @@ public:
>  	bool allocated() const { return !buffers_.empty(); }
>  	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
>  
> +	FrameBuffer *getBuffer(Stream *stream);
> +	void returnBuffer(Stream *stream, FrameBuffer *buffer);
> +
>  private:
>  	std::shared_ptr<Camera> camera_;
>  	std::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;
> +	std::map<Stream *, std::queue<FrameBuffer *>> availableBuffers_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 7ed80011c845..7429d6b9edb7 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
>  
>  FrameBufferAllocator::~FrameBufferAllocator()
>  {
> -	buffers_.clear();
> +	clear();
>  }
>  
>  /**
> @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)
>  	}
>  
>  	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> -	if (ret == -EINVAL)
> +	if (ret == -EINVAL) {
>  		LOG(Allocator, Error)
>  			<< "Stream is not part of " << camera_->id()
>  			<< " active configuration";
> -	return ret;
> +		return ret;
> +	}
> +
> +	for (const auto &buffer : buffers_[stream])
> +		availableBuffers_[stream].push(buffer.get());
> +
> +	return 0;
>  }
>  
>  /**
> @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)
>  	buffers.clear();
>  	buffers_.erase(iter);
>  
> +	availableBuffers_[stream] = {};
> +	availableBuffers_.erase(availableBuffers_.find(stream));
> +
>  	return 0;
>  }
>  
> @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)
>  void FrameBufferAllocator::clear()
>  {
>  	buffers_.clear();
> +	availableBuffers_.clear();
>  }
>  
>  /**
> @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const
>  	return iter->second;
>  }
>  
> +/**
> + * \brief Get a pointer to a \a buffer for the \a stream
> + * \param[in] stream The stream to get a buffer for
> + *
> + * The method returns a pointer to a FrameBuffer but does transfer the buffer
> + * ownership to the caller: the returned pointer remains valid until the
> + * FrameBufferAllocator does not get deleted or the allocated buffers do not get
> + * released with a call for free() or clear().
> + *
> + * \return A FrameBuffer pointer or nullptr if the no buffers is available
> + */
> +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)
> +{
> +	if (!allocated() || buffers_[stream].empty())
> +		return nullptr;
> +
> +	FrameBuffer *frameBuffer = availableBuffers_[stream].front();
> +	availableBuffers_[stream].pop();
> +
> +	return frameBuffer;
> +}
> +
> +/**
> + * \brief Return a \a buffer to the list of buffers available for the a \a stream
> + * \param[in] stream The stream to return buffer to
> + * \param[in] buffer The buffer to return
> + */
> +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)
> +{
> +	if (!allocated())
> +		return;
> +
> +	for (const auto &b : buffers_[stream]) {
> +		/*
> +		 * Return the buffer to the available queue only if it was part
> +		 * of the vector of buffers allocated for the Stream.
> +		 */
> +		if (b.get() != buffer)
> +			continue;
> +
> +		availableBuffers_[stream].push(buffer);
> +	}
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund