[{"id":3412,"web_url":"https://patchwork.libcamera.org/comment/3412/","msgid":"<20200111011709.GO4859@pendragon.ideasonboard.com>","date":"2020-01-11T01:17:09","subject":"Re: [libcamera-devel] [PATCH v3 25/33] 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 Fri, Jan 10, 2020 at 08:38:00PM +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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n> * Changes since v2\n> - Replace FrameBufferAllocator::streams() with FrameBufferAllocator::allocated()\n> - Add todo to see if we can find a better API for Camera::relase() with a active FrameBufferAllocator\n> - Touch up documentation\n> - Make sure stream to be allocated is part of the cameras active configuration not just the camera\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 |  45 +++++\n>  include/libcamera/meson.build             |   1 +\n>  src/libcamera/camera.cpp                  |  19 +-\n>  src/libcamera/framebuffer_allocator.cpp   | 219 ++++++++++++++++++++++\n>  src/libcamera/meson.build                 |   1 +\n>  6 files changed, 289 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..2fd5870b15c69087 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 to read state_ and activeStreams_. */\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..42812253ada94d94\n> --- /dev/null\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -0,0 +1,45 @@\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 <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 free(Stream *stream);\n> +\n> +\tbool allocated() const { return !buffers_.empty(); }\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::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..91f5ef7771782eba 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,16 @@ int Camera::release()\n>  \tif (!stateBetween(CameraAvailable, CameraConfigured))\n>  \t\treturn -EBUSY;\n>  \n> +\tif (allocator_) {\n> +\t\t/*\n> +\t\t * \\todo Try to find a better API that would make this error\n> +\t\t * impossible.\n> +\t\t */\n> +\t\tLOG(Camera, Error)\n> +\t\t\t<< \"Buffers must be freed before the camera can be reconfigured\";\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n>  \tpipe_->unlock();\n>  \n>  \tstate_ = CameraAvailable;\n> @@ -649,6 +660,12 @@ int Camera::configure(CameraConfiguration *config)\n>  \tif (!stateBetween(CameraAcquired, CameraConfigured))\n>  \t\treturn -EACCES;\n>  \n> +\tif (allocator_ && allocator_->allocated()) {\n> +\t\tLOG(Camera, Error)\n> +\t\t\t<< \"Allocator must be deleted before camera can be reconfigured\";\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..b0246b6b530b6997\n> --- /dev/null\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -0,0 +1,219 @@\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 free(), 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\n> + * \\param[in] camera The camera the allocator serves\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 has an allocator\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tcamera->allocator_ = new FrameBufferAllocator(camera);\n> +\treturn camera->allocator_;\n> +}\n> +\n> +/**\n> + * \\brief Construct a FrameBufferAllocator serving a camera\n> + * \\param[in] camera The camera\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> +}\n> +\n> +/**\n> + * \\brief Allocate buffers for a configured stream\n> + * \\param[in] stream The stream to allocate buffers for\n> + *\n> + * Allocate buffers suitable for capturing frames from the \\a stream. The Camera\n> + * shall have been previously configured with Camera::configure() and shall be\n> + * stopped, and the stream shall be part of the active camera configuration.\n> + *\n> + * Upon 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 \\a stream does not belong to the camera or the stream is\n> + * not part of the active camera configuration\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 &&\n> +\t    camera_->state_ != Camera::CameraPrepared) {\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 iterCamera = camera_->streams().find(stream);\n> +\tif (iterCamera == camera_->streams().end()) {\n\n\tif (camera_->streams().find(stream) == camera_->streams().end()) {\n\n> +\t\tLOG(Allocator, Error)\n> +\t\t\t<< \"Stream does not belong to \" << camera_->name();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tauto iterActive = camera_->activeStreams_.find(stream);\n> +\tif (iterActive == camera_->activeStreams_.end()) {\n\n\tif (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) {\n\n> +\t\tLOG(Allocator, Error)\n> +\t\t\t<< \"Stream is not part of \" << camera_->name()\n> +\t\t\t<< \" active configuration\";\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> +\tint ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream,\n> +\t\t\t\t\t\t     &buffers_[stream]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Free buffers previously allocated for a \\a stream\n> + * \\param[in] stream The stream\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::free(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> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn FrameBufferAllocator::allocated()\n> + * \\brief Retrieve the streams the allocator handles\n\nYou can drop this first brief, it's a leftover.\n\n> + * \\brief Check if the allocator have allocated buffers for any stream\n\ns/have/has/\n\n> + * \\return True if the allocator have allocated buffers for one or more\n\ns/have/has/\n\nand keep my Reviewed-by.\n\n> + * streams, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Retrieve the buffers allocated for a \\a stream\n> + * \\param[in] stream The stream to retrive buffers for\n> + *\n> + * This method shall only be called after successfully allocating buffers for\n> + * \\a stream with allocate(). The returned buffers are valid until free() is\n> + * called for the same stream or the FrameBufferAllocator instance is destroyed.\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> +} /* 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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 106BC606AC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 Jan 2020 02:17:23 +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 7AD0C52F;\n\tSat, 11 Jan 2020 02:17:22 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578705442;\n\tbh=3FVGDaVLoRA2k6Ik8L3No4nuCBlSLAMAXLfyJWW+sds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hnE1hFmLmyk+WlxvxsSrEoxPice+eCINIwBbaX7INCql39teM01wkiampCPBJI3AX\n\t35myO/K3ByrtFZbMeXo0BlCoOL1V5ouXYA6MmXrqLOA4apMByXZZBQL1Oi7ZrLRTWy\n\t3Gjq6THmYFQwfBGU6XvVVgoNKyFoir5wWybmKn2E=","Date":"Sat, 11 Jan 2020 03:17:09 +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":"<20200111011709.GO4859@pendragon.ideasonboard.com>","References":"<20200110193808.2266294-1-niklas.soderlund@ragnatech.se>\n\t<20200110193808.2266294-26-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":"<20200110193808.2266294-26-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 25/33] 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":"Sat, 11 Jan 2020 01:17:23 -0000"}}]