[{"id":3172,"web_url":"https://patchwork.libcamera.org/comment/3172/","msgid":"<20191202131624.3lmdiudphshrpqay@uno.localdomain>","date":"2019-12-02T13:16:24","subject":"Re: [libcamera-devel] [PATCH 25/30] libcamera: allocator: Add\n\tBufferAllocator to help applications allocate buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\nOn Wed, Nov 27, 2019 at 12:36:15AM +0100, Niklas Söderlund wrote:\n> The FrameBuffer interface is based on the idea that all buffers are\n> allocated outside libcamera and are only used by it. This is done to\n> make the API simpler and so that both internal and external buffers are\n> handled in the same way.\n>\n> As V4L2 do not provide a way to allocate buffers without the help of a\n> video device add an application facing helper which hide this. This\n> allow applications to allocate buffers and then use them with cameras.\n\nI'm sorry but I really fail to see why we need this abstraction.\n\nThe BufferAllocator\n- is Constructed with a camera\n- allocate buffers from Streams with a StreamConfiguration\n- maintain a list of allocated buffers, indexed by stream\n\nThe usage I see in cam is\n\nallocator = new Allocator(Camera);\nfor (streamConfig : camera->config())\n allocator.allocate(streamConfig->stream(),\n streamConfig);\n\nfor (nbufs : numBuffers)\n request = new Request()\n\n for (streamConfig : camera->config())\n buffer = allocator.buffer(streamConfig->stream())[nbufs];\n request->addBuffer(streamConfig->stream(), buffer);\n\nMy first question would be\n- if allocator.allocate takes a Stream and a config (the stream could\n be accessed from the config, but that's a different issue) and just\n calls 'stream.allocateBuffers(config)' what prevents application from\n calling stream.allocateBuffers() themselves ?\n\n- The whole point of allocator is to maintain a map to Stream to\n vectors of Buffers ? So can't that list live in the Stream itself ?\n Can't the map live in the camera if we want to ? They need to be put\n together when adding a buffer-stream pair to a request, and the\n whole dance of allocation and buffer access which all needs a Stream\n to work seems like quite a complex and API for applications.\n The above portion of code could be simplified to\n\nfor (stream : camera->streams()) {\n StreamConfiguration *config = camera->streamConfig(stream);\n ^- this is easy to implement\n stream->allocate(config);\n\nfor (nbufs : numBuffers)\n request = new Request()\n\n for (streamConfig : camera->config())\n buffer = streamConfig->stream().buffer(i);\n request->addBuffer(streamConfig->stream(), buffer);\n\nIsn't this less convoluted ?\n\nHow do we expect the buffer importing to look like, as I am missing\nhow this series handles it ?\n\nThanks\n j\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/allocator.h | 39 ++++++++++\n> include/libcamera/meson.build | 1 +\n> include/libcamera/stream.h | 1 +\n> src/libcamera/allocator.cpp | 141 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 183 insertions(+)\n> create mode 100644 include/libcamera/allocator.h\n> create mode 100644 src/libcamera/allocator.cpp\n>\n> diff --git a/include/libcamera/allocator.h b/include/libcamera/allocator.h\n> new file mode 100644\n> index 0000000000000000..36fce38491b5f3ef\n> --- /dev/null\n> +++ b/include/libcamera/allocator.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * allocator.h - Buffer allocator\n> + */\n> +#ifndef __LIBCAMERA_ALLOCATOR_H__\n> +#define __LIBCAMERA_ALLOCATOR_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera;\n> +class FrameBuffer;\n> +class Stream;\n> +struct StreamConfiguration;\n> +\n> +class BufferAllocator\n> +{\n> +public:\n> +\tBufferAllocator(std::shared_ptr camera);\n> +\t~BufferAllocator();\n> +\n> +\tint allocate(Stream *stream, const StreamConfiguration &config);\n> +\tvoid release(Stream *stream);\n> +\n> +\tconst std::vector &buffers(Stream *stream) const;\n> +\n> +private:\n> +\tstd::shared_ptr camera_;\n> +\tstd::map> buffers_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_ALLOCATOR_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 99abf06099407c1f..0d0ba2248fd8a679 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_api = files([\n> + 'allocator.h',\n> 'bound_method.h',\n> 'buffer.h',\n> 'camera.h',\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index a3c692c347340382..8e653408fd54cf74 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -85,6 +85,7 @@ public:\n> \tMemoryType memoryType() const { return memoryType_; }\n>\n> protected:\n> +\tfriend class BufferAllocator; /* To allocate and release buffers. */\n> \tfriend class Camera;\n>\n> \tvirtual int allocateBuffers(const StreamConfiguration &config,\n> diff --git a/src/libcamera/allocator.cpp b/src/libcamera/allocator.cpp\n> new file mode 100644\n> index 0000000000000000..8b517c809c05cbcd\n> --- /dev/null\n> +++ b/src/libcamera/allocator.cpp\n> @@ -0,0 +1,141 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * allocator.cpp - Buffer allocator\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file allocator.h\n> + * \\brief Buffer allocator\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Allocator)\n> +\n> +/**\n> + * \\class BufferAllocator\n> + * \\brief Buffer allocator for applications\n> + *\n> + * All buffers are to be treated as if they where allocated outside of the\n> + * camera. In some situations however the only source applications can allocate\n> + * buffers from is the camera. The BufferAllocator is the interface applications\n> + * shall use in these situations.\n> + *\n> + * The buffer allocator creates buffers using resources from a camera and/or\n> + * a stream. The buffers allocated this way are owned by the application and it\n> + * is responsible for their lifetime management. But just as buffers allocated\n> + * external from cameras and streams it's not valid to free a buffer while it's\n> + * queue to a camera.\n> + *\n> + * All buffers allocator are automatically freed when the allocator object is\n> + * deleted. It is the applications responsibility to make sure this do not\n> + * happen while one or more of the buffers are queued to a camera.\n> + *\n> + * If buffers are allocated outside the scope of libcamera by the application it\n> + * do not need to interact with the buffer allocator.\n> + */\n> +\n> +/**\n> + * \\brief Create a BufferAllocator serving a camera\n> + * \\param[in] camera Camera the allocator shall serve\n> + */\n> +\n> +BufferAllocator::BufferAllocator(std::shared_ptr camera)\n> +\t: camera_(camera)\n> +{\n> +}\n> +\n> +BufferAllocator::~BufferAllocator()\n> +{\n> +\tfor (auto &value : buffers_) {\n> +\t\tStream *stream = value.first;\n> +\t\tstd::vector &buffers = value.second;\n> +\n> +\t\tfor (FrameBuffer *buffer : buffers)\n> +\t\t\tdelete buffer;\n> +\n> +\t\tbuffers.clear();\n> +\n> +\t\tstream->releaseBuffers();\n> +\t}\n> +\n> +\tbuffers_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Allocate buffers from a stream\n> + * \\param[in] stream The stream to allocate buffers from\n> + * \\param[in] config The configuration described the buffers to be allocated\n> + *\n> + * Allocate buffers matching exactly what is described in \\a config. If buffers\n> + * matching \\a config can't be allocated an error is returned and no buffers are\n> + * allocated.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int BufferAllocator::allocate(Stream *stream, const StreamConfiguration &config)\n> +{\n> +\tauto iter = camera_->streams().find(stream);\n> +\tif (iter != camera_->streams().end())\n> +\t\treturn stream->allocateBuffers(config, &buffers_[stream]);\n> +\n> +\tLOG(Allocator, Error) << \"Stream do not belong to \" << camera_->name();\n> +\treturn -EINVAL;\n> +}\n> +\n> +/**\n> + * \\brief Free allocated buffers\n> + * \\param[in] stream The stream to free buffers for\n> + *\n> + * Free buffers allocated with allocate().\n> + */\n> +void BufferAllocator::release(Stream *stream)\n> +{\n> +\tauto iter = buffers_.find(stream);\n> +\tif (iter == buffers_.end())\n> +\t\treturn;\n> +\n> +\tstd::vector &buffers = iter->second;\n> +\n> +\tfor (FrameBuffer *buffer : buffers)\n> +\t\tdelete buffer;\n> +\n> +\tbuffers.clear();\n> +\n> +\tstream->releaseBuffers();\n> +\n> +\tbuffers_.erase(iter);\n> +}\n> +\n> +/**\n> + * \\brief Retrive array of allocated buffers\n> + * \\param[in] stream The stream to retrive buffers for\n> + *\n> + * \\return Array of buffers\n> + */\n> +const std::vector &BufferAllocator::buffers(Stream *stream) const\n> +{\n> +\tstatic std::vector empty;\n> +\n> +\tauto iter = buffers_.find(stream);\n> +\tif (iter == buffers_.end())\n> +\t\treturn empty;\n> +\n> +\treturn iter->second;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index c4f965bd7413b37e..b7041e003fdb1d49 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_sources = files([\n> + 'allocator.cpp',\n> 'bound_method.cpp',\n> 'buffer.cpp',\n> 'byte_stream_buffer.cpp',\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 relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C2EC60BC4\n\tfor ;\n\tMon, 2 Dec 2019 14:14:16 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 71C706000A;\n\tMon, 2 Dec 2019 13:14:15 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Mon, 2 Dec 2019 14:16:24 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191202131624.3lmdiudphshrpqay@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-26-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"tiiilcduudq5jiiz\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-26-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 25/30] libcamera: allocator: Add\n\tBufferAllocator to help applications allocate buffers","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 13:14:16 -0000"}},{"id":3252,"web_url":"https://patchwork.libcamera.org/comment/3252/","msgid":"<20191215031216.GI16015@pendragon.ideasonboard.com>","date":"2019-12-15T03:12:16","subject":"Re: [libcamera-devel] [PATCH 25/30] libcamera: allocator: Add\n\tBufferAllocator to help applications allocate buffers","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 02:16:24PM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:36:15AM +0100, Niklas Söderlund wrote:\n> > The FrameBuffer interface is based on the idea that all buffers are\n> > allocated outside libcamera and are only used by it. This is done to\n\ns/outside/externally to/\n\n> > make the API simpler and so that both internal and external buffers are\n> > handled in the same way.\n\n\"This is meant to create a simpler API centered around usage of buffers,\nregardless of where they come from.\"\n\n> > As V4L2 do not provide a way to allocate buffers without the help of a\n> > video device add an application facing helper which hide this. This\n> > allow applications to allocate buffers and then use them with cameras.\n\n\"Linux however lacks a centralized allocator at the moment, and not all\nusers of libcamera are expected to use another device that could provide\nsuitable buffers for the camera. This patch thus adds a helper class to\nallocate buffers internally in libcamera, in a way that matches the\nneeds of the FrameBuffer-based API.\"\n\n> \n> I'm sorry but I really fail to see why we need this abstraction.\n> \n> The BufferAllocator\n> - is Constructed with a camera\n> - allocate buffers from Streams with a StreamConfiguration\n> - maintain a list of allocated buffers, indexed by stream\n> \n> The usage I see in cam is\n> \n> allocator = new Allocator(Camera);\n> for (streamConfig : camera->config())\n> allocator.allocate(streamConfig->stream(),\n> streamConfig);\n> \n> for (nbufs : numBuffers)\n> request = new Request()\n> \n> for (streamConfig : camera->config())\n> buffer = allocator.buffer(streamConfig->stream())[nbufs];\n> request->addBuffer(streamConfig->stream(), buffer);\n> \n> My first question would be\n> - if allocator.allocate takes a Stream and a config (the stream could\n> be accessed from the config, but that's a different issue) and just\n> calls 'stream.allocateBuffers(config)' what prevents application from\n> calling stream.allocateBuffers() themselves ?\n\nStream::allocateBuffers() is an internal API. The idea behind the buffer\nallocator and the rest of the buffer rework is to create a single API\ncentered around buffer usage, but this isn't possible due to the lack of\na suitable centralized allocator at the moment. We thus need to provide\na way for applications to allocate buffers internally in the camera, and\nwe have decided to do so through a single helper class to avoid exposing\nany other allocation-related API to applications. The helper could then\nbe dropped later when a centralized allocator will be available, or\nconverted to use the centralized allocator transparently.\n\n> - The whole point of allocator is to maintain a map to Stream to\n> vectors of Buffers ? So can't that list live in the Stream itself ?\n\nThe main point of the allocator is to handle as much of the accounting\nneeded for buffer allocation internally to avoid spreading it through\nlibcamera. We can't achieve 100% of this goal as we still need to\nallocate the buffers on V4L2 video nodes.\n\n> Can't the map live in the camera if we want to ? They need to be put\n> together when adding a buffer-stream pair to a request, and the\n> whole dance of allocation and buffer access which all needs a Stream\n> to work seems like quite a complex and API for applications.\n> The above portion of code could be simplified to\n> \n> for (stream : camera->streams()) {\n> StreamConfiguration *config = camera->streamConfig(stream);\n> ^- this is easy to implement\n> stream->allocate(config);\n> \n> for (nbufs : numBuffers)\n> request = new Request()\n> \n> for (streamConfig : camera->config())\n> buffer = streamConfig->stream().buffer(i);\n> request->addBuffer(streamConfig->stream(), buffer);\n> \n> Isn't this less convoluted ?\n> \n> How do we expect the buffer importing to look like, as I am missing\n> how this series handles it ?\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/allocator.h | 39 ++++++++++\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/stream.h | 1 +\n> > src/libcamera/allocator.cpp | 141 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 5 files changed, 183 insertions(+)\n> > create mode 100644 include/libcamera/allocator.h\n> > create mode 100644 src/libcamera/allocator.cpp\n> >\n> > diff --git a/include/libcamera/allocator.h b/include/libcamera/allocator.h\n> > new file mode 100644\n> > index 0000000000000000..36fce38491b5f3ef\n> > --- /dev/null\n> > +++ b/include/libcamera/allocator.h\n> > @@ -0,0 +1,39 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * allocator.h - Buffer allocator\n\nbuffer_allocator.h ? Or maybe even better framebuffer_allocator.h ? Same\nfor the .cpp file.\n\ns/Buffer/FrameBuffer/\n\n> > + */\n> > +#ifndef __LIBCAMERA_ALLOCATOR_H__\n\nAnd __LIBCAMERA_BUFFER_ALLOCATOR_H__ (or FRAMEBUFFER as appropriate) ?\n\n> > +#define __LIBCAMERA_ALLOCATOR_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Camera;\n> > +class FrameBuffer;\n> > +class Stream;\n> > +struct StreamConfiguration;\n> > +\n> > +class BufferAllocator\n\nFrameBufferAllocator ?\n\n> > +{\n> > +public:\n> > +\tBufferAllocator(std::shared_ptr camera);\n> > +\t~BufferAllocator();\n> > +\n> > +\tint allocate(Stream *stream, const StreamConfiguration &config);\n> > +\tvoid release(Stream *stream);\n> > +\n> > +\tconst std::vector &buffers(Stream *stream) const;\n> > +\n> > +private:\n> > +\tstd::shared_ptr camera_;\n> > +\tstd::map> buffers_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_ALLOCATOR_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 99abf06099407c1f..0d0ba2248fd8a679 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_api = files([\n> > + 'allocator.h',\n> > 'bound_method.h',\n> > 'buffer.h',\n> > 'camera.h',\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index a3c692c347340382..8e653408fd54cf74 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -85,6 +85,7 @@ public:\n> > \tMemoryType memoryType() const { return memoryType_; }\n> >\n> > protected:\n> > +\tfriend class BufferAllocator; /* To allocate and release buffers. */\n> > \tfriend class Camera;\n> >\n> > \tvirtual int allocateBuffers(const StreamConfiguration &config,\n> > diff --git a/src/libcamera/allocator.cpp b/src/libcamera/allocator.cpp\n> > new file mode 100644\n> > index 0000000000000000..8b517c809c05cbcd\n> > --- /dev/null\n> > +++ b/src/libcamera/allocator.cpp\n> > @@ -0,0 +1,141 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * allocator.cpp - Buffer allocator\n\ns/Buffer/FrameBuffer/ (and similar changes below where appropriate)\n\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n\nI think you only need errno.h.\n\n> > +#include \n> > +#include \n\nYou should also include .\n\n> > +#include \"log.h\"\n> > +\n> > +/**\n> > + * \\file allocator.h\n> > + * \\brief Buffer allocator\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Allocator)\n> > +\n> > +/**\n> > + * \\class BufferAllocator\n> > + * \\brief Buffer allocator for applications\n> > + *\n> > + * All buffers are to be treated as if they where allocated outside of the\n> > + * camera. In some situations however the only source applications can allocate\n> > + * buffers from is the camera. The BufferAllocator is the interface applications\n> > + * shall use in these situations.\n> > + *\n> > + * The buffer allocator creates buffers using resources from a camera and/or\n> > + * a stream. The buffers allocated this way are owned by the application and it\n> > + * is responsible for their lifetime management. But just as buffers allocated\n> > + * external from cameras and streams it's not valid to free a buffer while it's\n> > + * queue to a camera.\n> > + *\n> > + * All buffers allocator are automatically freed when the allocator object is\n> > + * deleted. It is the applications responsibility to make sure this do not\n> > + * happen while one or more of the buffers are queued to a camera.\n> > + *\n> > + * If buffers are allocated outside the scope of libcamera by the application it\n> > + * do not need to interact with the buffer allocator.\n\n * The libcamera API is designed to consume buffers provided by applications as\n * FrameBuffer instances. This makes libcamera a user of buffers exported by\n * other devices (such as displays or video encoders), or allocated from an\n * external allocator (such as ION on Android platforms). In some situations,\n * applications do not have any mean to allocate or get hold of suitable\n * buffers, for instance when no other device is involved, on Linux platforms\n * that lack a centralized allocator. The FrameBufferAllocator class provides a\n * buffer allocator that can be used in these situations.\n *\n * Applications create a framebuffer allocator for a Camera, and use it to\n * allocate buffers for streams of a CameraConfiguration with allocate(). They\n * control which streams to allocate buffers for, and can thus use external\n * buffers for a subset of the streams if desired.\n *\n * Buffers are deleted for a stream with release(), and destroying the allocator\n * automatically deletes all allocated buffers. Applications own the buffers\n * allocated by the FrameBufferAllocator and are responsible for ensuring the\n * buffers are not deleted while they are in use (part of a Request that has\n * been queued and hasn't completed yet).\n *\n * Usage of the FrameBufferAllocator is optional, if all buffers for a camera\n * are provided externally applications shall not use this class.\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a BufferAllocator serving a camera\n> > + * \\param[in] camera Camera the allocator shall serve\n\ns/Camera/The camera/\n\nand maybe just\n\n * \\param[in] camera The camera\n\n> > + */\n> > +\n> > +BufferAllocator::BufferAllocator(std::shared_ptr camera)\n> > +\t: camera_(camera)\n> > +{\n> > +}\n> > +\n> > +BufferAllocator::~BufferAllocator()\n> > +{\n> > +\tfor (auto &value : buffers_) {\n> > +\t\tStream *stream = value.first;\n> > +\t\tstd::vector &buffers = value.second;\n> > +\n> > +\t\tfor (FrameBuffer *buffer : buffers)\n> > +\t\t\tdelete buffer;\n> > +\n> > +\t\tbuffers.clear();\n> > +\n> > +\t\tstream->releaseBuffers();\n> > +\t}\n> > +\n> > +\tbuffers_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Allocate buffers from a stream\n\n * \\brief Allocate buffers for a stream with a configuration\n\n> > + * \\param[in] stream The stream to allocate buffers from\n\ns/from/for/\n\n> > + * \\param[in] config The configuration described the buffers to be allocated\n\nThe configuration doesn't describe the buffers to be allocated. Unless\nsomeone has a better proposal, I would just go for\n\n * \\param[in] config The stream configuration\n\n> > + *\n> > + * Allocate buffers matching exactly what is described in \\a config. If buffers\n> > + * matching \\a config can't be allocated an error is returned and no buffers are\n> > + * allocated.\n\nHow about\n\n * Allocate buffers suitable for capturing frames from the \\a stream configured\n * as described by \\a config. The configuration shall be valid for the stream as\n * checked CameraConfiguration::validate().\n *\n * Upon successful allocation, the allocated buffers can be retrieved with the\n * buffers() method.\n\nI would drop the second sentence.\n\nDoesn't the camera need to have been configured too ? I think the\ndocumentation needs to be expanded to explain when allocating buffers is\nallowed. I also wonder what happens when reconfiguring a camera for\nwhich buffers have been allocated. Is this supported, and if so, in what\nconditions ? If you explain how this works I can help expanding the\ndocumentation.\n\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n\nHow about enumerating the error codes ? I think we should return -EINVAL\nif the stream doesn't belong to the camera of the configuration isn't\nvalid, -ENOMEM if the buffers can't be allocated and -EBUSY if they are\nalready allocated. But this would require guaranteeing that\nStream::allocateBuffers() only return those codes. I think it's doable,\nbut should be documented.\n\n> > + */\n> > +int BufferAllocator::allocate(Stream *stream, const StreamConfiguration &config)\n> > +{\n> > +\tauto iter = camera_->streams().find(stream);\n> > +\tif (iter != camera_->streams().end())\n> > +\t\treturn stream->allocateBuffers(config, &buffers_[stream]);\n> > +\n> > +\tLOG(Allocator, Error) << \"Stream do not belong to \" << camera_->name();\n\ns/do/does/\n\n> > +\treturn -EINVAL;\n\nI think you should handle the errors as they are detected.\n\n\tauto iter = camera_->streams().find(stream);\n\tif (iter == camera_->streams().end()) {\n\t\tLOG(Allocator, Error)\n\t\t\t<< \"Stream does not belong to \" << camera_->name();\n\t\treturn -EINVAL;\n\t}\n\n\treturn stream->allocateBuffers(config, &buffers_[stream]);\n\nShould you also protect against double allocation with\n\n\tif (buffers_.count(stream)) {\n\t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n\t\treturn -EBUSY;\n\t}\n\n> > +}\n> > +\n> > +/**\n> > + * \\brief Free allocated buffers\n\n * \\brief Free buffers previously allocated for a \\a stream\n\n> > + * \\param[in] stream The stream to free buffers for\n> > + *\n> > + * Free buffers allocated with allocate().\n\nAdd \"This invalidates the buffers returned by buffers().\"\n\n> > + */\n> > +void BufferAllocator::release(Stream *stream)\n> > +{\n> > +\tauto iter = buffers_.find(stream);\n> > +\tif (iter == buffers_.end())\n> > +\t\treturn;\n> > +\n> > +\tstd::vector &buffers = iter->second;\n> > +\n> > +\tfor (FrameBuffer *buffer : buffers)\n> > +\t\tdelete buffer;\n> > +\n> > +\tbuffers.clear();\n> > +\n> > +\tstream->releaseBuffers();\n> > +\n> > +\tbuffers_.erase(iter);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrive array of allocated buffers\n\n * \\brief Retrieve the buffers allocated for a \\a stream\n\n> > + * \\param[in] stream The stream to retrive buffers for\n\ns/retrive/retrieve/\n\n *\n * This method shall only be called after successfully allocating buffers for\n * \\a stream with allocate(). The returned buffers are valid until the earlier\n * of a call to release() for the same stream or the destruction of the\n * FrameBufferPool instance.\n *\n\n> > + *\n> > + * \\return Array of buffers\n\n * \\return The buffers allocated for the \\a stream\n\n> > + */\n> > +const std::vector &BufferAllocator::buffers(Stream *stream) const\n> > +{\n> > +\tstatic std::vector empty;\n> > +\n> > +\tauto iter = buffers_.find(stream);\n> > +\tif (iter == buffers_.end())\n> > +\t\treturn empty;\n> > +\n> > +\treturn iter->second;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index c4f965bd7413b37e..b7041e003fdb1d49 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_sources = files([\n> > + 'allocator.cpp',\n> > 'bound_method.cpp',\n> > 'buffer.cpp',\n> > 'byte_stream_buffer.cpp',","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 BB302601E5\n\tfor ;\n\tSun, 15 Dec 2019 04:12:27 +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 A89012D1;\n\tSun, 15 Dec 2019 04:12:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576379547;\n\tbh=NJtYFzfuOQEfx4emcdIIJ4tTW7r0PLXAxaBsAxKAA2o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ATE+x9iN6QiCSEUqdiDM/MgfTelAPQYNBUhyTpNr7HIZLftfZr9oVyqas6h46TS4G\n\tgtIkhLWgzpX85dsb8wDqs+9CamHgT6E5q77bU7OgZ6WghXZ2IdOKKGMKAPPjpWIWjE\n\tXypEuF84nDHNyZHkSExO8gNLaNvkRRgGqSja5bcM=","Date":"Sun, 15 Dec 2019 05:12:16 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191215031216.GI16015@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-26-niklas.soderlund@ragnatech.se>\n\t<20191202131624.3lmdiudphshrpqay@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191202131624.3lmdiudphshrpqay@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 25/30] libcamera: allocator: Add\n\tBufferAllocator to help applications allocate buffers","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 03:12:27 -0000"}}]