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

[Laurent Pinchart]

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 at 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 */

-- 
Regards,

Laurent Pinchart