[libcamera-devel,06/30] libcamera: buffer: Add FrameBuffer interface

2354 diff mbox series

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
Doxygen confused with FrameBuffer::Plane::length added in this chance.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/buffer.h |  33 ++++++++++
 src/libcamera/buffer.cpp   | 130 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 162 insertions(+), 1 deletion(-)

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the patch.

On Wed, Nov 27, 2019 at 03:04:43PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:35:56AM +0100, Niklas Söderlund wrote:
> > Add the FrameBuffer interface buffer implementation. This change just

"Add a new FrameBuffer class to describe memory used to store frames.
..."

> > 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@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?

As mentioned in the review of 05/30, I think we also need an offset
here, as well as a stride. And now that I think about it, the stride may
belong to BufferInfo::Plane, as it's related to the format stored in the
buffer.

We need to be very clear and explicit about what FrameBuffer::Plane and
BufferInfo::Plane represent. FrameBuffer may be seen as describing the
memory, leaving usage of that memory to BufferInfo, but that's likely
too simplistic. The fact that we have planes in a FrameBuffer already
implies that a FrameBuffer is linked to a PixelFormat.

I know this starts creating a long list of food for thought, but could
you give some consideration to this and make a proposal ? It doesn't
have to be formal documentation yet, but we need to agree on an explicit
design. I can then help with the documentation if needed.

> > +
> > +	FrameBuffer(std::vector<Plane> planes, unsigned int cookie = 0);
> > +	~FrameBuffer();
> > +
> > +	Request *request() const { return request_; }
> > +	const BufferInfo &info() const { return info_; };

Could you move those two lines down, to start with the static
information describing the memory (dmabufs and planes) and follow with
data that is associated with a FrameBuffer in a transcient way only ?
Please separate the two groups with a blank line. Same for the private
members.

> > +	const std::vector<Dmabuf *> &dmabufs() { return dmabufs_; }

Why do you store Dmabuf pointers and not Dmabuf instances ?

> > +
> > +	const std::vector<Plane> &planes() const { return planes_; }

The two vectors make me feel something is wrong :-S

> > +
> > +	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.

I'm afraid this is too vague and fuzzy. I won't propose a different
wording now as we need to agree on the design first, as explained above.

> 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.

I agree with Jacopo, I'm afraid this is way too vague. There's very
little chance someone not familiar with this series will understand the
FrameBuffer and FrameBuffer::Plane classes properly. A more detailed
overview of the FrameBuffer class is needed too. This class is one of
the areas where the documentation has to be crystal clear with surgical
precision.

> > + */
> > +
> > +/**
> > + * \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.

Unless I'm mistaken nobody gives a cookie to the constructor.

> > + */
> > +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 :)

This seems in the category of optimizations that would save a few CPU
cycles in a hot path that involves IPC, and thus hundreds of thousands
of cycles.

> > + *
> > + * 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?

Only the rkisp1 pipeline handler currently calls setCookie(), which
conflicts with the documentation of the cookie parameter to the
constructor that states it can be set by the user. This is error-prone
and needs to be reworked. Unless better documented with clear use cases
and a clear API, this feels like a hack to me.

> > + * \param[in] cookie The cookie to set on the request
> 
> To the request or to the buffer?
> 
> > + */
> > +
> >  } /* namespace libcamera */