[{"id":3168,"web_url":"https://patchwork.libcamera.org/comment/3168/","msgid":"<20191202120232.5hgygvem6aygbulg@uno.localdomain>","date":"2019-12-02T12:02:32","subject":"Re: [libcamera-devel] [PATCH 20/30] libcamera: stream: Add\n\tprototypes for new interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:36:10AM +0100, Niklas Söderlund wrote:\n> The buffer allocation rework will remove most of the methods in the\n> Stream class. This change adds the prototypes for the FrameBuffer\n> interface with stub default implementations.\n>\n> Once the new buffer allocation work is completed the prototypes added in\n> this change will be turned into pure virtual functions preventing\n> the base Stream class from being instantiated.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/stream.h | 11 ++++++++\n> src/libcamera/stream.cpp | 55 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 66 insertions(+)\n>\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index a404eccf34d9c93b..395148e45d342d92 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_STREAM_H__\n> #define __LIBCAMERA_STREAM_H__\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -74,6 +75,7 @@ class Stream\n> {\n> public:\n> \tStream();\n> +\tvirtual ~Stream(){};\n\nYou can add this when you'll make the methods virtual... anyway, it's\nfine\n\n>\n> \tstd::unique_ptr createBuffer(unsigned int index);\n> \tstd::unique_ptr createBuffer(const std::array &fds);\n> @@ -86,6 +88,15 @@ public:\n> protected:\n> \tfriend class Camera;\n>\n> +\tvirtual int allocateBuffers(const StreamConfiguration &config,\n> +\t\t\t\t std::vector *buffers)\n> +\t{\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tvirtual void releaseBuffers() { return; }\n> +\tvirtual int start() { return -EINVAL; }\n> +\tvirtual void stop() { return; }\n> +\n> \tint mapBuffer(const Buffer *buffer);\n> \tvoid unmapBuffer(const Buffer *buffer);\n>\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index e70a1e307ecaa5ba..ba3f571b08cb0c41 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -520,6 +520,61 @@ std::unique_ptr Stream::createBuffer(const std::array &fds)\n> * \\return The memory type used by the stream\n> */\n>\n> +/**\n> + * \\fn Stream::allocateBuffers()\n> + * \\brief Allocate buffers from the stream\n\nfrom or for ?\n\n> + * \\param[in] config A configuration describing the buffer(s) to be allocated\n> + * \\param[out] buffers Array of buffers successfully allocated\n\nvector or array ?\n\n\"succesfully allocate buffers\"\n\nAs the vector is emtpy if allocation fails, do you need \"successfully\" ?\n\n> + *\n> + * Allocate buffers matching exactly what is described in \\a config and return\n> + * then in \\a buffers. If buffers matching \\a config can't be allocated an error\n> + * shall be returned and no buffers returned in \\a buffers.\n\nI would\n\nAllocate buffers for the Stream using the provided configuration. If\nthe allocated buffer parameters cannot match the requested\nconfiguration an error is returned and the \\a buffers vector is empty.\n\n> + *\n> + * This is a helper which may be used by libcamera helper classes to allocate\nan helper\n\n> + * buffers from the stream itself. The allocated buffers may then be treated\n> + * in the same way as if they where externally allocated.\n\nNot sure I got what you mean (and helper is repeated twice in 2 lines)\n\n> + *\n> + * The only intended caller is buffer allocator helpers and the function must\n> + * be implemeted by all subclasses of Stream.\n\nThis last part is implied by the fact the operation is a pure virtual\none. Same for all the other comments.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Stream::releaseBuffers()\n> + * \\brief Relase buffers allocated from the stram\n\nfor or from ?\n\n> + *\n> + * This is a helper which release buffers allocated using allocateBuffers(). The\nan helper\n\n> + * only intended caller is buffer allocator helpers.\n\ns/helper//\n\n> + *\n> + * The only intended caller is buffer allocator helpers and the function must\n> + * be implemeted by all subclasses of Stream.\n\nimplied\n\n> + */\n> +\n> +/**\n> + * \\fn Stream::start()\n> + * \\brief Prepare a stream for capture\n> + *\n> + * The subclss shall prepare the stream for capture and if needed allocate\n> + * resources to allow for that. No buffers may be allocated from the stream\n\ns/may/can\n\n> + * using allocateBuffers() after a stream have been started.\n\ns/have/has\n\n> + *\n> + * The only intended caller is the camera base class and the function must be\n> + * implemeted by all subclasses of Stream.\n\nimplied\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Stream::stop()\n> + * \\brief Stop a stream after capture\n> + *\n> + * The subclass shall free all resources allocated in start().\n\nI would remove the use of \"subclass\" from these two methods\ndescription.\n\n> + *\n> + * The only intended caller is the camera base class and the function must be\n> + * implemeted by all subclasses of Stream.\n> + */\n> +\n> /**\n> * \\brief Map a Buffer to a buffer memory index\n> * \\param[in] buffer The buffer to map to a buffer memory index\n> --\n> 2.24.0\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 relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF31C60BC4\n\tfor ;\n\tMon, 2 Dec 2019 13:00:23 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 4D07E20000A;\n\tMon, 2 Dec 2019 12:00:23 +0000 (UTC)"],"Date":"Mon, 2 Dec 2019 13:02:32 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191202120232.5hgygvem6aygbulg@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-21-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vuwv3oh74c6kewdb\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-21-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 20/30] libcamera: stream: Add\n\tprototypes for new 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":"Mon, 02 Dec 2019 12:00:24 -0000"}},{"id":3257,"web_url":"https://patchwork.libcamera.org/comment/3257/","msgid":"<20191215095820.GB5786@pendragon.ideasonboard.com>","date":"2019-12-15T09:58:20","subject":"Re: [libcamera-devel] [PATCH 20/30] libcamera: stream: Add\n\tprototypes for new 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 Mon, Dec 02, 2019 at 01:02:32PM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:36:10AM +0100, Niklas Söderlund wrote:\n> > The buffer allocation rework will remove most of the methods in the\n> > Stream class. This change adds the prototypes for the FrameBuffer\n> > interface with stub default implementations.\n> >\n> > Once the new buffer allocation work is completed the prototypes added in\n> > this change will be turned into pure virtual functions preventing\n> > the base Stream class from being instantiated.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/stream.h | 11 ++++++++\n> > src/libcamera/stream.cpp | 55 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 66 insertions(+)\n> >\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index a404eccf34d9c93b..395148e45d342d92 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_STREAM_H__\n> > #define __LIBCAMERA_STREAM_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -74,6 +75,7 @@ class Stream\n> > {\n> > public:\n> > \tStream();\n> > +\tvirtual ~Stream(){};\n> \n> You can add this when you'll make the methods virtual... anyway, it's\n> fine\n\nNo need for a semicolon at the end of the line, missing space before {},\nand it's better to define destructors in .cpp files instead of inlining\nthem (although in this case the inlined destructor should be fairly\nsmall, but as it's not in a hot path, inlining isn't required anyway).\n\n> >\n> > \tstd::unique_ptr createBuffer(unsigned int index);\n> > \tstd::unique_ptr createBuffer(const std::array &fds);\n> > @@ -86,6 +88,15 @@ public:\n> > protected:\n> > \tfriend class Camera;\n> >\n> > +\tvirtual int allocateBuffers(const StreamConfiguration &config,\n> > +\t\t\t\t std::vector *buffers)\n> > +\t{\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tvirtual void releaseBuffers() { return; }\n> > +\tvirtual int start() { return -EINVAL; }\n> > +\tvirtual void stop() { return; }\n\nNo need for a return in releaseBuffers() and stop().\n\n> > +\n> > \tint mapBuffer(const Buffer *buffer);\n> > \tvoid unmapBuffer(const Buffer *buffer);\n> >\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index e70a1e307ecaa5ba..ba3f571b08cb0c41 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -520,6 +520,61 @@ std::unique_ptr Stream::createBuffer(const std::array &fds)\n> > * \\return The memory type used by the stream\n> > */\n> >\n> > +/**\n> > + * \\fn Stream::allocateBuffers()\n> > + * \\brief Allocate buffers from the stream\n> \n> from or for ?\n> \n> > + * \\param[in] config A configuration describing the buffer(s) to be allocated\n> > + * \\param[out] buffers Array of buffers successfully allocated\n> \n> vector or array ?\n> \n> \"succesfully allocate buffers\"\n> \n> As the vector is emtpy if allocation fails, do you need \"successfully\" ?\n> \n> > + *\n> > + * Allocate buffers matching exactly what is described in \\a config and return\n> > + * then in \\a buffers. If buffers matching \\a config can't be allocated an error\n> > + * shall be returned and no buffers returned in \\a buffers.\n> \n> I would\n> \n> Allocate buffers for the Stream using the provided configuration. If\n> the allocated buffer parameters cannot match the requested\n> configuration an error is returned and the \\a buffers vector is empty.\n> \n> > + *\n> > + * This is a helper which may be used by libcamera helper classes to allocate\n> an helper\n> \n> > + * buffers from the stream itself. The allocated buffers may then be treated\n> > + * in the same way as if they where externally allocated.\n> \n> Not sure I got what you mean (and helper is repeated twice in 2 lines)\n> \n> > + *\n> > + * The only intended caller is buffer allocator helpers and the function must\n> > + * be implemeted by all subclasses of Stream.\n> \n> This last part is implied by the fact the operation is a pure virtual\n> one. Same for all the other comments.\n> \n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn Stream::releaseBuffers()\n> > + * \\brief Relase buffers allocated from the stram\n> \n> for or from ?\n> \n> > + *\n> > + * This is a helper which release buffers allocated using allocateBuffers(). The\n> an helper\n> \n> > + * only intended caller is buffer allocator helpers.\n> \n> s/helper//\n> \n> > + *\n> > + * The only intended caller is buffer allocator helpers and the function must\n> > + * be implemeted by all subclasses of Stream.\n> \n> implied\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn Stream::start()\n> > + * \\brief Prepare a stream for capture\n> > + *\n> > + * The subclss shall prepare the stream for capture and if needed allocate\n> > + * resources to allow for that. No buffers may be allocated from the stream\n> \n> s/may/can\n> \n> > + * using allocateBuffers() after a stream have been started.\n> \n> s/have/has\n> \n> > + *\n> > + * The only intended caller is the camera base class and the function must be\n> > + * implemeted by all subclasses of Stream.\n> \n> implied\n> \n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn Stream::stop()\n> > + * \\brief Stop a stream after capture\n> > + *\n> > + * The subclass shall free all resources allocated in start().\n> \n> I would remove the use of \"subclass\" from these two methods\n> description.\n> \n> > + *\n> > + * The only intended caller is the camera base class and the function must be\n> > + * implemeted by all subclasses of Stream.\n> > + */\n\nAfter reading this I'm afraid I still don't like this interface and how\nit gets used :-S The good news, however, is that this is the only part I\ndislike in the series :-)\n\nThere are several issues that make me squirm. First of all, the start()\nand stop() methods don't start or stop a stream. This may just be a\nmatter of naming, but how they're automatically called from\nCamera::start() and Camera::stop() in patch 26/30 also fail to convince\nme. This removes control from pipeline handlers of when buffers get\nallocated or released for internal buffers, creating a special case for\nthe video node at the end of the pipeline. The fact that this isn't\nclearly documented also shows in my opinion that the interface is so\nspecial that we have trouble explaining it clearly.\n\nThe second issue, which is bigger in my opinion, is that pipeline\nhandlers need, with this interface, to decide at the time cameras are\ncreated which V4L2 video node will face applications. This may work\ntoday, but as soon as we'll try, for instance, to support raw capture,\nwe'll be in trouble as the video node will need to be selected based on\nthe camera configuration (using the raw capture video node for the\nRkISP1 pipeline handler and bypassing the ImgU for the IPU3 pipeline\nhandler).\n\nFor these reasons I think a different interface is needed, replacing\npatches 20/30 to 23/30 and 26/30.\n\n> > +\n> > /**\n> > * \\brief Map a Buffer to a buffer memory index\n> > * \\param[in] buffer The buffer to map to a buffer memory index","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 4B2C1601E4\n\tfor ;\n\tSun, 15 Dec 2019 10:58:30 +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 A394C2D1;\n\tSun, 15 Dec 2019 10:58:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576403909;\n\tbh=N/ZZY6T+zG614U5vbRUHzQW4b7JQ9tGeHPq6DH3JZOU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PmOEAYwFtVXsGGbMpMS/2HexPlINiRMC2LUSYWwl0Ptvk7+gXI3KBoHkMHHocuv2N\n\toRL/rf3mrSDm9zXRPzb8noeUBKlF4q1NOX0y+1A/YQt3jFfwjna8nK3deAD6zUFg82\n\t88vyJ4HrxF/rDqsd2+5dbGvX8+2tNbWcW1cr/apM=","Date":"Sun, 15 Dec 2019 11:58:20 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191215095820.GB5786@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-21-niklas.soderlund@ragnatech.se>\n\t<20191202120232.5hgygvem6aygbulg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191202120232.5hgygvem6aygbulg@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 20/30] libcamera: stream: Add\n\tprototypes for new 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":"Sun, 15 Dec 2019 09:58:30 -0000"}}]