[libcamera-devel,v3,08/33] libcamera: buffer: Add FrameBuffer interface

2555 diff mbox series

Add a new FrameBuffer class to describe memory used to store frames.
This change just adds the new interface, future patches will migrate all
parts of libcamera to use this and replace the old Buffer interface.

This change needs to specify the const keyword for Plane::length() as to
not get Doxygen confused with FrameBuffer::Plane::length added in this
change.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v2
- Drop default value for planes argument for FrameBuffer::FrameBuffer(planes, cookie).
- Deleted FrameBuffer copy constructor and assignment operator.
- Add a Move constructor for FrameBuffer.
- Update documentation.

Changes since v1:
- Rename FrameBuffer::info() to FrameBuffer::metadata()
- Fixup commit message
- Reorder method order
- Rewrite documentation
- Add operator==() and operator=!() for FrameBuffer::Plane
---
 include/libcamera/buffer.h |  36 ++++++++++
 src/libcamera/buffer.cpp   | 130 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 165 insertions(+), 1 deletion(-)

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:37:43PM +0100, Niklas Söderlund wrote:
> Add a new FrameBuffer class to describe memory used to store frames.
> This change just adds the new interface, future patches will migrate all
> parts of libcamera to use this and replace the old Buffer interface.
> 
> This change needs to specify the const keyword for Plane::length() as to
> not get Doxygen confused with FrameBuffer::Plane::length added in this
> change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Drop default value for planes argument for FrameBuffer::FrameBuffer(planes, cookie).
> - Deleted FrameBuffer copy constructor and assignment operator.
> - Add a Move constructor for FrameBuffer.
> - Update documentation.
> 
> Changes since v1:
> - Rename FrameBuffer::info() to FrameBuffer::metadata()
> - Fixup commit message
> - Reorder method order
> - Rewrite documentation
> - Add operator==() and operator=!() for FrameBuffer::Plane
> ---
>  include/libcamera/buffer.h |  36 ++++++++++
>  src/libcamera/buffer.cpp   | 130 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 165 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 0b95e41a2dd205b2..f5522edaf68bbf61 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -11,6 +11,8 @@
>  #include <stdint.h>
>  #include <vector>
>  
> +#include <libcamera/file_descriptor.h>
> +
>  namespace libcamera {
>  
>  class Request;
> @@ -33,6 +35,40 @@ struct FrameMetadata {
>  	std::vector<Plane> planes;
>  };
>  
> +class FrameBuffer final
> +{
> +public:
> +	struct Plane {
> +		FileDescriptor fd;
> +		unsigned int length;
> +	};
> +
> +	FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> +	FrameBuffer(FrameBuffer &&other);

I don't think the move constructor is needed. It's actually dangerous,
as if a FrameBuffer is added to a request and a new FrameBuffer is later
move-constructed from the original FrameBuffer, bad things will happen.
I think you can delete it (and the corresponding move-assignment
operator).

> +
> +	FrameBuffer(const FrameBuffer &other) = delete;
> +	FrameBuffer &operator=(const FrameBuffer &) = delete;
> +
> +	const std::vector<Plane> &planes() const { return planes_; }
> +
> +	Request *request() const { return request_; }
> +	const FrameMetadata &metadata() const { return metadata_; };
> +
> +	unsigned int cookie() const { return cookie_; }
> +	void setCookie(unsigned int cookie) { cookie_ = cookie; }
> +
> +private:
> +	friend class Request; /* Needed to update request_. */
> +	friend class V4L2VideoDevice; /* Needed to update metadata_. */
> +
> +	std::vector<Plane> planes_;
> +
> +	Request *request_;
> +	FrameMetadata metadata_;
> +
> +	unsigned int cookie_;
> +};
> +
>  class Plane final
>  {
>  public:
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index b7bc948c80748b18..3e2cc9b94595795e 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -240,7 +240,7 @@ void *Plane::mem()
>  }
>  
>  /**
> - * \fn Plane::length()
> + * \fn Plane::length() const
>   * \brief Retrieve the length of the memory region
>   * \return The length of the memory region
>   */
> @@ -473,4 +473,132 @@ void Buffer::cancel()
>   * The intended callers are Request::prepare() and Request::completeBuffer().
>   */
>  
> +/**
> + * \class FrameBuffer
> + * \brief Frame buffer data and its associated dynamic metadata
> + *
> + * The FrameBuffer class is the primary interface for applications, IPAs and
> + * pipelines to interact with frame memory. It contains all the static and

s/pipelines/pipeline handlers/

> + * dynamic information to manage the whole life cycle of a frame capture, from
> + * buffer creation to consumption.
> + *
> + * The static information describes the memory planes that make a frame. The
> + * planes are specified when creating the FrameBuffer and are expressed as a set
> + * of dmabuf file descriptors and length.
> + *
> + * The dynamic information is grouped in a FrameMetadata instance. It is updated
> + * during the processing of a queued capture request, and is valid from the
> + * completion of the buffer as signaled by Camera::bufferComplete() until the
> + * FrameBuffer is either reused in a new request or deleted.
> + *
> + * The creator of a FrameBuffer (application, IPA or pipeline) may associate

s/pipelines/pipeline handler/

> + * to it an integer cookie for any private purpose. The cookie may be set when
> + * creating the FrameBuffer, and updated at any time with setCookie(). The
> + * cookie is transparent to the libcamera core and shall only be set by the
> + * creator of the FrameBuffer. This mechanism supplements the Request cookie.
> + */
> +
> +/**
> + * \struct FrameBuffer::Plane
> + * \brief A memory region to store a single plane of a frame
> + *
> + * Planar pixel formats use multiple memory regions to store the different
> + * colour components of a frame. The Plane structure describes such a memory
> + * region by a dmabuf file descriptor and a length. A FrameBuffer then
> + * contains one or multiple planes, depending on the pixel format of the
> + * frames it is meant to store.
> + *
> + * To support DMA access, planes are associated with dmabuf objects represented
> + * by FileDescriptor handles. The Plane class doesn't handle mapping of the
> + * memory to the CPU, but applications and IPAs may use the dmabuf file
> + * descriptors to map the plane memory with mmap() and access its contents.
> + *
> + * \todo Once we have a Kernel API which can express offsets within a plane
> + * this structure shall be extended to contain this information. See commit
> + * 83148ce8be55e for initial documentation of this feature.
> + */
> +
> +/**
> + * \var FrameBuffer::Plane::fd
> + * \brief The dmabuf file descriptor
> + */
> +
> +/**
> + * \var FrameBuffer::Plane::length
> + * \brief The plane length in bytes
> + */
> +
> +/**
> + * \brief Construct a FrameBuffer with an array of planes
> + * \param[in] planes The frame memory planes
> + * \param[in] cookie Cookie
> + *
> + * The file descriptors associated with each plane are duplicated and can be
> + * closed by the caller after the FrameBuffer has been constructed.

This isn't true anymore, now that the FileDescriptor can be cheaply
copied, is it ? You could explicitly state that the file descriptor must
not be closed by the caller, but FileDescriptor doesn't allow explicit
close. I think you can thus just drop this sentence.

> + */
> +FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> +	: planes_(planes), request_(nullptr), cookie_(cookie)
> +{
> +}
> +
> +/**
> + * \brief Move constructor, create a FrameBuffer by taking over \a other
> + * \param[in] other The other FrameBuffer
> + *
> + * The \a other FrameBuffer will be invalidated.
> + */
> +FrameBuffer::FrameBuffer(FrameBuffer &&other)
> +	: planes_(std::move(other.planes_)),
> +	  request_(utils::exchange(other.request_, nullptr)),
> +	  metadata_(std::move(other.metadata_)),
> +	  cookie_(utils::exchange(other.cookie_, 0))

Now that FileDescriptor can be cheaply copied we can drop the move
constructor. Sorry for having asked you to add one in the review of the
previous version.

> +{
> +}
> +
> +/**
> + * \fn FrameBuffer::planes()
> + * \brief Retrieve the static plane descriptors
> + * \return Array of plane descriptors
> + */
> +
> +/**
> + * \fn FrameBuffer::request()
> + * \brief Retrieve the request this buffer belongs to
> + *
> + * The intended callers of this method are buffer completion handlers that
> + * need to associate a buffer to the request it belongs to.

You've dropped the following paragraph present in v2:

 * A Buffer is associated to a request by Request::prepare() and the
 * association is valid until the buffer completes. The returned request
 * pointer is valid only during that interval.

While I had comments about the documentation, I don't think this should
be dropped, it's important to document the validaty of the association.
More importantly I asked why the buffer wasn't associated with the
request at Request::addBuffer() time.

> + *
> + * \return The Request the Buffer belongs to, or nullptr if the buffer is
> + * not associated with a request
> + */
> +
> +/**
> + * \fn FrameBuffer::metadata()
> + * \brief Retrieve the dynamic metadata
> + * \return Dynamic metadata for the frame contained in the buffer
> + */
> +
> +/**
> + * \fn FrameBuffer::cookie()
> + * \brief Retrieve the cookie
> + *
> + * The cookie belongs to the creator of the FrameBuffer, which controls its
> + * lifetime and value.
> + *
> + * \sa setCookie()
> + *
> + * \return The cookie
> + */
> +
> +/**
> + * \fn FrameBuffer::setCookie()
> + * \brief Set the cookie
> + * \param[in] cookie Cookie to set
> + *
> + * The cookie belongs to the creator of the FrameBuffer. Its value may be
> + * modified at any time with this method. Applications and IPAs shall not modify
> + * the cookie value of buffers they haven't created themselves. The libcamera
> + * core never modifies the buffer cookie.
> + */
> +
>  } /* namespace libcamera */