[{"id":3020,"web_url":"https://patchwork.libcamera.org/comment/3020/","msgid":"<20191115151156.rjqsrzhdq5vqs5ez@uno.localdomain>","date":"2019-11-15T15:11:56","subject":"Re: [libcamera-devel] [RFC 08/12] libcamera: buffer: Add a buffer\n\tallocator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Oct 28, 2019 at 03:25:21AM +0100, Niklas Söderlund wrote:\n> From an applications point of view buffers shall not be allocated\n> directly by a camera or stream. Instead they shall be allocated\n> externally and used by a camera.\n>\n> To model this behavior add a buffer allocator that is exposed to\n> applications. How the allocator creates buffers is pipeline specific and\n> handled by sub-classing the stream class.\n\nI would dare to question the whole allocator idea here... I mean, the\nallocator basically uses the streams' virtual allocateBuffer, whose\nimportBuffers counterpart is the invoked at Camera::start().\n\nWe still need to call Camera::allocateBuffers() for internal buffers\nallocation, which makes the 'buffer allocation' operations quite a\nfew...\n\nI wonder if, as the allocator really need to be exposed to\napplications..\n\nI assume for the internally allocated buffer it could be simply\nreplaced by calling Stream::allocate() on each Stream part of the\ncamera configuration.\n\nI still fail to find how externally allocated buffers are imported\nnow, will they go through the buffer allocator as well ?\n\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 17 ++++++++++++++\n> include/libcamera/stream.h | 1 +\n> src/libcamera/buffer.cpp | 48 ++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 66 insertions(+)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index c626f669040b3c04..adb642ad5da072d2 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -8,11 +8,14 @@\n> #define __LIBCAMERA_BUFFER_H__\n>\n> #include \n> +#include \n> +#include \n> #include \n> #include \n>\n> namespace libcamera {\n>\n> +class Camera;\n> class Request;\n> class Stream;\n>\n> @@ -139,6 +142,20 @@ private:\n> \tstd::vector planes_;\n> };\n>\n> +class BufferAllocator\n> +{\n> +public:\n> +\tBufferAllocator(std::shared_ptr camera);\n> +\t~BufferAllocator();\n> +\n> +\tint allocate(Stream *stream);\n> +\tstd::vector get(Stream *stream);\n> +\n> +private:\n> +\tstd::shared_ptr camera_;\n> +\tstd::map> allocated_;\n> +};\n> +\n> } /* namespace libcamera */\n>\n> #endif /* __LIBCAMERA_BUFFER_H__ */\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index dac4831cfa1a9b1d..b051341511a7ab7c 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -84,6 +84,7 @@ public:\n> \tMemoryType memoryType() const { return memoryType_; }\n>\n> protected:\n> +\tfriend class BufferAllocator;\n> \tfriend class Camera;\n>\n> \tvirtual int allocateBuffers(std::vector *buffers) { return -EINVAL; }\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 10b16a862393b536..fce1ce5e49cbbf42 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -12,6 +12,9 @@\n> #include \n> #include \n>\n> +#include \n> +#include \n> +\n> #include \"log.h\"\n>\n> /**\n> @@ -393,4 +396,49 @@ void Buffer::cancel()\n> * The intended callers are Request::prepare() and Request::completeBuffer().\n> */\n>\n> +BufferAllocator::BufferAllocator(std::shared_ptr camera)\n> +\t: camera_(camera)\n> +{\n> +}\n> +\n> +BufferAllocator::~BufferAllocator()\n> +{\n> +\tfor (auto &it : allocated_) {\n> +\t\tStream *stream = it.first;\n> +\t\tstd::vector &buffers = it.second;\n> +\n> +\t\tfor (Buffer *buffer : buffers)\n> +\t\t\tdelete buffer;\n> +\n> +\t\tbuffers.clear();\n> +\n> +\t\tstream->allocateBuffers(nullptr);\n> +\t}\n> +}\n> +\n> +int BufferAllocator::allocate(Stream *stream)\n> +{\n> +\tbool found = false;\n> +\n> +\tfor (const Stream *s : camera_->streams()) {\n> +\t\tif (stream == s) {\n> +\t\t\tfound = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!found)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn stream->allocateBuffers(&allocated_[stream]);\n> +}\n> +\n> +std::vector BufferAllocator::get(Stream *stream)\n> +{\n> +\tif (allocated_.find(stream) == allocated_.end())\n> +\t\treturn {};\n> +\n> +\treturn allocated_[stream];\n> +}\n> +\n> } /* namespace libcamera */\n> --\n> 2.23.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 relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AB966136F\n\tfor ;\n\tFri, 15 Nov 2019 16:09:55 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 1D396FF80F;\n\tFri, 15 Nov 2019 15:09:54 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 15 Nov 2019 16:11:56 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191115151156.rjqsrzhdq5vqs5ez@uno.localdomain>","References":"<20191028022525.796995-1-niklas.soderlund@ragnatech.se>\n\t<20191028022525.796995-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"3agrqrareclnnwrr\"","Content-Disposition":"inline","In-Reply-To":"<20191028022525.796995-9-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC 08/12] libcamera: buffer: Add a buffer\n\tallocator","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, 15 Nov 2019 15:09:55 -0000"}},{"id":3075,"web_url":"https://patchwork.libcamera.org/comment/3075/","msgid":"<20191118194924.GJ4888@pendragon.ideasonboard.com>","date":"2019-11-18T19:49:24","subject":"Re: [libcamera-devel] [RFC 08/12] libcamera: buffer: Add a buffer\n\tallocator","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, Nov 15, 2019 at 04:11:56PM +0100, Jacopo Mondi wrote:\n> On Mon, Oct 28, 2019 at 03:25:21AM +0100, Niklas Söderlund wrote:\n> > From an applications point of view buffers shall not be allocated\n> > directly by a camera or stream. Instead they shall be allocated\n> > externally and used by a camera.\n> >\n> > To model this behavior add a buffer allocator that is exposed to\n> > applications. How the allocator creates buffers is pipeline specific and\n\ns/pipeline specific/pipeline-specific/\n\n> > handled by sub-classing the stream class.\n> \n> I would dare to question the whole allocator idea here... I mean, the\n> allocator basically uses the streams' virtual allocateBuffer, whose\n> importBuffers counterpart is the invoked at Camera::start().\n\nI like the allocator, but I would dare to question the commit message\n:-) I know what this is about as we've discussed it during the last code\ncamp, but it's not very clear here. Documentation will help, and I think\nyou could then rewrite the commit message with some excerpts of the\ndocumentation. It should explain the design idea behind this (which is,\nin a nutshell, that the libcamera API should be designed based on\nexternally-allocated buffers, with a helper allocator to provide buffers\nto applications that can't allocate them externally).\n\n> We still need to call Camera::allocateBuffers() for internal buffers\n> allocation, which makes the 'buffer allocation' operations quite a\n> few...\n> \n> I wonder if, as the allocator really need to be exposed to\n> applications..\n> \n> I assume for the internally allocated buffer it could be simply\n> replaced by calling Stream::allocate() on each Stream part of the\n> camera configuration.\n> \n> I still fail to find how externally allocated buffers are imported\n> now, will they go through the buffer allocator as well ?\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/buffer.h | 17 ++++++++++++++\n> > include/libcamera/stream.h | 1 +\n> > src/libcamera/buffer.cpp | 48 ++++++++++++++++++++++++++++++++++++++\n> > 3 files changed, 66 insertions(+)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index c626f669040b3c04..adb642ad5da072d2 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -8,11 +8,14 @@\n> > #define __LIBCAMERA_BUFFER_H__\n> >\n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> >\n> > namespace libcamera {\n> >\n> > +class Camera;\n> > class Request;\n> > class Stream;\n> >\n> > @@ -139,6 +142,20 @@ private:\n> > \tstd::vector planes_;\n> > };\n> >\n> > +class BufferAllocator\n> > +{\n> > +public:\n> > +\tBufferAllocator(std::shared_ptr camera);\n> > +\t~BufferAllocator();\n> > +\n> > +\tint allocate(Stream *stream);\n\nI like the buffer allocator concept, but I wonder if the API is right.\nYou rely on allocate() being called after Camera::configure() to let the\npipeline handler determine the buffers sizes. Isn't this a bit too\nlimiting ? Shouldn't we let the application request a specific size\ninstead ? This would require usage of VIDIOC_CREATE_BUFS in\nV4L2VideoDevice. Do you think it's overkill, and applications that need\nto allocate buffers with larger sizes should then allocate them\nexternally ?\n\nThe lack of free() makes the API unbalanced. It feels a bit fragile, but\nI'd have to see how this class is used to judge if that's fine.\n\n> > +\tstd::vector get(Stream *stream);\n\nShould this return a const std::vector & ? Or do you envision\nthe caller would need a writable copy anyway ?\n\n> > +\n> > +private:\n> > +\tstd::shared_ptr camera_;\n> > +\tstd::map> allocated_;\n\ns/allocated_/buffers_/ ?\n\n> > +};\n> > +\n\nThis should be moved to a separate file to show that it's really an\noptional helper and not part of the core buffer API.\n\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_BUFFER_H__ */\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index dac4831cfa1a9b1d..b051341511a7ab7c 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -84,6 +84,7 @@ public:\n> > \tMemoryType memoryType() const { return memoryType_; }\n> >\n> > protected:\n> > +\tfriend class BufferAllocator;\n\nDoes this belong to a separate patch ?\n\n> > \tfriend class Camera;\n> >\n> > \tvirtual int allocateBuffers(std::vector *buffers) { return -EINVAL; }\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 10b16a862393b536..fce1ce5e49cbbf42 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -12,6 +12,9 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"log.h\"\n> >\n> > /**\n> > @@ -393,4 +396,49 @@ void Buffer::cancel()\n> > * The intended callers are Request::prepare() and Request::completeBuffer().\n> > */\n> >\n> > +BufferAllocator::BufferAllocator(std::shared_ptr camera)\n> > +\t: camera_(camera)\n> > +{\n> > +}\n> > +\n> > +BufferAllocator::~BufferAllocator()\n> > +{\n> > +\tfor (auto &it : allocated_) {\n\nIt just occurred to me that, technically speaking, this isn't an\niterator, so the name \"it\" isn't the best. The variable's type is\ndecltype(allocated_)::value_type, which resolves to\n\nstd::pair>\n\nMaybe this should be renamed to streamBuffers ? Or possibly value ? Or\nanother short name we could use through the libcamera code base in\nsimilar cases (val ? mapval ?) ? Or maybe this is overkill :-)\n\n> > +\t\tStream *stream = it.first;\n> > +\t\tstd::vector &buffers = it.second;\n> > +\n> > +\t\tfor (Buffer *buffer : buffers)\n> > +\t\t\tdelete buffer;\n> > +\n> > +\t\tbuffers.clear();\n> > +\n> > +\t\tstream->allocateBuffers(nullptr);\n\nWouldn't Stream::freeBuffers() be a nicer name ?\n\n> > +\t}\n> > +}\n> > +\n> > +int BufferAllocator::allocate(Stream *stream)\n> > +{\n> > +\tbool found = false;\n> > +\n> > +\tfor (const Stream *s : camera_->streams()) {\n> > +\t\tif (stream == s) {\n> > +\t\t\tfound = true;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!found)\n> > +\t\treturn -EINVAL;\n\nShould we log a message here ? Should we store a pointer to the camera\nin the Stream class to facilitate this kind of error checking ?\n\n> > +\n> > +\treturn stream->allocateBuffers(&allocated_[stream]);\n> > +}\n> > +\n> > +std::vector BufferAllocator::get(Stream *stream)\n> > +{\n> > +\tif (allocated_.find(stream) == allocated_.end())\n> > +\t\treturn {};\n> > +\n> > +\treturn allocated_[stream];\n\nThis results in a double lookup.\n\n\tauto iter = allocated_.find(stream);\n\tif (iter == allocated_.end())\n\t\treturn {};\n\n\treturn iter->second;\n\n> > +}\n> > +\n> > } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6761C60F1C\n\tfor ;\n\tMon, 18 Nov 2019 20:49:43 +0100 (CET)","from pendragon.ideasonboard.com (unknown [38.98.37.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A316A540;\n\tMon, 18 Nov 2019 20:49:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574106583;\n\tbh=zXF93GQjOWDMaHTW1q1VY+6ubfsG6tL2R6KgWJwpATg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XG4XKxDZiBw9z3NgFwwgYe0YvNpW2w1Zi+MLuTdNm7JZn5giYIQiY24hl8bUyjpCb\n\tCFYmO11PBsIv7QgHKFD0XzJ6EJz7r92EFz/ijMoQgtlbCNgZRh7Uo8BazxhRU7etWd\n\tkgb7jeLqF7SD8/etuer2mOkUklLCrODSzqVfjDdk=","Date":"Mon, 18 Nov 2019 21:49:24 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191118194924.GJ4888@pendragon.ideasonboard.com>","References":"<20191028022525.796995-1-niklas.soderlund@ragnatech.se>\n\t<20191028022525.796995-9-niklas.soderlund@ragnatech.se>\n\t<20191115151156.rjqsrzhdq5vqs5ez@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191115151156.rjqsrzhdq5vqs5ez@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 08/12] libcamera: buffer: Add a buffer\n\tallocator","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, 18 Nov 2019 19:49:43 -0000"}}]