[libcamera-devel] [PATCH v4 09/12] libcamera: buffer: Store Request reference in Buffer

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-09 21:25:45 +0200, Jacopo Mondi wrote:
> Add to the Buffer class methods to set and retrieve a reference to the
> Request instance this buffer is part of.
> 
> As Buffers might outlive the Request they are associated with, the
> reference is only temporarly valid during the buffer completion interval
> (since when the buffer gets queued to Camera for processing, until it
> gets marked as completed).

I do not like this as the new functions are visible both to applications 
and pipeline handlers where we really should make the API fool proof.  
Looking a head in the series I do not like how this is used neither.

Something like this might be needed depending on outcome of discussions
later patches in this series. The patch itself is proper and if I agreed 
that it was needed and/or that the usage of it later I would have 
attached by tag. For now I'm withholding it pending discussion.

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/buffer.h |  5 +++++
>  src/libcamera/buffer.cpp   | 39 +++++++++++++++++++++++++++++++++++++-
>  src/libcamera/request.cpp  |  4 ++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 0c844d126a27..10c46cd57ab0 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -13,6 +13,7 @@
>  namespace libcamera {
>  
>  class BufferPool;
> +class Request;
>  
>  class Plane final
>  {
> @@ -53,6 +54,9 @@ public:
>  	Status status() const { return status_; }
>  	std::vector<Plane> &planes() { return planes_; }
>  
> +	void setRequest(Request *req) { request_ = req; }
> +	Request *request() const { return request_; }
> +
>  private:
>  	friend class BufferPool;
>  	friend class PipelineHandler;
> @@ -67,6 +71,7 @@ private:
>  	Status status_;
>  
>  	std::vector<Plane> planes_;
> +	Request *request_;
>  };
>  
>  class BufferPool final
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index e2d1cf04411e..68738b733ba6 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -196,7 +196,7 @@ void *Plane::mem()
>   */
>  
>  Buffer::Buffer()
> -	: index_(-1)
> +	: index_(-1), request_(nullptr)
>  {
>  }
>  
> @@ -248,6 +248,43 @@ Buffer::Buffer()
>   * \return The buffer status
>   */
>  
> +/**
> + * \fn Buffer::setRequest()
> + * \brief Set the request this buffer belongs to
> + *
> + * Buffers are associated to Streams in a Request, which is then sent to the
> + * Camera for processing. This method stores in the Buffer a pointer to the
> + * Request this Buffer is part of, for later retrieval through the
> + * Buffer::request() method.
> + *
> + * Buffers are associated to requests at Request::prepare() time and said
> + * association is valid until the buffer does not complete at
> + * Request::completeBuffer() time. Before and after the buffer completion
> + * interval (the time between when the request is queued to the Camera, and
> + * the buffer is marked as 'complete' by pipeline handlers) the reference to
> + * Request is set to nullptr.
> + */
> +
> +/**
> + * \fn Buffer::request()
> + * \brief Retrieve the request this buffer belongs to
> + *
> + * The intended callers of this method are buffer completion slots
> + * implemented in CameraData sub-classes which needs to associated a request
> + * to the just completed buffer, before calling Request::completeBuffer().
> + *
> + * It is up to the caller of this function to deal with the case the buffer has
> + * been already marked as complete, and the reference to the Request has been
> + * invalidated and set to nullptr.
> + *
> + * See also Buffer::setRequest() for a more detailed explanation of the
> + * validity interval of the Request reference contained in a Buffer.
> + *
> + * \return The Request this Buffer belongs to or nullptr if the Buffer has
> + * not been queued to the Camera for processing yet or it has completed already.
> + * \sa Buffer::setRequest()
> + */
> +
>  /**
>   * \brief Mark a buffer as cancel by setting its status to BufferCancelled
>   */
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 3e10c995a6fa..7d80c635fa92 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -130,6 +130,8 @@ int Request::prepare()
>  {
>  	for (auto const &pair : bufferMap_) {
>  		Buffer *buffer = pair.second;
> +
> +		buffer->setRequest(this);
>  		pending_.insert(buffer);
>  	}
>  
> @@ -166,6 +168,8 @@ bool Request::completeBuffer(Buffer *buffer)
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  
> +	buffer->setRequest(nullptr);
> +
>  	return empty();
>  }
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund