[libcamera-devel] [PATCH 06/30] libcamera: buffer: Add FrameBuffer interface
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 27 15:04:43 CET 2019
Hi Niklas,
On Wed, Nov 27, 2019 at 12:35:56AM +0100, Niklas Söderlund wrote:
> Add the FrameBuffer interface buffer implementation. 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 clarify the const for Plane::length() as to not get
s/clarify/specify ?
s/const/const keyword ?
> Doxygen confused with FrameBuffer::Plane::length added in this chance.
s/chance/change ?
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/buffer.h | 33 ++++++++++
> src/libcamera/buffer.cpp | 130 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 162 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 3c430afbfe8e9a05..fe5195327b540f5c 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -171,6 +171,39 @@ private:
> Stream *stream_;
> };
>
> +class FrameBuffer final
> +{
> +public:
> + struct Plane {
> + int fd;
> + unsigned int length;
> + };
It seems to me the use of FrameBuffer::Plane are basically 2:
- serialization
- the address of the planes_ array is used as cookies to maintain an
assoiciation between a FrameBuffer and the v4l2 buffer index
Can this be unified with the Dmabuf class?
> +
> + FrameBuffer(std::vector<Plane> planes, unsigned int cookie = 0);
> + ~FrameBuffer();
> +
> + Request *request() const { return request_; }
> + const BufferInfo &info() const { return info_; };
> + const std::vector<Dmabuf *> &dmabufs() { return dmabufs_; }
> +
> + const std::vector<Plane> &planes() const { return planes_; }
> +
> + 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 info_. */
> +
> + Request *request_;
> + BufferInfo info_;
> + std::vector<Dmabuf *> dmabufs_;
> +
> + std::vector<Plane> planes_;
> +
> + unsigned int cookie_;
> +};
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_BUFFER_H__ */
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 5516055b2ea885c2..07647124a2cd9c62 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -412,7 +412,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
> */
> @@ -645,4 +645,132 @@ void Buffer::cancel()
> * The intended callers are Request::prepare() and Request::completeBuffer().
> */
>
> +/**
> + * \class FrameBuffer
> + * \brief A buffer handle and dynamic metadata
> + *
> + * The FrameBuffer class references a buffer memory and associates dynamic
> + * metadata related to the frame contained in the buffer. It allows referencing
> + * buffer memory.
> + *
> + * A FrameBuffer object can be created from both internal and externally
> + * allocated dmabuf.
s/dmabuf/memory buffer
Techincally, a dmabuf handle is not allocated
> + */
> +
> +/**
> + * \struct FrameBuffer::Plane
> + * \brief Describe a DMA buffer using low level data
As commented on the previous patch, your use of DMA confuses me. But
it might just be my poor understanding of the issue.
> + *
> + * The plane description contains a file descriptor and a length, together
> + * they represent a dmabuf.
As commented on the previous patch your use of the term dmabuf
confuses me.
> + */
> +
> +/**
> + * \var FrameBuffer::Plane::fd
> + * \brief The dmabuf file handle
> + */
> +
> +/**
> + * \var FrameBuffer::Plane::length
> + * \brief The dmabuf length
s/dmabuf/buffer
> + */
> +
> +/**
> + * \brief Create a libcamera FrameBuffer object from an array of dmabufs
> + * \param[in] planes dmabufs described as planes
> + * \param[in] cookie Opaque cookie for application use
> + *
> + * Buffers used by libcamera might be allocated externally or internally to
> + * libcamera, the FrameBuffer object handles both cases.
> + *
> + * The \a cookie is stored in the buffer and is accessible through the
> + * cookie() method at any time. It is typically used by user to map the
> + * buffer to an external resource, and is completely opaque to the FrameBuffer
> + * object.
> + */
> +FrameBuffer::FrameBuffer(std::vector<Plane> planes, unsigned int cookie)
> + : request_(nullptr), cookie_(cookie)
> +{
> + /* Clone all file descriptors and create dmabufs. */
> + for (Plane plane : planes)
Why two loops?
for (planes)
new Dmabuf(plane)
for (dmabuf)
Plane p(dmabuf)
Can't this be:
for (planes)
d = new Dmabuf()
Plane p(d)
push_back(d)
push_back(p)
?
> + dmabufs_.push_back(new Dmabuf(plane.fd, plane.length));
Why is dmabufs_ an array of pointers? You create them here and destroy
them at delete time. Wouldn't it be better to emplace_back() an
instance in the dmabufs_ vector ?
> +
> + /* Cache the new plane description for this frame buffer. */
> + for (const Dmabuf *dmabuf : dmabufs_) {
> + Plane plane = {
> + .fd = dmabuf->fd(),
> + .length = dmabuf->length()
> + };
> +
> + planes_.emplace_back(plane);
I think you're here invoking the copy constructor of Planes if I'm not
mistaken, as you create a Plane instance and the push_back to the
vector, so you're not emplacing but copying instead.
I think you would need a constructor for the Plane class which takes
fd and length and the you could emplace
Something like https://paste.debian.net/1118240/
But I still think you should really use the planes argument to
construct dmabuf and the if you want to copy each plane in planes
into planes_ instead of going through Plane->Dmabuf->Plan again.
> + }
> +}
> +
> +FrameBuffer::~FrameBuffer()
> +{
> + for (Dmabuf *dmabuf : dmabufs_)
> + delete dmabuf;
> +}
> +
> +/**
> + * \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.
> + *
> + * 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.
> + *
> + * \return The Request the Buffer belongs to, or nullptr if the buffer is
> + * either completed or not associated with a request
> + */
> +
> +/**
> + * \fn FrameBuffer::info()
> + * \brief Retrieve the buffer metadata information
> + *
> + * The buffer metadata information is update every time the buffer contained
"the buffer contained "
contained in ? The frame buffer? So I would use "the buffer content
is"
> + * are changed, for example when it is dequeued from hardware.
> + *
> + * \return Metadata of the buffer
> + */
> +
> +/**
> + * \fn FrameBuffer::dmabufs()
> + * \brief Retrieve the Dmabuf(s) from the buffer
> + *
> + * This is intended for applications who wish to access the frame buffer
> + * content. The array returned is one item for each plane in the frame buffer.
To me an application should access "planes" (or "memory") and from
there get the dmabuf handles or the memory mapped CPU accessible
address. Have I already said so? :)
> + *
> + * \return Array of Dmabuf(s)
Do not pluralize class names. Dmabuf instances.
> + */
> +
> +/**
> + * \fn FrameBuffer::planes()
> + * \brief Retrieve the Dmabuf(s) as plane descriptions
planes() seems to return a vector of Plane instances, not Dmabuf ones.
> + *
> + * This is intended for internal libcamera use-cases where a frame buffer needs
> + * to be sent over an IPC barrier. Since IPC may be in the hot-path of capture
> + * a dedicated cached method is provided, the result might otherwise be
> + * constructed from information retrieved from dmabuf().
You know my opinion there :)
> + *
> + * The array returned is one item for each plane in the frame buffer.
> + *
> + * \return Array of Dmabuf(s) as plane descriptions
ditto
> + */
> +
> +/**
> + * \fn FrameBuffer::cookie()
> + * \brief Retrieve the cookie associated with the buffer
> + * \return The cookie
> + */
> +
> +/**
> + * \fn FrameBuffer::setCookie()
> + * \brief Set the cookie associated with the buffer
Who should set the cookie?
> + * \param[in] cookie The cookie to set on the request
To the request or to the buffer?
Thanks
j
> + */
> +
> } /* namespace libcamera */
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191127/5d7681c7/attachment.sig>
More information about the libcamera-devel
mailing list