[{"id":3385,"web_url":"https://patchwork.libcamera.org/comment/3385/","msgid":"<20200108022913.GC10933@pendragon.ideasonboard.com>","date":"2020-01-08T02:29:13","subject":"Re: [libcamera-devel] [PATCH v2 19/25] libcamera: allocator: Add\n\tFrameBufferAllocator 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 30, 2019 at 01:05:04PM +0100, Niklas Söderlund wrote:\n> The FrameBuffer interface is based on the idea that all buffers are\n> allocated externally to libcamera and are only used by it. This is meant\n> to create a simpler API centered around usage of buffers, regardless of\n> where they come from.\n> \n> Linux however lacks a centralized allocator at the moment, and not all\n> users of libcamera are expected to use another device that could provide\n> suitable buffers for the camera. This patch thus adds a helper class to\n> allocate buffers internally in libcamera, in a way that matches the\n> needs of the FrameBuffer-based API.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n> * Changes since v1\n> - Update commit message\n> - Rename class to FrameBufferAllocator\n> - Rename source files framebuffer_allocator.{cpp,h}\n> - Update include headers\n> - Update documentation\n> - Check for double allocation for the same stream in allocate()\n> - Add interactions with Camera::allocator_ and enforce buffers may only\n>   be allocated when the camera is in the configured state.\n> ---\n>  include/libcamera/camera.h                |   5 +\n>  include/libcamera/framebuffer_allocator.h |  47 +++++\n>  include/libcamera/meson.build             |   1 +\n>  src/libcamera/camera.cpp                  |  15 +-\n>  src/libcamera/framebuffer_allocator.cpp   | 213 ++++++++++++++++++++++\n>  src/libcamera/meson.build                 |   1 +\n>  6 files changed, 281 insertions(+), 1 deletion(-)\n>  create mode 100644 include/libcamera/framebuffer_allocator.h\n>  create mode 100644 src/libcamera/framebuffer_allocator.cpp\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index ef6a37bb142c83a6..f2827438871189a1 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -20,6 +20,7 @@\n>  namespace libcamera {\n>  \n>  class Buffer;\n> +class FrameBufferAllocator;\n>  class PipelineHandler;\n>  class Request;\n>  \n> @@ -126,6 +127,10 @@ private:\n>  \n>  \tbool disconnected_;\n>  \tState state_;\n> +\n> +\t/* Needed to update allocator_ and read state_. */\n> +\tfriend class FrameBufferAllocator;\n> +\tFrameBufferAllocator *allocator_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> new file mode 100644\n> index 0000000000000000..75952762884e44ac\n> --- /dev/null\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -0,0 +1,47 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * framebuffer_allocator.h - FrameBuffer allocator\n> + */\n> +#ifndef __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__\n> +#define __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__\n> +\n> +#include <map>\n> +#include <memory>\n> +#include <unordered_set>\n> +#include <vector>\n> +\n> +namespace libcamera {\n> +\n> +class Camera;\n> +class FrameBuffer;\n> +class Stream;\n> +\n> +class FrameBufferAllocator\n> +{\n> +public:\n> +\tstatic FrameBufferAllocator *create(std::shared_ptr<Camera> camera);\n> +\n> +\tFrameBufferAllocator(const Camera &) = delete;\n> +\tFrameBufferAllocator &operator=(const Camera &) = delete;\n> +\n> +\t~FrameBufferAllocator();\n> +\n> +\tint allocate(Stream *stream);\n> +\tint release(Stream *stream);\n\ns/release/free/ ?\n\n> +\n> +\tconst std::unordered_set<Stream *> &streams() const { return streams_; }\n\nThis method seems to only be used by Camera::configure() to check if any\nbuffer has been allocated. How about replacing it with a\n\n\tbool allocated() const { return !buffers_.empty(); }\n\nand dropping the streams_ data member ?\n\n> +\tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;\n> +\n> +private:\n> +\tFrameBufferAllocator(std::shared_ptr<Camera> camera);\n> +\n> +\tstd::shared_ptr<Camera> camera_;\n> +\tstd::unordered_set<Stream *> streams_;\n> +\tstd::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 543e6773cc5158a0..8db217bb782c1443 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -7,6 +7,7 @@ libcamera_api = files([\n>      'event_dispatcher.h',\n>      'event_notifier.h',\n>      'file_descriptor.h',\n> +    'framebuffer_allocator.h',\n>      'geometry.h',\n>      'logging.h',\n>      'object.h',\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index e810fb725d81350d..222c6811d5698f0f 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -9,6 +9,7 @@\n>  \n>  #include <iomanip>\n>  \n> +#include <libcamera/framebuffer_allocator.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -405,7 +406,7 @@ const std::string &Camera::name() const\n>  \n>  Camera::Camera(PipelineHandler *pipe, const std::string &name)\n>  \t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> -\t  state_(CameraAvailable)\n> +\t  state_(CameraAvailable), allocator_(nullptr)\n>  {\n>  }\n>  \n> @@ -541,6 +542,12 @@ int Camera::release()\n>  \tif (!stateBetween(CameraAvailable, CameraConfigured))\n>  \t\treturn -EBUSY;\n>  \n> +\tif (allocator_) {\n> +\t\tLOG(Camera, Error)\n> +\t\t\t<< \"Allocator must be deleted before camera can be released\";\n\nCould you add a\n\n\t/*\n\t * \\todo Try to find a better API that would make this error\n\t * impossible\n\t */\n\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n>  \tpipe_->unlock();\n>  \n>  \tstate_ = CameraAvailable;\n> @@ -649,6 +656,12 @@ int Camera::configure(CameraConfiguration *config)\n>  \tif (!stateBetween(CameraAcquired, CameraConfigured))\n>  \t\treturn -EACCES;\n>  \n> +\tif (allocator_ && allocator_->streams().size()) {\n> +\t\tLOG(Camera, Error)\n> +\t\t\t<< \"Allocator must be deleted before camera can be reconfigured\";\n\n\"Buffers must be freed before the camera can be reconfigured\" ?\n\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n>  \tif (config->validate() != CameraConfiguration::Valid) {\n>  \t\tLOG(Camera, Error)\n>  \t\t\t<< \"Can't configure camera with invalid configuration\";\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> new file mode 100644\n> index 0000000000000000..98b649ecf6053790\n> --- /dev/null\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -0,0 +1,213 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * framebuffer_allocator.cpp - FrameBuffer allocator\n> + */\n> +\n> +#include <libcamera/framebuffer_allocator.h>\n> +\n> +#include <errno.h>\n> +\n> +#include <libcamera/buffer.h>\n> +#include <libcamera/camera.h>\n> +#include <libcamera/stream.h>\n> +\n> +#include \"log.h\"\n> +#include \"pipeline_handler.h\"\n> +\n> +/**\n> + * \\file framebuffer_allocator.h\n> + * \\brief FrameBuffer allocator\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Allocator)\n> +\n> +/**\n> + * \\class FrameBufferAllocator\n> + * \\brief FrameBuffer allocator for applications\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> + * \\brief Create a framebuffer allocator instance\n\ns/framebuffer/FrameBuffer/\n\ns/ instance// ?\n\n> + * \\param[in] camera The camera the allocator serves\n> + *\n\n * A single allocator may be created for a Camera instance.\n\n> + * The caller is responsible for deleting the allocator before the camera is\n> + * released.\n> + *\n> + * \\return A pointer to the newly created allocator object or nullptr on error\n> + */\n> +FrameBufferAllocator *\n> +FrameBufferAllocator::create(std::shared_ptr<Camera> camera)\n> +{\n> +\tif (camera->allocator_) {\n> +\t\tLOG(Allocator, Error) << \"Camera already have an allocator\";\n\ns/have/has/\n\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera);\n> +\n> +\tcamera->allocator_ = allocator;\n> +\n> +\treturn allocator;\n\n\tcamera->allocator_ = new FrameBufferAllocator(camera);\n\treturn camera->allocator_;\n\n?\n\n> +}\n> +\n> +/**\n> + * \\brief Allocate buffers for a stream with a configuration\n\ns/stream with a configuration/configured stream/\n\n> + * \\param[in] stream The stream to allocate buffers for\n> + *\n> + * Allocate buffers suitable for capturing frames from the \\a stream.\n\n * The Camera shall have been previously configured with Camera::configure() and\n * shall be stopped, and the stream shall be part of the active camera\n * configuration.\n\n> Upon\n> + * successful allocation, the allocated buffers can be retrieved with the\n> + * buffers() method.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> + * \\retval -EINVAL The allocator do not handle the \\a stream\n\n\"The \\a stream does not belong to the camera\"\n\n\"... or the stream is not part of the active camera configuration\"\n\nand this error should be checked.\n\n> + * \\retval -EBUSY Buffers are already allocated for the \\a stream\n> + */\n> +int FrameBufferAllocator::allocate(Stream *stream)\n> +{\n> +\tif (camera_->state_ != Camera::CameraConfigured && camera_->state_ != Camera::CameraPrepared) {\n\nLine wrap ?\n\n> +\t\tLOG(Allocator, Error)\n> +\t\t\t<< \"Camera must be in the configured state to allocate buffers\";\n> +\t\treturn -EACCES;\n> +\t}\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> +\tif (buffers_.count(stream)) {\n> +\t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tunsigned int bufferCount = stream->configuration().bufferCount;\n> +\tint ret = camera_->pipe_->allocateFrameBuffers(camera_.get(), stream,\n> +\t\t\t\t\t\t       bufferCount,\n> +\t\t\t\t\t\t       &buffers_[stream]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstreams_.insert(stream);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Free buffers previously allocated for a \\a stream\n> + * \\param[in] stream The stream to free buffers for\n> + *\n> + * Free buffers allocated with allocate().\n> + *\n> + * This invalidates the buffers returned by buffers().\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EACCES The camera is not in a state where buffers can be freed\n> + * \\retval -EINVAL The allocator do not handle the \\a stream\n> + */\n> +int FrameBufferAllocator::release(Stream *stream)\n> +{\n> +\tif (camera_->state_ != Camera::CameraConfigured && camera_->state_ != Camera::CameraPrepared) {\n> +\t\tLOG(Allocator, Error)\n> +\t\t\t<< \"Camera must be in the configured state to free buffers\";\n> +\t\treturn -EACCES;\n> +\t}\n> +\n> +\tauto iter = buffers_.find(stream);\n> +\tif (iter == buffers_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n> +\n> +\tbuffers.clear();\n> +\n> +\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> +\n> +\tbuffers_.erase(iter);\n> +\n> +\tstreams_.erase(stream);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn FrameBufferAllocator::streams()\n> + * \\brief Retrieve the streams the allocator handles\n> + * \\return The streams the allocator handles\n> + */\n\nI won't comment on this as I think the method should be dropped :-)\n\n> +\n> +/**\n> + * \\brief Retrieve the buffers allocated for a \\a stream\n> + * \\param[in] stream The stream to retrive buffers for\n\ns/retrive/retrieve/\n\nor just \"The stream\"\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 release() is\n> + * called for the same stream or the destruction of the FrameBufferAllocator\n> + * instance.\n\n\"... or the FrameBufferAllocator instance is destroyed.\"\n\nOr maybe your proposal is fine, according to\nhttps://en.wikipedia.org/wiki/Coordination_(linguistics)#Mismatch_in_syntactic_category\n? Kieran, do you feel like a linguist today ? :-)\n\n> + *\n> + * \\return The buffers allocated for the \\a stream\n> + */\n> +const std::vector<std::unique_ptr<FrameBuffer>>\n> +&FrameBufferAllocator::buffers(Stream *stream) const\n> +{\n> +\tstatic std::vector<std::unique_ptr<FrameBuffer>> 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> +/**\n> + * \\brief Create a FrameBufferAllocator serving a camera\n\ns/Create/Construct/\n\n> + * \\param[in] camera The camera\n> + */\n> +\n\nNo blank line.\n\n> +FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)\n> +\t: camera_(camera)\n> +{\n> +}\n> +\n> +FrameBufferAllocator::~FrameBufferAllocator()\n> +{\n> +\tfor (auto &value : buffers_) {\n> +\t\tStream *stream = value.first;\n> +\t\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> +\t}\n> +\n> +\tbuffers_.clear();\n> +\n> +\tcamera_->allocator_ = nullptr;\n\nThis should be before allocate() to match the order in the class\ndefinition. I recommend moving both the constructor and the destructor\njust after ::create()\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 722c5bc15afe52ef..68d89559b290befd 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -16,6 +16,7 @@ libcamera_sources = files([\n>      'event_notifier.cpp',\n>      'file_descriptor.cpp',\n>      'formats.cpp',\n> +    'framebuffer_allocator.cpp',\n>      'geometry.cpp',\n>      'ipa_context_wrapper.cpp',\n>      'ipa_controls.cpp',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F24AD6045D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jan 2020 03:29:25 +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 EBD7252F;\n\tWed,  8 Jan 2020 03:29:24 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578450565;\n\tbh=CN8c06y+Zj15dVWz7d2pmnAXQ2iv9VBYzhcxMoTVFCk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mTQWWt0Hi60V+nozuwB5aOG/9eMdCX9nFdK2LSl625poVMktd6cH8wPSpEvjMgSOH\n\t4zF9J3ccP/IU1qOM+eBPypI+4aXMXWkGaHK5mycp5iW3vfUWNZKeS6n52qatfv/zfo\n\tilGubMOMUiw7e0lbKg+3baGeHLyrwBTHipJaFk/E=","Date":"Wed, 8 Jan 2020 04:29:13 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200108022913.GC10933@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-20-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191230120510.938333-20-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 19/25] libcamera: allocator: Add\n\tFrameBufferAllocator to help applications allocate buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 08 Jan 2020 02:29:26 -0000"}}]