[{"id":3316,"web_url":"https://patchwork.libcamera.org/comment/3316/","msgid":"<20200103162804.er36hzbyh2z3iili@uno.localdomain>","date":"2020-01-03T16:28:04","subject":"Re: [libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add\n\tFrameBuffer interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Dec 30, 2019 at 01:04:47PM +0100, Niklas Söderlund wrote:\n> Add a new FrameBuffer class to describe memory used to store frames.\n> This change just adds the new interface, future patches will migrate all\n> parts of libcamera to use this and replace the old Buffer interface.\n>\n> This change needs to specify the const keyword for Plane::length() as to\n> not get Doxygen confused with FrameBuffer::Plane::length added in this\n> change.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> Changes since v1:\n> - Rename FrameBuffer::info() to FrameBuffer::metadata()\n> - Fixup commit message\n> - Reorder method order\n> - Rewrite documentation\n> - Add operator==() and operator=!() for FrameBuffer::Plane\n> ---\n> include/libcamera/buffer.h | 32 +++++++++\n> src/libcamera/buffer.cpp | 132 ++++++++++++++++++++++++++++++++++++-\n> 2 files changed, 163 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 0b95e41a2dd205b2..862031123b4b510c 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -11,6 +11,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n\nit doesn't work forward-declaring it.. weird\n\n> namespace libcamera {\n>\n> class Request;\n> @@ -33,6 +35,36 @@ struct FrameMetadata {\n> \tstd::vector planes;\n> };\n>\n> +class FrameBuffer final\n> +{\n> +public:\n> +\tstruct Plane {\n> +\t\tFileDescriptor fd;\n> +\t\tunsigned int length;\n> +\t};\n> +\n> +\tFrameBuffer(const std::vector &planes = {}, unsigned int cookie = 0);\n\nThis planes = {} looks weird and not safe, as you would zero all your\nplane informations in the constructor.\n\nI tried removing it, and I got an error in rkisp1 IPA module, but I\nthink that part is not right, and so you need this workaround here.\n\nIt's possibly worth commenting on the patch that introduces that part,\nbut since I'm here already..\n\n\nSo if I remove the default = {} planes parameter value I get:\n\n[snip]\ntuple:1674:9: error: no matching constructor for initialization of 'libcamera::FrameBuffer'\n second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)\n[snip]\n../src/ipa/rkisp1/rkisp1.cpp:109:16: note: in instantiation of member function 'std::map, std::allocator > >::operator[]' requested here\n buffers_[buffer.id].planes()[0].length,\n\n\nThe culprit here seems to be in the following lines of rkisp1.cpp\n\nvoid IPARkISP1::mapBuffers(const std::vector &buffers)\n{\n\tfor (const IPABuffer &buffer : buffers) {\n\t\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n\t\t\t\t\t\t buffers_[buffer.id].planes()[0].length,\n\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n\t\t\t\t\t\t MAP_SHARED,\n\t\t\t\t\t\t buffers_[buffer.id].planes()[0].fd.fd(),\n\t\t\t\t\t\t 0);\n\n\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n\t\t\tint ret = -errno;\n\t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n\t\t\t\t\t << strerror(-ret);\n\t\t}\n\t}\n}\n\nIn\n\t\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n\nYou're first constructing a FrameBuffer, then calling the copy\nconstructor when emplacing, but that's unrelated with the error above.\n\nLater you access buffers_ using operator[] which the compiler\ncomplains about because if there is no such element with key=id it\ngets created, and the compiler fails to find an opportune\nconstructor (std::map::operator[] is nasty!)\n\nWith the below change, you can then remove the = {} and then delete\nthe FrameBuffer copy constructor as well (and possibly the assignement\noperator too).\n\ndiff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\nindex 9d4443cf56d4..e1ea97c7ec75 100644\n--- a/include/libcamera/buffer.h\n+++ b/include/libcamera/buffer.h\n@@ -41,7 +41,8 @@ public:\n unsigned int length;\n };\n\n- FrameBuffer(const std::vector &planes = {}, unsigned int cookie = 0);\n+ FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n+ FrameBuffer(const FrameBuffer &other) = delete;\n\n const std::vector &planes() const { return planes_; }\n\ndiff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\nindex 63776563b149..0538d356f6f9 100644\n--- a/src/ipa/rkisp1/rkisp1.cpp\n+++ b/src/ipa/rkisp1/rkisp1.cpp\n@@ -104,12 +104,14 @@ void IPARkISP1::configure(const std::map &streamConfig,\n void IPARkISP1::mapBuffers(const std::vector &buffers)\n {\n for (const IPABuffer &buffer : buffers) {\n- buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n+ buffers_.emplace(buffer.id, buffer.planes);\n+\n+ const FrameBuffer &b = buffers_.at(buffer.id);\n buffersMemory_[buffer.id] = mmap(NULL,\n- buffers_[buffer.id].planes()[0].length,\n+ b.planes()[0].length,\n PROT_READ | PROT_WRITE,\n MAP_SHARED,\n- buffers_[buffer.id].planes()[0].fd.fd(),\n+ b.planes()[0].fd.fd(),\n 0);\n\n if (buffersMemory_[buffer.id] == MAP_FAILED) {\n@@ -123,7 +125,11 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n void IPARkISP1::unmapBuffers(const std::vector &ids)\n {\n for (unsigned int id : ids) {\n- munmap(buffersMemory_[id], buffers_[id].planes()[0].length);\n+ const auto b = buffers_.find(id);\n+ if (b == buffers_.end())\n+ continue;\n+\n+ munmap(buffersMemory_[id], b->second.planes()[0].length);\n buffersMemory_.erase(id);\n buffers_.erase(id);\n }\n\nSorry if this mixes comments on two different patches.\n\n> +\n> +\tconst std::vector &planes() const { return planes_; }\n> +\n> +\tRequest *request() const { return request_; }\n> +\tconst FrameMetadata &metadata() const { return metadata_; };\n> +\n> +\tunsigned int cookie() const { return cookie_; }\n> +\tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> +\n> +private:\n> +\tfriend class Request; /* Needed to update request_. */\n> +\tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> +\n> +\tstd::vector planes_;\n> +\n> +\tRequest *request_;\n> +\tFrameMetadata metadata_;\n> +\n> +\tunsigned int cookie_;\n> +};\n> +\n> class Plane final\n> {\n> public:\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 7673b96f29728a24..7c78b9b8f1e90aa5 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -229,7 +229,7 @@ void *Plane::mem()\n> }\n>\n> /**\n> - * \\fn Plane::length()\n> + * \\fn Plane::length() const\n> * \\brief Retrieve the length of the memory region\n> * \\return The length of the memory region\n> */\n> @@ -462,4 +462,134 @@ void Buffer::cancel()\n> * The intended callers are Request::prepare() and Request::completeBuffer().\n> */\n>\n> +/**\n> + * \\class FrameBuffer\n> + * \\brief A buffer handle and dynamic metadata\n\ns/A buffer handle and/Frame buffer data and its associated/ ?\n> + *\n> + * The FrameBuffer class is the primary interface for applications, IPAs and\n> + * pipelines to interact with capture memory. It contains all static and\n> + * dynamic information to handle the whole life-cycle of creating, queueing,\n> + * capturing and consumption of a single frame capture.\n\ns/consumption/consuming ? given the usage of 'ing' in the other verbs\n? (I'm asking, not sure)\n\n> + *\n> + * The static information describes the one or more memory planes which makes\n\ns/makes/make\n\nand I would say \"planes of which an image frame is composed of\"\n\n> + * up the complete frame. The static information needs to be available at\n> + * creation of the FrameBuffer and needs to be expressed as a valid dmabuf file\n\nand needs to be/are\n\n> + * descriptor and length.\n> + *\n> + * The dynamic data, content of the memory planes and the metadata, is updated\n> + * each time the buffer is successfully processed by a camera, by queueing it as\n> + * part of a request. The dynamic data is only valid after the cameras buffer\n> + * complete signal have been raised and until the FrameBuffer is queued once\n\ns/raised/emitted\n\n> + * more to the camera as part of a new request.\n\ns/once more/again\n\n> + *\n> + * The creator of a FrameBuffer (applications, IPA or pipeline) may associate\n> + * an integer cookie with the instance at any time to aid in its usage by the\n> + * object creator. This cookie is transparent to libcamera core and shall only\n\nI get what you mean but it's a bit convoluted.\n\n> + * be set and consumed by the user. This supplements the Request objects cookie.\n> + */\n> +\n> +/**\n> + * \\struct FrameBuffer::Plane\n> + * \\brief A description of a memory plane\n> + *\n> + * A memory plane is described by a dmabuf file descriptor and a plane length.\n\ndmabuf handle ?\n\n> + *\n> + * Applications or IPAs can use this information to map the planes memory and\n> + * access its content. The mapper of the memory is responsible for unmaping\n> + * the memory before the FrameBuffer::Plane object is destroyed.\n\nIsn't libcamera performing the mapping internally ?\n\n> + *\n> + * \\todo Once we have a Kernel API which can express offsets within a plane\n> + * this structure shall be extended to contain this information.\n> + */\n> +\n> +/**\n> + * \\var FrameBuffer::Plane::fd\n> + * \\brief The dmabuf file descriptor\n> + */\n> +\n> +/**\n> + * \\var FrameBuffer::Plane::length\n> + *\n> + * The length value is only used if the memory of the plane needs to be mapped.\n> + * Therefor a length of zero is valid for planes allocated outside libcamera and\n> + * queued to a Camera as part of a Request, libcamera will never poke inside\n> + * that memory and thus do not need to know its length.\n> + *\n> + * All planes created by pipeline handlers however need to have a correct length\n> + * set as IPAs and other parts of libcamera core might wish to memory map the\n> + * plane.\n> + *\n> + * \\brief The plane length\n> + */\n> +\n> +/**\n> + * \\brief Create a libcamera FrameBuffer object from an array of planes\n\nConnstruct a FrameBuffer with an array of planes\n\n> + * \\param[in] planes Planes that makes up the FrameBuffer\n\nsimnply \"The frame memory planes\" ?\n\n> + * \\param[in] cookie Integer cookie\n> + *\n> + * Create a FrameBuffer object from an array of static plane descriptions. The\n> + * creator may close the file descriptors in the plane description after the\n\ncreator/caller ?\n\nI would instead:\n\"The file descriptor associated with each plane are duplicated and can\nbe closed by the caller after the FrameBuffer has been constructed\"\n\n\n> + * FrameBuffer have been created.\n> + */\n> +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> +\t: request_(nullptr), cookie_(cookie)\n> +{\n> +\t/* Clone all file descriptors. */\n\nDuplicate or clone ?\n\n> +\tplanes_ = planes;\n> +}\n> +\n> +/**\n> + * \\fn FrameBuffer::planes()\n> + * \\brief Retrieve the static plane descriptions\n> + * \\return Array of plane descriptions\n> + */\n> +\n> +/**\n> + * \\fn FrameBuffer::request()\n> + * \\brief Retrieve the request this buffer belongs to\n> + *\n> + * The intended callers of this method are buffer completion handlers that\n> + * need to associate a buffer to the request it belongs to.\n> + *\n> + * A Buffer is associated to a request by Request::prepare() and the\n> + * association is valid until the buffer completes. The returned request\n> + * pointer is valid only during that interval.\n> + *\n> + * \\return The Request the Buffer belongs to, or nullptr if the buffer is\n> + * either completed or not associated with a request\n> + */\n> +\n> +/**\n> + * \\fn FrameBuffer::metadata()\n> + * \\brief Retrieve the dynamic metadata\n> + * \\return Dynamic metadata for the frame\n\ns/for/of/\n\n> + */\n> +\n> +/**\n> + * \\fn FrameBuffer::cookie()\n> + * \\brief Retrieve the integer cookie\n> + *\n> + * The cookie shall only be read by the creator of the FrameBuffer. The cookie\n> + * is transparent to libcamera core.\n> + *\n> + * \\return Integer cookie\n> + */\n> +\n> +/**\n> + * \\fn FrameBuffer::setCookie()\n> + * \\brief Set the integer cookie\n> + * \\param[in] cookie Cookie to set\n> + *\n> + * The cookie shall only be set by the creator of the FrameBuffer. The cookie\n> + * is transparent to libcamera core.\n> + */\n\nNot sure \"Integer cookie\" is more appropriate than just \"cookie\"\n\n> +\n> +/**\n> + * \\var FrameBuffer::request_\n> + * \\brief The request this frame belongs to\n> + *\n> + * This member is intended to be set by Request::prepare() and\n> + * Request::completeBuffer().\n> + */\n> +\n> } /* namespace libcamera */\n> --\n> 2.24.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65190605CD\n\tfor ;\n\tFri, 3 Jan 2020 17:25:46 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id E447320002;\n\tFri, 3 Jan 2020 16:25:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 3 Jan 2020 17:28:04 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200103162804.er36hzbyh2z3iili@uno.localdomain>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"je54qmcpc3jxomay\"","Content-Disposition":"inline","In-Reply-To":"<20191230120510.938333-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add\n\tFrameBuffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 03 Jan 2020 16:25:46 -0000"}},{"id":3334,"web_url":"https://patchwork.libcamera.org/comment/3334/","msgid":"<20200107023552.GF22189@pendragon.ideasonboard.com>","date":"2020-01-07T02:35:52","subject":"Re: [libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add\n\tFrameBuffer interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Jan 03, 2020 at 05:28:04PM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 30, 2019 at 01:04:47PM +0100, Niklas Söderlund wrote:\n> > Add a new FrameBuffer class to describe memory used to store frames.\n> > This change just adds the new interface, future patches will migrate all\n> > parts of libcamera to use this and replace the old Buffer interface.\n> >\n> > This change needs to specify the const keyword for Plane::length() as to\n> > not get Doxygen confused with FrameBuffer::Plane::length added in this\n> > change.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > Changes since v1:\n> > - Rename FrameBuffer::info() to FrameBuffer::metadata()\n> > - Fixup commit message\n> > - Reorder method order\n> > - Rewrite documentation\n> > - Add operator==() and operator=!() for FrameBuffer::Plane\n> > ---\n> > include/libcamera/buffer.h | 32 +++++++++\n> > src/libcamera/buffer.cpp | 132 ++++++++++++++++++++++++++++++++++++-\n> > 2 files changed, 163 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 0b95e41a2dd205b2..862031123b4b510c 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -11,6 +11,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> \n> it doesn't work forward-declaring it.. weird\n\nThat's because FileDescriptor is embedded in FrameBuffer::Plane, so the\ncompiler needs the full definition.\n\n> > namespace libcamera {\n> >\n> > class Request;\n> > @@ -33,6 +35,36 @@ struct FrameMetadata {\n> > \tstd::vector planes;\n> > };\n> >\n> > +class FrameBuffer final\n> > +{\n> > +public:\n> > +\tstruct Plane {\n> > +\t\tFileDescriptor fd;\n> > +\t\tunsigned int length;\n> > +\t};\n> > +\n> > +\tFrameBuffer(const std::vector &planes = {}, unsigned int cookie = 0);\n> \n> This planes = {} looks weird and not safe, as you would zero all your\n> plane informations in the constructor.\n> \n> I tried removing it, and I got an error in rkisp1 IPA module, but I\n> think that part is not right, and so you need this workaround here.\n> \n> It's possibly worth commenting on the patch that introduces that part,\n> but since I'm here already..\n> \n> \n> So if I remove the default = {} planes parameter value I get:\n> \n> [snip]\n> tuple:1674:9: error: no matching constructor for initialization of 'libcamera::FrameBuffer'\n> second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)\n> [snip]\n> ../src/ipa/rkisp1/rkisp1.cpp:109:16: note: in instantiation of member function 'std::map, std::allocator > >::operator[]' requested here\n> buffers_[buffer.id].planes()[0].length,\n> \n> \n> The culprit here seems to be in the following lines of rkisp1.cpp\n> \n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> {\n> \tfor (const IPABuffer &buffer : buffers) {\n> \t\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n> \t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> \t\t\t\t\t\t buffers_[buffer.id].planes()[0].length,\n> \t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> \t\t\t\t\t\t MAP_SHARED,\n> \t\t\t\t\t\t buffers_[buffer.id].planes()[0].fd.fd(),\n> \t\t\t\t\t\t 0);\n> \n> \t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> \t\t\tint ret = -errno;\n> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> \t\t\t\t\t << strerror(-ret);\n> \t\t}\n> \t}\n> }\n> \n> In\n> \t\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n> \n> You're first constructing a FrameBuffer, then calling the copy\n> constructor when emplacing, but that's unrelated with the error above.\n> \n> Later you access buffers_ using operator[] which the compiler\n> complains about because if there is no such element with key=id it\n> gets created, and the compiler fails to find an opportune\n> constructor (std::map::operator[] is nasty!)\n> \n> With the below change, you can then remove the = {} and then delete\n> the FrameBuffer copy constructor as well (and possibly the assignement\n> operator too).\n\nThe assignment operator should be deleted too, as classes should have\nboth a copy constructor and a copy assignment operator, or neither\n(https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all).\n\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 9d4443cf56d4..e1ea97c7ec75 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -41,7 +41,8 @@ public:\n> unsigned int length;\n> };\n> \n> - FrameBuffer(const std::vector &planes = {}, unsigned int cookie = 0);\n> + FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> + FrameBuffer(const FrameBuffer &other) = delete;\n> \n> const std::vector &planes() const { return planes_; }\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 63776563b149..0538d356f6f9 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -104,12 +104,14 @@ void IPARkISP1::configure(const std::map &streamConfig,\n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> {\n> for (const IPABuffer &buffer : buffers) {\n> - buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n> + buffers_.emplace(buffer.id, buffer.planes);\n> +\n> + const FrameBuffer &b = buffers_.at(buffer.id);\n\ns/b/fb/\n\nYou can also use the fact that emplate returns an iterator:\n\n\t\tconst FrameBuffer &fb = buffers_.emplace(buffer.id, buffer.planes).first->second;\n\nor\n\n\t\tauto elem = buffers_.emplace(buffer.id, buffer.planes);\n\t\tconst FrameBuffer &fb = elem.first->second;\n\n> buffersMemory_[buffer.id] = mmap(NULL,\n> - buffers_[buffer.id].planes()[0].length,\n> + b.planes()[0].length,\n> PROT_READ | PROT_WRITE,\n> MAP_SHARED,\n> - buffers_[buffer.id].planes()[0].fd.fd(),\n> + b.planes()[0].fd.fd(),\n> 0);\n> \n> if (buffersMemory_[buffer.id] == MAP_FAILED) {\n> @@ -123,7 +125,11 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n> void IPARkISP1::unmapBuffers(const std::vector &ids)\n> {\n> for (unsigned int id : ids) {\n> - munmap(buffersMemory_[id], buffers_[id].planes()[0].length);\n> + const auto b = buffers_.find(id);\n\ns/b/fd/ here too.\n\n> + if (b == buffers_.end())\n> + continue;\n> +\n> + munmap(buffersMemory_[id], b->second.planes()[0].length);\n> buffersMemory_.erase(id);\n> buffers_.erase(id);\n> }\n> \n> Sorry if this mixes comments on two different patches.\n\nI like this proposal :-)\n\n> > +\n> > +\tconst std::vector &planes() const { return planes_; }\n> > +\n> > +\tRequest *request() const { return request_; }\n> > +\tconst FrameMetadata &metadata() const { return metadata_; };\n> > +\n> > +\tunsigned int cookie() const { return cookie_; }\n> > +\tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > +\n> > +private:\n> > +\tfriend class Request; /* Needed to update request_. */\n> > +\tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> > +\n> > +\tstd::vector planes_;\n> > +\n> > +\tRequest *request_;\n> > +\tFrameMetadata metadata_;\n> > +\n> > +\tunsigned int cookie_;\n> > +};\n> > +\n> > class Plane final\n> > {\n> > public:\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 7673b96f29728a24..7c78b9b8f1e90aa5 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -229,7 +229,7 @@ void *Plane::mem()\n> > }\n> >\n> > /**\n> > - * \\fn Plane::length()\n> > + * \\fn Plane::length() const\n> > * \\brief Retrieve the length of the memory region\n> > * \\return The length of the memory region\n> > */\n> > @@ -462,4 +462,134 @@ void Buffer::cancel()\n> > * The intended callers are Request::prepare() and Request::completeBuffer().\n> > */\n> >\n> > +/**\n> > + * \\class FrameBuffer\n> > + * \\brief A buffer handle and dynamic metadata\n> \n> s/A buffer handle and/Frame buffer data and its associated/ ?\n> > + *\n> > + * The FrameBuffer class is the primary interface for applications, IPAs and\n> > + * pipelines to interact with capture memory. It contains all static and\n\ns/capture/frame/ (as we'll support reprocessing at some point).\n\n> > + * dynamic information to handle the whole life-cycle of creating, queueing,\n> > + * capturing and consumption of a single frame capture.\n> \n> s/consumption/consuming ? given the usage of 'ing' in the other verbs\n> ? (I'm asking, not sure)\n\nIn which case s/of a/a/. I think this needs to be slightly rephrased as\n\"creating [...] of a single frame capture\" doesn't sound right.\n\n\"It contains all the static and dynamic information to manage the whole\nlife cycle of a frame capture, from buffer creation to consumption.\" ?\n\n> > + *\n> > + * The static information describes the one or more memory planes which makes\n>\n> s/makes/make\n> \n> and I would say \"planes of which an image frame is composed of\"\n\nThat sounds very heavy.\n\n\"The static information describes the memory planes that make a frame\",\nor \"... that store the content of a frame\".\n\n> > + * up the complete frame. The static information needs to be available at\n> > + * creation of the FrameBuffer and needs to be expressed as a valid dmabuf file\n> \n> and needs to be/are\n> \n> > + * descriptor and length.\n\n\"The planes are specified when creating the FrameBuffer and are\nexpressed as a set of dmabuf file descriptors and length.\"\n\n(I think we can omit \"valid\", it's implicit, isn't it ?)\n\n> > + *\n> > + * The dynamic data, content of the memory planes and the metadata, is updated\n\nI'm not sure I would mention content of the memory planes here (assuming\nyou mean the frame pixel data).\n\n> > + * each time the buffer is successfully processed by a camera, by queueing it as\n> > + * part of a request.\n\nThis sounds to me that \"the data [...] is updated [...] by queueing it\nas part of a request.\".\n\n> The dynamic data is only valid after the cameras buffer\n> > + * complete signal have been raised and until the FrameBuffer is queued once\n> \n> s/raised/emitted\n> \n> > + * more to the camera as part of a new request.\n> \n> s/once more/again\n\n * The dynamic information is grouped in a FrameMetadata instance. It is updated\n * during the processing of a queued capture request, and is valid from the\n * completion of the buffer as signaled by Camera::bufferComplete() until the\n * FrameBuffer is either reused in a new request or deleted.\n\n> > + *\n> > + * The creator of a FrameBuffer (applications, IPA or pipeline) may associate\n\ns/applications/application/\n\n> > + * an integer cookie with the instance at any time to aid in its usage by the\n> > + * object creator. This cookie is transparent to libcamera core and shall only\n> \n> I get what you mean but it's a bit convoluted.\n> \n> > + * be set and consumed by the user. This supplements the Request objects cookie.\n\ns/objects/object's/ or just drop \"objects\".\n\n\"may associate to it an integer cookie for any private purpose. The\ncookie may be set when creating the FrameBuffer, and updated at any time\nwith setCookie(). The cookie is transparent to the libcamera core and\nshall only be set by the creator of the FrameBuffer. This mechanism\nsupplements the Request cookie.\"\n\n> > + */\n> > +\n> > +/**\n> > + * \\struct FrameBuffer::Plane\n> > + * \\brief A description of a memory plane\n> > + *\n> > + * A memory plane is described by a dmabuf file descriptor and a plane length.\n> \n> dmabuf handle ?\n\ns/plane length/length/\n\n> > + *\n> > + * Applications or IPAs can use this information to map the planes memory and\n\ns/planes/plane/\n\n> > + * access its content. The mapper of the memory is responsible for unmaping\n> > + * the memory before the FrameBuffer::Plane object is destroyed.\n\nIs that strictly required ?\n\n> Isn't libcamera performing the mapping internally ?\n\nNot anymore as far as I can tell. I'm tempted to say that this paragraph\nis out of scope as we don't care what users do with the dmabuf fd, but\nwe may get puzzled users asking how they can access the content of the\nplanes (and I think we need some kind of helper for this, but it can be\nimplemented on top).\n\n> > + *\n> > + * \\todo Once we have a Kernel API which can express offsets within a plane\n> > + * this structure shall be extended to contain this information.\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameBuffer::Plane::fd\n> > + * \\brief The dmabuf file descriptor\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameBuffer::Plane::length\n> > + *\n> > + * The length value is only used if the memory of the plane needs to be mapped.\n> > + * Therefor a length of zero is valid for planes allocated outside libcamera and\n> > + * queued to a Camera as part of a Request, libcamera will never poke inside\n> > + * that memory and thus do not need to know its length.\n\nHow will we inform V4L2 when queueing the imported dmabuf then ? I think\nthe length should always be valid.\n\n> > + *\n> > + * All planes created by pipeline handlers however need to have a correct length\n> > + * set as IPAs and other parts of libcamera core might wish to memory map the\n> > + * plane.\n\nI think we can drop all this.\n\n> > + *\n> > + * \\brief The plane length\n\n\"The plane length in bytes\"\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a libcamera FrameBuffer object from an array of planes\n> \n> Connstruct a FrameBuffer with an array of planes\n\ns/Connstruct/Construct/\n\n> > + * \\param[in] planes Planes that makes up the FrameBuffer\n> \n> simnply \"The frame memory planes\" ?\n> \n> > + * \\param[in] cookie Integer cookie\n> > + *\n> > + * Create a FrameBuffer object from an array of static plane descriptions. The\n\ns/object/instance/ ?\ns/static plane descriptions/planes/ ?\n\n> > + * creator may close the file descriptors in the plane description after the\n> \n> creator/caller ?\n> \n> I would instead:\n> \"The file descriptor associated with each plane are duplicated and can\n\ns/descriptor/descriptors/\n\n> be closed by the caller after the FrameBuffer has been constructed\"\n\n> > + * FrameBuffer have been created.\n> > + */\n> > +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > +\t: request_(nullptr), cookie_(cookie)\n> > +{\n> > +\t/* Clone all file descriptors. */\n> \n> Duplicate or clone ?\n> \n> > +\tplanes_ = planes;\n\nCan't you do this as part of the initializers ?\n\n> > +}\n\nI think we also need a move constructor. If you look at the usage of\nthis class in V4L2VideoDevice::createFrameBuffer(), the fds from the\nplanes vector are duplicated when creating the FrameBuffer and then\nimmediately closed as planes gets out of scope. A move version of this\nconstructor would solve this issue.\n\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::planes()\n> > + * \\brief Retrieve the static plane descriptions\n> > + * \\return Array of plane descriptions\n\n\"descriptions\" or \"descriptors\" ? I like \"descriptors\" better as it\nmatches the dmabuf file descriptor.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::request()\n> > + * \\brief Retrieve the request this buffer belongs to\n> > + *\n> > + * The intended callers of this method are buffer completion handlers that\n> > + * need to associate a buffer to the request it belongs to.\n> > + *\n> > + * A Buffer is associated to a request by Request::prepare() and the\n\ns/Buffer/FrameBuffer/\n\nRequest::prepare() is a private method. Why don't we associate the\nbuffer in addBuffer() ? We need a user-facing API here.\n\n> > + * association is valid until the buffer completes. The returned request\n> > + * pointer is valid only during that interval.\n\n\"until the buffer completes\" may be ambiguous, does it mean before or\nafter emitting the buffer completion signal ? I know it's the latter,\nbut we should be explicit here. \"until the buffer has completed\" may be\nslightly better. \"until after the buffer has completed\" ? Feel free to\npropose a better wording.\n\n> > + *\n> > + * \\return The Request the Buffer belongs to, or nullptr if the buffer is\n> > + * either completed or not associated with a request\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::metadata()\n> > + * \\brief Retrieve the dynamic metadata\n> > + * \\return Dynamic metadata for the frame\n> \n> s/for/of/\n\nMaybe \"for the frame contained in the buffer\" ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::cookie()\n> > + * \\brief Retrieve the integer cookie\n> > + *\n> > + * The cookie shall only be read by the creator of the FrameBuffer. The cookie\n> > + * is transparent to libcamera core.\n\nNothing will break if someone else reads the cookie, and while it\nwouldn't be very useful as the value would be meaningless, it could\nstill be done for instance for debugging purpose. I would write\n\n * The cookie belongs to the creator of the FrameBuffer, which controls its\n * lifetime and value.\n *\n * \\sa setCookie()\n\n> > + *\n> > + * \\return Integer cookie\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::setCookie()\n> > + * \\brief Set the integer cookie\n> > + * \\param[in] cookie Cookie to set\n> > + *\n> > + * The cookie shall only be set by the creator of the FrameBuffer. The cookie\n> > + * is transparent to libcamera core.\n\nHow about\n\n * The cookie belongs to the creator of the FrameBuffer. Its value may be\n * modified at any time with this method. Applications and IPAs shall not modify\n * the cookie value of buffers they haven't created themselves. The libcamera\n * core never modifies the buffer cookie.\n\n> > + */\n> \n> Not sure \"Integer cookie\" is more appropriate than just \"cookie\"\n\nAgreed, I would say \"cookie\" everywhere (except perhaps once at the top\nin the description of the class).\n\n> > +\n> > +/**\n> > + * \\var FrameBuffer::request_\n> > + * \\brief The request this frame belongs to\n\ns/frame/buffer/ ?\n\n> > + *\n> > + * This member is intended to be set by Request::prepare() and\n> > + * Request::completeBuffer().\n> > + */\n> > +\n> > } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D5BD6045E\n\tfor ;\n\tTue, 7 Jan 2020 03:36:03 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC6BC52F;\n\tTue, 7 Jan 2020 03:36:02 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578364562;\n\tbh=ifKkZVJRKD/vwmSFeenT0/huli+4blvmwAY4Cy5NaIs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YGA2vSk5cyG9wMWxnxuNPGEEVxMpglu1S5kTCGvDALHUDyvqs/h/7dbRdhlxvvk7F\n\trLbOE9Hfx5QDPqRPx/pRFs76v3pMlPK/Y+dDhSk5vpMp1Z6h8IyhHudnj5VGF70E7X\n\tOiGk+/kjIAXxwr8ngoEsuJOwZUEeTEf8btnlckZE=","Date":"Tue, 7 Jan 2020 04:35:52 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200107023552.GF22189@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-3-niklas.soderlund@ragnatech.se>\n\t<20200103162804.er36hzbyh2z3iili@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200103162804.er36hzbyh2z3iili@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 02/25] libcamera: buffer: Add\n\tFrameBuffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 07 Jan 2020 02:36:03 -0000"}}]