[libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add FrameBuffer interface
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 3 17:28:04 CET 2020
Hi Niklas,
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
> 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).
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);
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);
+ 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.
> +
> + 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
> + * 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)
> + *
> + * 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"
> + * 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 dynamic data, content of the memory planes and the metadata, is updated
> + * each time the buffer is successfully processed by a camera, 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 creator of a FrameBuffer (applications, IPA or pipeline) may associate
> + * 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.
> + */
> +
> +/**
> + * \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 ?
> + *
> + * Applications or IPAs can use this information to map the planes memory and
> + * access its content. The mapper of the memory is responsible for unmaping
> + * the memory before the FrameBuffer::Plane object is destroyed.
Isn't libcamera performing the mapping internally ?
> + *
> + * \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.
> + *
> + * 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.
> + *
> + * \brief The plane length
> + */
> +
> +/**
> + * \brief Create a libcamera FrameBuffer object from an array of planes
Connstruct a FrameBuffer with an array of planes
> + * \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
> + * 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
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;
> +}
> +
> +/**
> + * \fn FrameBuffer::planes()
> + * \brief Retrieve the static plane descriptions
> + * \return Array of plane descriptions
> + */
> +
> +/**
> + * \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::metadata()
> + * \brief Retrieve the dynamic metadata
> + * \return Dynamic metadata for the frame
s/for/of/
> + */
> +
> +/**
> + * \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.
> + *
> + * \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.
> + */
Not sure "Integer cookie" is more appropriate than just "cookie"
> +
> +/**
> + * \var FrameBuffer::request_
> + * \brief The request this frame belongs to
> + *
> + * This member is intended to be set by Request::prepare() and
> + * Request::completeBuffer().
> + */
> +
> } /* namespace libcamera */
> --
> 2.24.1
>
> _______________________________________________
> 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/20200103/31c3d973/attachment-0001.sig>
More information about the libcamera-devel
mailing list