[libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add FrameBuffer interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 7 03:35:52 CET 2020
Hi Niklas,
Thank you for the patch.
On Fri, Jan 03, 2020 at 05:28:04PM +0100, Jacopo Mondi wrote:
> On Mon, Dec 30, 2019 at 01:04:47PM +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 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 | 32 +++++++++
> > src/libcamera/buffer.cpp | 132 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 163 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 0b95e41a2dd205b2..862031123b4b510c 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>
> > +
>
> it doesn't work forward-declaring it.. weird
That's because FileDescriptor is embedded in FrameBuffer::Plane, so the
compiler needs the full definition.
> > namespace libcamera {
> >
> > class Request;
> > @@ -33,6 +35,36 @@ 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);
>
> This planes = {} looks weird and not safe, as you would zero all your
> plane informations in the constructor.
>
> I tried removing it, and I got an error in rkisp1 IPA module, but I
> think that part is not right, and so you need this workaround here.
>
> It's possibly worth commenting on the patch that introduces that part,
> but since I'm here already..
>
>
> So if I remove the default = {} planes parameter value I get:
>
> [snip]
> tuple:1674:9: error: no matching constructor for initialization of 'libcamera::FrameBuffer'
> second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
> [snip]
> ../src/ipa/rkisp1/rkisp1.cpp:109:16: note: in instantiation of member function 'std::map<unsigned int, libcamera::FrameBuffer, std::less<unsigned int>, std::allocator<std::pair<const unsigned int, libcamera::FrameBuffer> > >::operator[]' requested here
> buffers_[buffer.id].planes()[0].length,
>
>
> The culprit here seems to be in the following lines of rkisp1.cpp
>
> void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> {
> for (const IPABuffer &buffer : buffers) {
> buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));
> buffersMemory_[buffer.id] = mmap(NULL,
> buffers_[buffer.id].planes()[0].length,
> PROT_READ | PROT_WRITE,
> MAP_SHARED,
> buffers_[buffer.id].planes()[0].fd.fd(),
> 0);
>
> if (buffersMemory_[buffer.id] == MAP_FAILED) {
> int ret = -errno;
> LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: "
> << strerror(-ret);
> }
> }
> }
>
> In
> buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));
>
> You're first constructing a FrameBuffer, then calling the copy
> constructor when emplacing, but that's unrelated with the error above.
>
> Later you access buffers_ using operator[] which the compiler
> complains about because if there is no such element with key=id it
> gets created, and the compiler fails to find an opportune
> constructor (std::map::operator[] is nasty!)
>
> With the below change, you can then remove the = {} and then delete
> the FrameBuffer copy constructor as well (and possibly the assignement
> operator too).
The assignment operator should be deleted too, as classes should have
both a copy constructor and a copy assignment operator, or neither
(https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all).
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 9d4443cf56d4..e1ea97c7ec75 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -41,7 +41,8 @@ public:
> unsigned int length;
> };
>
> - FrameBuffer(const std::vector<Plane> &planes = {}, unsigned int cookie = 0);
> + FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> + FrameBuffer(const FrameBuffer &other) = delete;
>
> const std::vector<Plane> &planes() const { return planes_; }
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 63776563b149..0538d356f6f9 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -104,12 +104,14 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> {
> for (const IPABuffer &buffer : buffers) {
> - buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));
> + buffers_.emplace(buffer.id, buffer.planes);
> +
> + const FrameBuffer &b = buffers_.at(buffer.id);
s/b/fb/
You can also use the fact that emplate returns an iterator:
const FrameBuffer &fb = buffers_.emplace(buffer.id, buffer.planes).first->second;
or
auto elem = buffers_.emplace(buffer.id, buffer.planes);
const FrameBuffer &fb = elem.first->second;
> buffersMemory_[buffer.id] = mmap(NULL,
> - buffers_[buffer.id].planes()[0].length,
> + b.planes()[0].length,
> PROT_READ | PROT_WRITE,
> MAP_SHARED,
> - buffers_[buffer.id].planes()[0].fd.fd(),
> + b.planes()[0].fd.fd(),
> 0);
>
> if (buffersMemory_[buffer.id] == MAP_FAILED) {
> @@ -123,7 +125,11 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> {
> for (unsigned int id : ids) {
> - munmap(buffersMemory_[id], buffers_[id].planes()[0].length);
> + const auto b = buffers_.find(id);
s/b/fd/ here too.
> + if (b == buffers_.end())
> + continue;
> +
> + munmap(buffersMemory_[id], b->second.planes()[0].length);
> buffersMemory_.erase(id);
> buffers_.erase(id);
> }
>
> Sorry if this mixes comments on two different patches.
I like this proposal :-)
> > +
> > + 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 7673b96f29728a24..7c78b9b8f1e90aa5 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -229,7 +229,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
> > */
> > @@ -462,4 +462,134 @@ void Buffer::cancel()
> > * The intended callers are Request::prepare() and Request::completeBuffer().
> > */
> >
> > +/**
> > + * \class FrameBuffer
> > + * \brief A buffer handle and dynamic metadata
>
> s/A buffer handle and/Frame buffer data and its associated/ ?
> > + *
> > + * The FrameBuffer class is the primary interface for applications, IPAs and
> > + * pipelines to interact with capture memory. It contains all static and
s/capture/frame/ (as we'll support reprocessing at some point).
> > + * dynamic information to handle the whole life-cycle of creating, queueing,
> > + * capturing and consumption of a single frame capture.
>
> s/consumption/consuming ? given the usage of 'ing' in the other verbs
> ? (I'm asking, not sure)
In which case s/of a/a/. I think this needs to be slightly rephrased as
"creating [...] of a single frame capture" doesn't sound right.
"It contains all the static and dynamic information to manage the whole
life cycle of a frame capture, from buffer creation to consumption." ?
> > + *
> > + * The static information describes the one or more memory planes which makes
>
> s/makes/make
>
> and I would say "planes of which an image frame is composed of"
That sounds very heavy.
"The static information describes the memory planes that make a frame",
or "... that store the content of a frame".
> > + * up the complete frame. The static information needs to be available at
> > + * creation of the FrameBuffer and needs to be expressed as a valid dmabuf file
>
> and needs to be/are
>
> > + * descriptor and length.
"The planes are specified when creating the FrameBuffer and are
expressed as a set of dmabuf file descriptors and length."
(I think we can omit "valid", it's implicit, isn't it ?)
> > + *
> > + * The dynamic data, content of the memory planes and the metadata, is updated
I'm not sure I would mention content of the memory planes here (assuming
you mean the frame pixel data).
> > + * each time the buffer is successfully processed by a camera, by queueing it as
> > + * part of a request.
This sounds to me that "the data [...] is updated [...] by queueing it
as part of a request.".
> The dynamic data is only valid after the cameras buffer
> > + * complete signal have been raised and until the FrameBuffer is queued once
>
> s/raised/emitted
>
> > + * more to the camera as part of a new request.
>
> s/once more/again
* 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 (applications, IPA or pipeline) may associate
s/applications/application/
> > + * an integer cookie with the instance at any time to aid in its usage by the
> > + * object creator. This cookie is transparent to libcamera core and shall only
>
> I get what you mean but it's a bit convoluted.
>
> > + * be set and consumed by the user. This supplements the Request objects cookie.
s/objects/object's/ or just drop "objects".
"may associate 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 description of a memory plane
> > + *
> > + * A memory plane is described by a dmabuf file descriptor and a plane length.
>
> dmabuf handle ?
s/plane length/length/
> > + *
> > + * Applications or IPAs can use this information to map the planes memory and
s/planes/plane/
> > + * access its content. The mapper of the memory is responsible for unmaping
> > + * the memory before the FrameBuffer::Plane object is destroyed.
Is that strictly required ?
> Isn't libcamera performing the mapping internally ?
Not anymore as far as I can tell. I'm tempted to say that this paragraph
is out of scope as we don't care what users do with the dmabuf fd, but
we may get puzzled users asking how they can access the content of the
planes (and I think we need some kind of helper for this, but it can be
implemented on top).
> > + *
> > + * \todo Once we have a Kernel API which can express offsets within a plane
> > + * this structure shall be extended to contain this information.
> > + */
> > +
> > +/**
> > + * \var FrameBuffer::Plane::fd
> > + * \brief The dmabuf file descriptor
> > + */
> > +
> > +/**
> > + * \var FrameBuffer::Plane::length
> > + *
> > + * The length value is only used if the memory of the plane needs to be mapped.
> > + * Therefor a length of zero is valid for planes allocated outside libcamera and
> > + * queued to a Camera as part of a Request, libcamera will never poke inside
> > + * that memory and thus do not need to know its length.
How will we inform V4L2 when queueing the imported dmabuf then ? I think
the length should always be valid.
> > + *
> > + * All planes created by pipeline handlers however need to have a correct length
> > + * set as IPAs and other parts of libcamera core might wish to memory map the
> > + * plane.
I think we can drop all this.
> > + *
> > + * \brief The plane length
"The plane length in bytes"
> > + */
> > +
> > +/**
> > + * \brief Create a libcamera FrameBuffer object from an array of planes
>
> Connstruct a FrameBuffer with an array of planes
s/Connstruct/Construct/
> > + * \param[in] planes Planes that makes up the FrameBuffer
>
> simnply "The frame memory planes" ?
>
> > + * \param[in] cookie Integer cookie
> > + *
> > + * Create a FrameBuffer object from an array of static plane descriptions. The
s/object/instance/ ?
s/static plane descriptions/planes/ ?
> > + * creator may close the file descriptors in the plane description after the
>
> creator/caller ?
>
> I would instead:
> "The file descriptor associated with each plane are duplicated and can
s/descriptor/descriptors/
> be closed by the caller after the FrameBuffer has been constructed"
> > + * FrameBuffer have been created.
> > + */
> > +FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > + : request_(nullptr), cookie_(cookie)
> > +{
> > + /* Clone all file descriptors. */
>
> Duplicate or clone ?
>
> > + planes_ = planes;
Can't you do this as part of the initializers ?
> > +}
I think we also need a move constructor. If you look at the usage of
this class in V4L2VideoDevice::createFrameBuffer(), the fds from the
planes vector are duplicated when creating the FrameBuffer and then
immediately closed as planes gets out of scope. A move version of this
constructor would solve this issue.
> > +
> > +/**
> > + * \fn FrameBuffer::planes()
> > + * \brief Retrieve the static plane descriptions
> > + * \return Array of plane descriptions
"descriptions" or "descriptors" ? I like "descriptors" better as it
matches the dmabuf file descriptor.
> > + */
> > +
> > +/**
> > + * \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
s/Buffer/FrameBuffer/
Request::prepare() is a private method. Why don't we associate the
buffer in addBuffer() ? We need a user-facing API here.
> > + * association is valid until the buffer completes. The returned request
> > + * pointer is valid only during that interval.
"until the buffer completes" may be ambiguous, does it mean before or
after emitting the buffer completion signal ? I know it's the latter,
but we should be explicit here. "until the buffer has completed" may be
slightly better. "until after the buffer has completed" ? Feel free to
propose a better wording.
> > + *
> > + * \return The Request the Buffer belongs to, or nullptr if the buffer is
> > + * either completed or not associated with a request
> > + */
> > +
> > +/**
> > + * \fn FrameBuffer::metadata()
> > + * \brief Retrieve the dynamic metadata
> > + * \return Dynamic metadata for the frame
>
> s/for/of/
Maybe "for the frame contained in the buffer" ?
> > + */
> > +
> > +/**
> > + * \fn FrameBuffer::cookie()
> > + * \brief Retrieve the integer cookie
> > + *
> > + * The cookie shall only be read by the creator of the FrameBuffer. The cookie
> > + * is transparent to libcamera core.
Nothing will break if someone else reads the cookie, and while it
wouldn't be very useful as the value would be meaningless, it could
still be done for instance for debugging purpose. I would write
* The cookie belongs to the creator of the FrameBuffer, which controls its
* lifetime and value.
*
* \sa setCookie()
> > + *
> > + * \return Integer cookie
> > + */
> > +
> > +/**
> > + * \fn FrameBuffer::setCookie()
> > + * \brief Set the integer cookie
> > + * \param[in] cookie Cookie to set
> > + *
> > + * The cookie shall only be set by the creator of the FrameBuffer. The cookie
> > + * is transparent to libcamera core.
How about
* 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.
> > + */
>
> Not sure "Integer cookie" is more appropriate than just "cookie"
Agreed, I would say "cookie" everywhere (except perhaps once at the top
in the description of the class).
> > +
> > +/**
> > + * \var FrameBuffer::request_
> > + * \brief The request this frame belongs to
s/frame/buffer/ ?
> > + *
> > + * This member is intended to be set by Request::prepare() and
> > + * Request::completeBuffer().
> > + */
> > +
> > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list