[{"id":20736,"web_url":"https://patchwork.libcamera.org/comment/20736/","msgid":"<163641399493.948064.7503143066630957175@Monstersaurus>","date":"2021-11-08T23:26:34","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> After we unify identical streams to one stream configuration, the\n> capture requested by a HAL client can have been resolved as\n> CameraStream::Mapped. That is, a buffer to be written by a camera\n> is not provided by a client. We would handle this case by\n> dynamically allocating FrameBuffer.\n> \n> The existing FrameBufferAllocator cannot used because it is not\n> allowed to allocate a new buffer while Camera is running.\n> \n> This introduces PlatformFrameBufferAllocator. It allocates\n> FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> the underlying buffer but must be destroyed before\n> PlatformFrameBufferAllocator is destroyed.\n> \n> Signed-off-by: Hirokazu Honda \n\nI'm finding this one harder to grasp. It's late so I think I might have\nto continue tomorrow.\n\nI wish we had better infrastructure for unit tests on the Android layer,\nas I'm sure some of this would benefit from more tests, but even then\nwe'd have to have an environment to run the unit tests against the\nallocators...\n\n\n> ---\n> src/android/frame_buffer_allocator.h | 54 +++++++\n> .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 4 files changed, 292 insertions(+), 2 deletions(-)\n> create mode 100644 src/android/frame_buffer_allocator.h\n> create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> \n> diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> new file mode 100644\n> index 00000000..41ed4ae1\n> --- /dev/null\n> +++ b/src/android/frame_buffer_allocator.h\n> @@ -0,0 +1,54 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> + * platform dependent way.\n> + */\n> +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +class CameraDevice;\n> +\n> +class PlatformFrameBufferAllocator : libcamera::Extensible\n\nDoes the 'PlatformFrameBufferAllocater' benefit from using Extensible?\nIs it required somehow? Can't it just be a normal class that the CrOS\nand Android variants inherit from?\n\n\n> +{\n> + LIBCAMERA_DECLARE_PRIVATE()\n> +\n> +public:\n> + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> + ~PlatformFrameBufferAllocator();\n> +\n> + /*\n> + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> + * Note: The returned FrameBuffer needs to be destroyed before\n> + * PlatformFrameBufferAllocator is destroyed.\n> + */\n> + std::unique_ptr allocate(\n> + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +};\n> +\n> +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> + CameraDevice *const cameraDevice) \\\n> + : Extensible(std::make_unique(this, cameraDevice)) \\\n> +{ \\\n> +} \\\n> +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> +{ \\\n> +} \\\n> +std::unique_ptr \\\n> +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> + const libcamera::Size& size, \\\n> + uint32_t usage) \\\n> +{ \\\n> + return _d()->allocate(halPixelFormat, size, usage); \\\n> +}\n> +\n> +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> new file mode 100644\n> index 00000000..9c7e4ea4\n> --- /dev/null\n> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> @@ -0,0 +1,90 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> + * CameraBufferManager\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/framebuffer.h\"\n> +\n> +#include \"../camera_device.h\"\n> +#include \"../frame_buffer_allocator.h\"\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n> +class CrosFrameBufferData : public FrameBuffer::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> +public:\n> + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> + {\n> + }\n> +\n> + ~CrosFrameBufferData() override = default;\n> +\n> +private:\n> + cros::ScopedBufferHandle scopedHandle_;\n> +};\n> +} /* namespace */\n> +\n> +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> +\n> +public:\n> + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> + [[maybe_unused]] CameraDevice *const cameraDevice)\n> + {\n> + }\n> +\n> + ~Private() override = default;\n> +\n> + std::unique_ptr allocate(\n> + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +};\n> +\n> +std::unique_ptr\n> +PlatformFrameBufferAllocator::Private::allocate(\n> + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> +{\n> + cros::ScopedBufferHandle scopedHandle =\n> + cros::CameraBufferManager::AllocateScopedBuffer(\n> + size.width, size.height, halPixelFormat, usage);\n> + if (!scopedHandle) {\n> + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> + return nullptr;\n> + }\n> +\n> + buffer_handle_t handle = *scopedHandle;\n> + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> + std::vector planes(numPlanes);\n> + FileDescriptor fd{ handle->data[0] };\n> + if (!fd.isValid()) {\n> + LOG(HAL, Fatal) << \"Invalid fd\";\n> + return nullptr;\n> + }\n> +\n> + /* This code assumes all the planes are located in the same buffer. */\n> + for (auto [i, plane] : utils::enumerate(planes)) {\n> + plane.fd = fd;\n> + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> + }\n> +\n> + return std::make_unique(\n> + std::make_unique(std::move(scopedHandle)),\n> + planes);\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> new file mode 100644\n> index 00000000..167cc1a5\n> --- /dev/null\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -0,0 +1,144 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../camera_device.h\"\n> +#include \"../frame_buffer_allocator.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n> +class GenericFrameBufferData : public FrameBuffer::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> +\n> +public:\n> + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> + buffer_handle_t handle)\n> + : allocDevice_(allocDevice), handle_(handle)\n> + {\n> + ASSERT(allocDevice_);\n> + ASSERT(handle_);\n> + }\n> +\n> + ~GenericFrameBufferData() override\n> + {\n> + /*\n> + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> + * owned by PlatformFrameBufferAllocator::Private.\n> + * GenericFrameBufferData must be destroyed before it is\n> + * destroyed.\n> + *\n> + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> + * if this is difficult to maintain.\n> + *\n> + * Q: Thread safety against alloc_device_t is not documented.\n> + * Is it no problem to call alloc/free in parallel?\n> + */\n> + ASSERT(allocDevice_);\n> + allocDevice_->free(allocDevice_, handle_);\n> + }\n> +\n> +private:\n> + struct alloc_device_t *allocDevice_;\n> + const buffer_handle_t handle_;\n> +};\n> +} /* namespace */\n> +\n> +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> +\n> +public:\n> + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> + CameraDevice *const cameraDevice)\n> + : cameraDevice_(cameraDevice),\n> + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> + allocDevice_(nullptr)\n> + {\n> + ASSERT(hardwareModule_);\n> + }\n> +\n> + ~Private() override;\n> +\n> + std::unique_ptr allocate(\n> + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +\n> +private:\n> + const CameraDevice *const cameraDevice_;\n> + struct hw_module_t *const hardwareModule_;\n> + struct alloc_device_t *allocDevice_;\n> +};\n> +\n> +PlatformFrameBufferAllocator::Private::~Private()\n> +{\n> + if (allocDevice_)\n> + gralloc_close(allocDevice_);\n> +}\n> +\n> +std::unique_ptr\n> +PlatformFrameBufferAllocator::Private::allocate(\n> + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> +{\n> + if (!allocDevice_) {\n> + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> + if (ret) {\n> + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> + return nullptr;\n> + }\n> + }\n> +\n> + int stride = 0;\n> + buffer_handle_t handle = nullptr;\n> + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> + halPixelFormat, usage, &handle, &stride);\n> + if (ret) {\n> + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> + return nullptr;\n> + }\n> + if (!handle) {\n> + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> + return nullptr;\n> + }\n> +\n> + const libcamera::PixelFormat pixelFormat =\n> + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> + const auto &info = PixelFormatInfo::info(pixelFormat);\n> + const unsigned int numPlanes = info.numPlanes();\n> + std::vector planes(numPlanes);\n> +\n> + /* This code assumes the planes are mapped consecutively. */\n> + FileDescriptor fd{ handle->data[0] };\n> + size_t offset = 0;\n> + for (auto [i, plane] : utils::enumerate(planes)) {\n> + const size_t planeSize = info.planeSize(size.height, i, stride);\n> +\n> + planes[i].fd = fd;\n> + planes[i].offset = offset;\n> + planes[i].length = planeSize;\n> + offset += planeSize;\n> + }\n> +\n> + return std::make_unique(\n> + std::make_unique(allocDevice_, handle),\n> + planes);\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> index eeb5cc2e..d40a3b0b 100644\n> --- a/src/android/mm/meson.build\n> +++ b/src/android/mm/meson.build\n> @@ -2,8 +2,10 @@\n> \n> platform = get_option('android_platform')\n> if platform == 'generic'\n> - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> + android_hal_sources += files(['generic_camera_buffer.cpp',\n> + 'generic_frame_buffer_allocator.cpp'])\n> elif platform == 'cros'\n> - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> + android_hal_sources += files(['cros_camera_buffer.cpp',\n> + 'cros_frame_buffer_allocator.cpp'])\n> android_deps += [dependency('libcros_camera')]\n> endif\n> -- \n> 2.33.1.1089.g2158813163f-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1852BBF415\n\tfor ;\n\tMon, 8 Nov 2021 23:26:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44BEE60361;\n\tTue, 9 Nov 2021 00:26:39 +0100 (CET)","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 16DA860121\n\tfor ;\n\tTue, 9 Nov 2021 00:26:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3B7C8D4;\n\tTue, 9 Nov 2021 00:26:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IyeuTJ7h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636413997;\n\tbh=y1G24e0AUF/sg15tuzFBqlVn1hXJfwKBhK20F9zxT/Q=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=IyeuTJ7h5TOjj228+NS4jZnVq6l2/pZ7oIqKgzVjeDk/y6GA0y+vR1KkxPjn9TVgs\n\tO60IE+Omgu5YBYoOfi0tVKqTjt5dKwow1G/h+1uEO2HfuRon0AaTcmoLrmrHjpjYMD\n\tl6J8gMo2rHMlcz+LP2/wT/fX0xcfPqMFr2s4Jy4o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211101071652.107912-3-hiroh@chromium.org>","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>","From":"Kieran Bingham ","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","Date":"Mon, 08 Nov 2021 23:26:34 +0000","Message-ID":"<163641399493.948064.7503143066630957175@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20740,"web_url":"https://patchwork.libcamera.org/comment/20740/","msgid":"","date":"2021-11-09T05:46:14","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\nOn Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham\n wrote:\n>\n> Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> > After we unify identical streams to one stream configuration, the\n> > capture requested by a HAL client can have been resolved as\n> > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > is not provided by a client. We would handle this case by\n> > dynamically allocating FrameBuffer.\n> >\n> > The existing FrameBufferAllocator cannot used because it is not\n> > allowed to allocate a new buffer while Camera is running.\n> >\n> > This introduces PlatformFrameBufferAllocator. It allocates\n> > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > the underlying buffer but must be destroyed before\n> > PlatformFrameBufferAllocator is destroyed.\n> >\n> > Signed-off-by: Hirokazu Honda \n>\n> I'm finding this one harder to grasp. It's late so I think I might have\n> to continue tomorrow.\n>\n> I wish we had better infrastructure for unit tests on the Android layer,\n> as I'm sure some of this would benefit from more tests, but even then\n> we'd have to have an environment to run the unit tests against the\n> allocators...\n>\n>\n> > ---\n> > src/android/frame_buffer_allocator.h | 54 +++++++\n> > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> > src/android/mm/meson.build | 6 +-\n> > 4 files changed, 292 insertions(+), 2 deletions(-)\n> > create mode 100644 src/android/frame_buffer_allocator.h\n> > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> >\n> > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> > new file mode 100644\n> > index 00000000..41ed4ae1\n> > --- /dev/null\n> > +++ b/src/android/frame_buffer_allocator.h\n> > @@ -0,0 +1,54 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > + * platform dependent way.\n> > + */\n> > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +class CameraDevice;\n> > +\n> > +class PlatformFrameBufferAllocator : libcamera::Extensible\n>\n> Does the 'PlatformFrameBufferAllocater' benefit from using Extensible?\n> Is it required somehow? Can't it just be a normal class that the CrOS\n> and Android variants inherit from?\n>\n\nI followed CameraBuffer design pattern here.\nI am fine to make them normal classes.\nHow shall I decide which to be used in run time?\n\n-Hiro\n>\n> > +{\n> > + LIBCAMERA_DECLARE_PRIVATE()\n> > +\n> > +public:\n> > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> > + ~PlatformFrameBufferAllocator();\n> > +\n> > + /*\n> > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> > + * Note: The returned FrameBuffer needs to be destroyed before\n> > + * PlatformFrameBufferAllocator is destroyed.\n> > + */\n> > + std::unique_ptr allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > +};\n> > +\n> > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > + CameraDevice *const cameraDevice) \\\n> > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > +{ \\\n> > +} \\\n> > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > +{ \\\n> > +} \\\n> > +std::unique_ptr \\\n> > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > + const libcamera::Size& size, \\\n> > + uint32_t usage) \\\n> > +{ \\\n> > + return _d()->allocate(halPixelFormat, size, usage); \\\n> > +}\n> > +\n> > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > new file mode 100644\n> > index 00000000..9c7e4ea4\n> > --- /dev/null\n> > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > @@ -0,0 +1,90 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> > + * CameraBufferManager\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > +\n> > +#include \"../camera_device.h\"\n> > +#include \"../frame_buffer_allocator.h\"\n> > +#include \"cros-camera/camera_buffer_manager.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > +namespace {\n> > +class CrosFrameBufferData : public FrameBuffer::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > +public:\n> > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > + {\n> > + }\n> > +\n> > + ~CrosFrameBufferData() override = default;\n> > +\n> > +private:\n> > + cros::ScopedBufferHandle scopedHandle_;\n> > +};\n> > +} /* namespace */\n> > +\n> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > +\n> > +public:\n> > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > + {\n> > + }\n> > +\n> > + ~Private() override = default;\n> > +\n> > + std::unique_ptr allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > +};\n> > +\n> > +std::unique_ptr\n> > +PlatformFrameBufferAllocator::Private::allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > +{\n> > + cros::ScopedBufferHandle scopedHandle =\n> > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > + size.width, size.height, halPixelFormat, usage);\n> > + if (!scopedHandle) {\n> > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > + return nullptr;\n> > + }\n> > +\n> > + buffer_handle_t handle = *scopedHandle;\n> > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > + std::vector planes(numPlanes);\n> > + FileDescriptor fd{ handle->data[0] };\n> > + if (!fd.isValid()) {\n> > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > + return nullptr;\n> > + }\n> > +\n> > + /* This code assumes all the planes are located in the same buffer. */\n> > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > + plane.fd = fd;\n> > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > + }\n> > +\n> > + return std::make_unique(\n> > + std::make_unique(std::move(scopedHandle)),\n> > + planes);\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > new file mode 100644\n> > index 00000000..167cc1a5\n> > --- /dev/null\n> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > @@ -0,0 +1,144 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"../camera_device.h\"\n> > +#include \"../frame_buffer_allocator.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > +namespace {\n> > +class GenericFrameBufferData : public FrameBuffer::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > +\n> > +public:\n> > + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> > + buffer_handle_t handle)\n> > + : allocDevice_(allocDevice), handle_(handle)\n> > + {\n> > + ASSERT(allocDevice_);\n> > + ASSERT(handle_);\n> > + }\n> > +\n> > + ~GenericFrameBufferData() override\n> > + {\n> > + /*\n> > + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> > + * owned by PlatformFrameBufferAllocator::Private.\n> > + * GenericFrameBufferData must be destroyed before it is\n> > + * destroyed.\n> > + *\n> > + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> > + * if this is difficult to maintain.\n> > + *\n> > + * Q: Thread safety against alloc_device_t is not documented.\n> > + * Is it no problem to call alloc/free in parallel?\n> > + */\n> > + ASSERT(allocDevice_);\n> > + allocDevice_->free(allocDevice_, handle_);\n> > + }\n> > +\n> > +private:\n> > + struct alloc_device_t *allocDevice_;\n> > + const buffer_handle_t handle_;\n> > +};\n> > +} /* namespace */\n> > +\n> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > +\n> > +public:\n> > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > + CameraDevice *const cameraDevice)\n> > + : cameraDevice_(cameraDevice),\n> > + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> > + allocDevice_(nullptr)\n> > + {\n> > + ASSERT(hardwareModule_);\n> > + }\n> > +\n> > + ~Private() override;\n> > +\n> > + std::unique_ptr allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > +\n> > +private:\n> > + const CameraDevice *const cameraDevice_;\n> > + struct hw_module_t *const hardwareModule_;\n> > + struct alloc_device_t *allocDevice_;\n> > +};\n> > +\n> > +PlatformFrameBufferAllocator::Private::~Private()\n> > +{\n> > + if (allocDevice_)\n> > + gralloc_close(allocDevice_);\n> > +}\n> > +\n> > +std::unique_ptr\n> > +PlatformFrameBufferAllocator::Private::allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > +{\n> > + if (!allocDevice_) {\n> > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > + if (ret) {\n> > + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> > + return nullptr;\n> > + }\n> > + }\n> > +\n> > + int stride = 0;\n> > + buffer_handle_t handle = nullptr;\n> > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > + halPixelFormat, usage, &handle, &stride);\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > + return nullptr;\n> > + }\n> > + if (!handle) {\n> > + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> > + return nullptr;\n> > + }\n> > +\n> > + const libcamera::PixelFormat pixelFormat =\n> > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > + const unsigned int numPlanes = info.numPlanes();\n> > + std::vector planes(numPlanes);\n> > +\n> > + /* This code assumes the planes are mapped consecutively. */\n> > + FileDescriptor fd{ handle->data[0] };\n> > + size_t offset = 0;\n> > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > +\n> > + planes[i].fd = fd;\n> > + planes[i].offset = offset;\n> > + planes[i].length = planeSize;\n> > + offset += planeSize;\n> > + }\n> > +\n> > + return std::make_unique(\n> > + std::make_unique(allocDevice_, handle),\n> > + planes);\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > index eeb5cc2e..d40a3b0b 100644\n> > --- a/src/android/mm/meson.build\n> > +++ b/src/android/mm/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > platform = get_option('android_platform')\n> > if platform == 'generic'\n> > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > + 'generic_frame_buffer_allocator.cpp'])\n> > elif platform == 'cros'\n> > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > + 'cros_frame_buffer_allocator.cpp'])\n> > android_deps += [dependency('libcros_camera')]\n> > endif\n> > --\n> > 2.33.1.1089.g2158813163f-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05FBCBF415\n\tfor ;\n\tTue, 9 Nov 2021 05:46:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 276E16035D;\n\tTue, 9 Nov 2021 06:46:26 +0100 (CET)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02B6760121\n\tfor ;\n\tTue, 9 Nov 2021 06:46:24 +0100 (CET)","by mail-ed1-x532.google.com with SMTP id o8so71937013edc.3\n\tfor ;\n\tMon, 08 Nov 2021 21:46:24 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Xi+MJmYf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=IZaKJRYfCkTaG6/3BiJ18U+/kywpnL5kuR2JfHKAkbs=;\n\tb=Xi+MJmYfDzv+v7liaiL0c4Qt8UjmfJifUvF3qY2VIvEjend0bPj0MfNCk0KiNidv+i\n\tb99gsBRmgAy3Qe3TD8anPVCrMpllZKnPv274mGHBe8Q32JQO2eM7WLd0hEtPHFfSRFPN\n\t7QEKXi7aj2iVpee6tY4CeNf/H8sShe66C24Es=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IZaKJRYfCkTaG6/3BiJ18U+/kywpnL5kuR2JfHKAkbs=;\n\tb=hEm9Fx2hqQvSkx3mLJNrVRkpXDV8GO2K8qLLkfYFUegXqvutdx/HbjuLeUAUOvwG8O\n\tPIuPkeRcSZ+f0Hy6WguuAQs/fSf7S/dxtM9Hh72hXL+z7wm2RqzN4EXzB2uPG8XJtpLN\n\tfZhO278EKk084oA+rTUtGdmKzWGB3LS1hFnZRX/tYT7E7kkjg92WJBU51mJ2jDLG62gN\n\tPp+Uhb6cM3YkPTNhVSwWKwRO8KJxjwSrl7qruXuMABNgVlYx1QRyNrNZZac0sfvHUuHb\n\tJo2X7qICBq4lHSCn8DwKlAToib+O0Hp57cew7bDWOY25pKLwgO8vcQUvaQO+AXsfgOpj\n\tdRRQ==","X-Gm-Message-State":"AOAM532VCYGJQY0PcZ02EduKLYHgWb4EIt6ciCsWeBpk2sBCVpRmbkE2\n\tlysifuh1Jwl1FFxB9BHLNC6zNlU2GkwEYHeXlVemO7FpsBw=","X-Google-Smtp-Source":"ABdhPJyj8fDPFY8Xjkgdc/UVCgwYwHD5cGqsOspkT89Rl3OphzkbiswufnnEwerKFNXQeufKoCcVqM5zQH38vmHq2VA=","X-Received":"by 2002:a17:906:26ce:: with SMTP id\n\tu14mr6033955ejc.559.1636436784541; \n\tMon, 08 Nov 2021 21:46:24 -0800 (PST)","MIME-Version":"1.0","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>","In-Reply-To":"<163641399493.948064.7503143066630957175@Monstersaurus>","From":"Hirokazu Honda ","Date":"Tue, 9 Nov 2021 14:46:14 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20743,"web_url":"https://patchwork.libcamera.org/comment/20743/","msgid":"<20211109085726.x3xw7leybye4uwa4@uno.localdomain>","date":"2021-11-09T08:57:26","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello\n\nOn Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote:\n> Hi Kieran,\n>\n> On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham\n> wrote:\n> >\n> > Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> > > After we unify identical streams to one stream configuration, the\n> > > capture requested by a HAL client can have been resolved as\n> > > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > > is not provided by a client. We would handle this case by\n> > > dynamically allocating FrameBuffer.\n> > >\n> > > The existing FrameBufferAllocator cannot used because it is not\n> > > allowed to allocate a new buffer while Camera is running.\n> > >\n> > > This introduces PlatformFrameBufferAllocator. It allocates\n> > > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > > the underlying buffer but must be destroyed before\n> > > PlatformFrameBufferAllocator is destroyed.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> >\n> > I'm finding this one harder to grasp. It's late so I think I might have\n> > to continue tomorrow.\n> >\n> > I wish we had better infrastructure for unit tests on the Android layer,\n> > as I'm sure some of this would benefit from more tests, but even then\n> > we'd have to have an environment to run the unit tests against the\n> > allocators...\n> >\n> >\n> > > ---\n> > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 4 files changed, 292 insertions(+), 2 deletions(-)\n> > > create mode 100644 src/android/frame_buffer_allocator.h\n> > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> > >\n> > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> > > new file mode 100644\n> > > index 00000000..41ed4ae1\n> > > --- /dev/null\n> > > +++ b/src/android/frame_buffer_allocator.h\n> > > @@ -0,0 +1,54 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > > + * platform dependent way.\n> > > + */\n> > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +class PlatformFrameBufferAllocator : libcamera::Extensible\n> >\n> > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible?\n> > Is it required somehow? Can't it just be a normal class that the CrOS\n> > and Android variants inherit from?\n> >\n>\n> I followed CameraBuffer design pattern here.\n> I am fine to make them normal classes.\n> How shall I decide which to be used in run time?\n>\n\nI think compile time selection like it's done for CameraBuffer is\nbetter and I don't see what benefit would bring removing the Extensible\npattern.\n\nThanks\n j\n\n> -Hiro\n> >\n> > > +{\n> > > + LIBCAMERA_DECLARE_PRIVATE()\n> > > +\n> > > +public:\n> > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> > > + ~PlatformFrameBufferAllocator();\n> > > +\n> > > + /*\n> > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> > > + * Note: The returned FrameBuffer needs to be destroyed before\n> > > + * PlatformFrameBufferAllocator is destroyed.\n> > > + */\n> > > + std::unique_ptr allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > +};\n> > > +\n> > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > + CameraDevice *const cameraDevice) \\\n> > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > +{ \\\n> > > +} \\\n> > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > +{ \\\n> > > +} \\\n> > > +std::unique_ptr \\\n> > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > + const libcamera::Size& size, \\\n> > > + uint32_t usage) \\\n> > > +{ \\\n> > > + return _d()->allocate(halPixelFormat, size, usage); \\\n> > > +}\n> > > +\n> > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > new file mode 100644\n> > > index 00000000..9c7e4ea4\n> > > --- /dev/null\n> > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,90 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> > > + * CameraBufferManager\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/framebuffer.h\"\n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +#include \"../frame_buffer_allocator.h\"\n> > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\n> > > +\n> > > +namespace {\n> > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > +public:\n> > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > + {\n> > > + }\n> > > +\n> > > + ~CrosFrameBufferData() override = default;\n> > > +\n> > > +private:\n> > > + cros::ScopedBufferHandle scopedHandle_;\n> > > +};\n> > > +} /* namespace */\n> > > +\n> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > +\n> > > +public:\n> > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > + {\n> > > + }\n> > > +\n> > > + ~Private() override = default;\n> > > +\n> > > + std::unique_ptr allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > +};\n> > > +\n> > > +std::unique_ptr\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > +{\n> > > + cros::ScopedBufferHandle scopedHandle =\n> > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > + size.width, size.height, halPixelFormat, usage);\n> > > + if (!scopedHandle) {\n> > > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + buffer_handle_t handle = *scopedHandle;\n> > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > > + std::vector planes(numPlanes);\n> > > + FileDescriptor fd{ handle->data[0] };\n> > > + if (!fd.isValid()) {\n> > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + /* This code assumes all the planes are located in the same buffer. */\n> > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > + plane.fd = fd;\n> > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > > + }\n> > > +\n> > > + return std::make_unique(\n> > > + std::make_unique(std::move(scopedHandle)),\n> > > + planes);\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > new file mode 100644\n> > > index 00000000..167cc1a5\n> > > --- /dev/null\n> > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,144 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/formats.h\"\n> > > +#include \"libcamera/internal/framebuffer.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +#include \"../frame_buffer_allocator.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\n> > > +\n> > > +namespace {\n> > > +class GenericFrameBufferData : public FrameBuffer::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > +\n> > > +public:\n> > > + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> > > + buffer_handle_t handle)\n> > > + : allocDevice_(allocDevice), handle_(handle)\n> > > + {\n> > > + ASSERT(allocDevice_);\n> > > + ASSERT(handle_);\n> > > + }\n> > > +\n> > > + ~GenericFrameBufferData() override\n> > > + {\n> > > + /*\n> > > + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> > > + * owned by PlatformFrameBufferAllocator::Private.\n> > > + * GenericFrameBufferData must be destroyed before it is\n> > > + * destroyed.\n> > > + *\n> > > + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> > > + * if this is difficult to maintain.\n> > > + *\n> > > + * Q: Thread safety against alloc_device_t is not documented.\n> > > + * Is it no problem to call alloc/free in parallel?\n> > > + */\n> > > + ASSERT(allocDevice_);\n> > > + allocDevice_->free(allocDevice_, handle_);\n> > > + }\n> > > +\n> > > +private:\n> > > + struct alloc_device_t *allocDevice_;\n> > > + const buffer_handle_t handle_;\n> > > +};\n> > > +} /* namespace */\n> > > +\n> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > +\n> > > +public:\n> > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > + CameraDevice *const cameraDevice)\n> > > + : cameraDevice_(cameraDevice),\n> > > + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> > > + allocDevice_(nullptr)\n> > > + {\n> > > + ASSERT(hardwareModule_);\n> > > + }\n> > > +\n> > > + ~Private() override;\n> > > +\n> > > + std::unique_ptr allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > +\n> > > +private:\n> > > + const CameraDevice *const cameraDevice_;\n> > > + struct hw_module_t *const hardwareModule_;\n> > > + struct alloc_device_t *allocDevice_;\n> > > +};\n> > > +\n> > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > +{\n> > > + if (allocDevice_)\n> > > + gralloc_close(allocDevice_);\n> > > +}\n> > > +\n> > > +std::unique_ptr\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > +{\n> > > + if (!allocDevice_) {\n> > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > + if (ret) {\n> > > + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> > > + return nullptr;\n> > > + }\n> > > + }\n> > > +\n> > > + int stride = 0;\n> > > + buffer_handle_t handle = nullptr;\n> > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > + halPixelFormat, usage, &handle, &stride);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > + return nullptr;\n> > > + }\n> > > + if (!handle) {\n> > > + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + const libcamera::PixelFormat pixelFormat =\n> > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > > + const unsigned int numPlanes = info.numPlanes();\n> > > + std::vector planes(numPlanes);\n> > > +\n> > > + /* This code assumes the planes are mapped consecutively. */\n> > > + FileDescriptor fd{ handle->data[0] };\n> > > + size_t offset = 0;\n> > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > > +\n> > > + planes[i].fd = fd;\n> > > + planes[i].offset = offset;\n> > > + planes[i].length = planeSize;\n> > > + offset += planeSize;\n> > > + }\n> > > +\n> > > + return std::make_unique(\n> > > + std::make_unique(allocDevice_, handle),\n> > > + planes);\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > index eeb5cc2e..d40a3b0b 100644\n> > > --- a/src/android/mm/meson.build\n> > > +++ b/src/android/mm/meson.build\n> > > @@ -2,8 +2,10 @@\n> > >\n> > > platform = get_option('android_platform')\n> > > if platform == 'generic'\n> > > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > > + 'generic_frame_buffer_allocator.cpp'])\n> > > elif platform == 'cros'\n> > > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > > + 'cros_frame_buffer_allocator.cpp'])\n> > > android_deps += [dependency('libcros_camera')]\n> > > endif\n> > > --\n> > > 2.33.1.1089.g2158813163f-goog\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 21476BF415\n\tfor ;\n\tTue, 9 Nov 2021 08:56:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8C2F6035D;\n\tTue, 9 Nov 2021 09:56:34 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF7B9600B5\n\tfor ;\n\tTue, 9 Nov 2021 09:56:33 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 102211C0016;\n\tTue, 9 Nov 2021 08:56:32 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 09:57:26 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211109085726.x3xw7leybye4uwa4@uno.localdomain>","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20745,"web_url":"https://patchwork.libcamera.org/comment/20745/","msgid":"<163645285905.1317171.1464624108423094618@Monstersaurus>","date":"2021-11-09T10:14:19","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-09 08:57:26)\n> Hello\n> \n> On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote:\n> > Hi Kieran,\n> >\n> > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> > > > After we unify identical streams to one stream configuration, the\n> > > > capture requested by a HAL client can have been resolved as\n> > > > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > > > is not provided by a client. We would handle this case by\n> > > > dynamically allocating FrameBuffer.\n> > > >\n> > > > The existing FrameBufferAllocator cannot used because it is not\n> > > > allowed to allocate a new buffer while Camera is running.\n> > > >\n> > > > This introduces PlatformFrameBufferAllocator. It allocates\n> > > > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > > > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > > > the underlying buffer but must be destroyed before\n> > > > PlatformFrameBufferAllocator is destroyed.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > >\n> > > I'm finding this one harder to grasp. It's late so I think I might have\n> > > to continue tomorrow.\n> > >\n> > > I wish we had better infrastructure for unit tests on the Android layer,\n> > > as I'm sure some of this would benefit from more tests, but even then\n> > > we'd have to have an environment to run the unit tests against the\n> > > allocators...\n> > >\n> > >\n> > > > ---\n> > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> > > > src/android/mm/meson.build | 6 +-\n> > > > 4 files changed, 292 insertions(+), 2 deletions(-)\n> > > > create mode 100644 src/android/frame_buffer_allocator.h\n> > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> > > >\n> > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> > > > new file mode 100644\n> > > > index 00000000..41ed4ae1\n> > > > --- /dev/null\n> > > > +++ b/src/android/frame_buffer_allocator.h\n> > > > @@ -0,0 +1,54 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > > > + * platform dependent way.\n> > > > + */\n> > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +class CameraDevice;\n> > > > +\n> > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\n> > >\n> > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible?\n> > > Is it required somehow? Can't it just be a normal class that the CrOS\n> > > and Android variants inherit from?\n> > >\n> >\n> > I followed CameraBuffer design pattern here.\n> > I am fine to make them normal classes.\n> > How shall I decide which to be used in run time?\n> >\n> \n> I think compile time selection like it's done for CameraBuffer is\n> better and I don't see what benefit would bring removing the Extensible\n> pattern.\n\nAha, that's the part I'd missed last night. Yes this is fine for\nExtensible I think.\n\n \n> Thanks\n> j\n> \n> > -Hiro\n> > >\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PRIVATE()\n> > > > +\n> > > > +public:\n> > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> > > > + ~PlatformFrameBufferAllocator();\n> > > > +\n> > > > + /*\n> > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> > > > + * Note: The returned FrameBuffer needs to be destroyed before\n> > > > + * PlatformFrameBufferAllocator is destroyed.\n> > > > + */\n> > > > + std::unique_ptr allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > +};\n> > > > +\n> > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > > + CameraDevice *const cameraDevice) \\\n> > > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > > +{ \\\n> > > > +} \\\n> > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > > +{ \\\n> > > > +} \\\n> > > > +std::unique_ptr \\\n> > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > > + const libcamera::Size& size, \\\n> > > > + uint32_t usage) \\\n> > > > +{ \\\n> > > > + return _d()->allocate(halPixelFormat, size, usage); \\\n> > > > +}\n> > > > +\n> > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > new file mode 100644\n> > > > index 00000000..9c7e4ea4\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > @@ -0,0 +1,90 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> > > > + * CameraBufferManager\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > +\n> > > > +#include \"../camera_device.h\"\n> > > > +#include \"../frame_buffer_allocator.h\"\n> > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > +\n> > > > +namespace {\n> > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > +public:\n> > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + ~CrosFrameBufferData() override = default;\n> > > > +\n> > > > +private:\n> > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > +};\n> > > > +} /* namespace */\n> > > > +\n> > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > +\n> > > > +public:\n> > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + ~Private() override = default;\n> > > > +\n> > > > + std::unique_ptr allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > +};\n> > > > +\n> > > > +std::unique_ptr\n> > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > +{\n> > > > + cros::ScopedBufferHandle scopedHandle =\n> > > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > > + size.width, size.height, halPixelFormat, usage);\n> > > > + if (!scopedHandle) {\n> > > > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + buffer_handle_t handle = *scopedHandle;\n> > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > > > + std::vector planes(numPlanes);\n> > > > + FileDescriptor fd{ handle->data[0] };\n> > > > + if (!fd.isValid()) {\n> > > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + /* This code assumes all the planes are located in the same buffer. */\n> > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > + plane.fd = fd;\n> > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > > > + }\n> > > > +\n> > > > + return std::make_unique(\n> > > > + std::make_unique(std::move(scopedHandle)),\n> > > > + planes);\n> > > > +}\n> > > > +\n> > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > new file mode 100644\n> > > > index 00000000..167cc1a5\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > @@ -0,0 +1,144 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"../camera_device.h\"\n> > > > +#include \"../frame_buffer_allocator.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > +\n> > > > +namespace {\n> > > > +class GenericFrameBufferData : public FrameBuffer::Private\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > +\n> > > > +public:\n> > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> > > > + buffer_handle_t handle)\n> > > > + : allocDevice_(allocDevice), handle_(handle)\n> > > > + {\n> > > > + ASSERT(allocDevice_);\n> > > > + ASSERT(handle_);\n> > > > + }\n> > > > +\n> > > > + ~GenericFrameBufferData() override\n> > > > + {\n> > > > + /*\n> > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> > > > + * owned by PlatformFrameBufferAllocator::Private.\n> > > > + * GenericFrameBufferData must be destroyed before it is\n> > > > + * destroyed.\n> > > > + *\n> > > > + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> > > > + * if this is difficult to maintain.\n> > > > + *\n> > > > + * Q: Thread safety against alloc_device_t is not documented.\n> > > > + * Is it no problem to call alloc/free in parallel?\n> > > > + */\n> > > > + ASSERT(allocDevice_);\n> > > > + allocDevice_->free(allocDevice_, handle_);\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + struct alloc_device_t *allocDevice_;\n> > > > + const buffer_handle_t handle_;\n> > > > +};\n> > > > +} /* namespace */\n> > > > +\n> > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > +\n> > > > +public:\n> > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > + CameraDevice *const cameraDevice)\n> > > > + : cameraDevice_(cameraDevice),\n> > > > + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> > > > + allocDevice_(nullptr)\n> > > > + {\n> > > > + ASSERT(hardwareModule_);\n> > > > + }\n> > > > +\n> > > > + ~Private() override;\n> > > > +\n> > > > + std::unique_ptr allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > +\n> > > > +private:\n> > > > + const CameraDevice *const cameraDevice_;\n> > > > + struct hw_module_t *const hardwareModule_;\n> > > > + struct alloc_device_t *allocDevice_;\n> > > > +};\n> > > > +\n> > > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > > +{\n> > > > + if (allocDevice_)\n> > > > + gralloc_close(allocDevice_);\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr\n> > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > +{\n> > > > + if (!allocDevice_) {\n> > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > > + if (ret) {\n> > > > + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> > > > + return nullptr;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + int stride = 0;\n> > > > + buffer_handle_t handle = nullptr;\n> > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > > + halPixelFormat, usage, &handle, &stride);\n> > > > + if (ret) {\n> > > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > > + return nullptr;\n> > > > + }\n> > > > + if (!handle) {\n> > > > + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + const libcamera::PixelFormat pixelFormat =\n> > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > > > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > + std::vector planes(numPlanes);\n> > > > +\n> > > > + /* This code assumes the planes are mapped consecutively. */\n> > > > + FileDescriptor fd{ handle->data[0] };\n> > > > + size_t offset = 0;\n> > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > > > +\n> > > > + planes[i].fd = fd;\n> > > > + planes[i].offset = offset;\n> > > > + planes[i].length = planeSize;\n> > > > + offset += planeSize;\n> > > > + }\n> > > > +\n> > > > + return std::make_unique(\n> > > > + std::make_unique(allocDevice_, handle),\n> > > > + planes);\n> > > > +}\n> > > > +\n> > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > > index eeb5cc2e..d40a3b0b 100644\n> > > > --- a/src/android/mm/meson.build\n> > > > +++ b/src/android/mm/meson.build\n> > > > @@ -2,8 +2,10 @@\n> > > >\n> > > > platform = get_option('android_platform')\n> > > > if platform == 'generic'\n> > > > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > > > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > > > + 'generic_frame_buffer_allocator.cpp'])\n> > > > elif platform == 'cros'\n> > > > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > > > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > > > + 'cros_frame_buffer_allocator.cpp'])\n> > > > android_deps += [dependency('libcros_camera')]\n> > > > endif\n> > > > --\n> > > > 2.33.1.1089.g2158813163f-goog\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6249FBF415\n\tfor ;\n\tTue, 9 Nov 2021 10:14:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6EA976034E;\n\tTue, 9 Nov 2021 11:14:23 +0100 (CET)","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 EBA1D600B5\n\tfor ;\n\tTue, 9 Nov 2021 11:14:21 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6AFAA501;\n\tTue, 9 Nov 2021 11:14:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V+fN65Sa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636452861;\n\tbh=L4uDKGSL0d0RdajKiC4PWH4TwI/PBcHcBjLbihibBLo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=V+fN65SaASaAVI90wyG9Vnnx6f/bbSMQxp26lXjz7PCK7VTQPzbDFmr4vsy463q5y\n\tZyqY0nn1z5FzyizUaV6h34MiPscc46gQHZxr02p23xhqV7Zci0H+bczRV/KkPBvu3Z\n\tjouZPuPUjVJ1stEAbd15EMTRnjSEWev1HgilMpoc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109085726.x3xw7leybye4uwa4@uno.localdomain>","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>","From":"Kieran Bingham ","To":"Hirokazu Honda , Jacopo Mondi ","Date":"Tue, 09 Nov 2021 10:14:19 +0000","Message-ID":"<163645285905.1317171.1464624108423094618@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20775,"web_url":"https://patchwork.libcamera.org/comment/20775/","msgid":"<163648987990.1743947.14546877859839703594@Monstersaurus>","date":"2021-11-09T20:31:19","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2021-11-09 10:14:19)\n> Quoting Jacopo Mondi (2021-11-09 08:57:26)\n> > Hello\n> > \n> > On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote:\n> > > Hi Kieran,\n> > >\n> > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> > > > > After we unify identical streams to one stream configuration, the\n> > > > > capture requested by a HAL client can have been resolved as\n> > > > > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > > > > is not provided by a client. We would handle this case by\n> > > > > dynamically allocating FrameBuffer.\n> > > > >\n> > > > > The existing FrameBufferAllocator cannot used because it is not\n> > > > > allowed to allocate a new buffer while Camera is running.\n> > > > >\n> > > > > This introduces PlatformFrameBufferAllocator. It allocates\n> > > > > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > > > > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > > > > the underlying buffer but must be destroyed before\n> > > > > PlatformFrameBufferAllocator is destroyed.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > I'm finding this one harder to grasp. It's late so I think I might have\n> > > > to continue tomorrow.\n> > > >\n> > > > I wish we had better infrastructure for unit tests on the Android layer,\n> > > > as I'm sure some of this would benefit from more tests, but even then\n> > > > we'd have to have an environment to run the unit tests against the\n> > > > allocators...\n> > > >\n> > > >\n> > > > > ---\n> > > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> > > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> > > > > src/android/mm/meson.build | 6 +-\n> > > > > 4 files changed, 292 insertions(+), 2 deletions(-)\n> > > > > create mode 100644 src/android/frame_buffer_allocator.h\n> > > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > >\n> > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> > > > > new file mode 100644\n> > > > > index 00000000..41ed4ae1\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/frame_buffer_allocator.h\n> > > > > @@ -0,0 +1,54 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > > > > + * platform dependent way.\n> > > > > + */\n> > > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +class CameraDevice;\n> > > > > +\n> > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\n> > > >\n> > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible?\n> > > > Is it required somehow? Can't it just be a normal class that the CrOS\n> > > > and Android variants inherit from?\n> > > >\n> > >\n> > > I followed CameraBuffer design pattern here.\n> > > I am fine to make them normal classes.\n> > > How shall I decide which to be used in run time?\n> > >\n> > \n> > I think compile time selection like it's done for CameraBuffer is\n> > better and I don't see what benefit would bring removing the Extensible\n> > pattern.\n> \n> Aha, that's the part I'd missed last night. Yes this is fine for\n> Extensible I think.\n\nIn fact, it doesn't matter too much - but I still don't see why this\nneeds to be Extensible.\n\nThe implementation for PlatformFrameBufferAllocator can have an\nidentically named class in both \n\tsrc/android/mm/cros_frame_buffer_allocator.cpp\n\tsrc/android/mm/generic_frame_buffer_allocator.cpp\n\nAnd the one that gets used will be the one that is compiled in...?\n\n\n> > Thanks\n> > j\n> > \n> > > -Hiro\n> > > >\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PRIVATE()\n> > > > > +\n> > > > > +public:\n> > > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> > > > > + ~PlatformFrameBufferAllocator();\n> > > > > +\n> > > > > + /*\n> > > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> > > > > + * Note: The returned FrameBuffer needs to be destroyed before\n> > > > > + * PlatformFrameBufferAllocator is destroyed.\n> > > > > + */\n> > > > > + std::unique_ptr allocate(\n> > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > +};\n> > > > > +\n> > > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> > > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > > > + CameraDevice *const cameraDevice) \\\n> > > > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > > > +{ \\\n> > > > > +} \\\n> > > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > > > +{ \\\n> > > > > +} \\\n> > > > > +std::unique_ptr \\\n> > > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > > > + const libcamera::Size& size, \\\n> > > > > + uint32_t usage) \\\n> > > > > +{ \\\n> > > > > + return _d()->allocate(halPixelFormat, size, usage); \\\n> > > > > +}\n> > > > > +\n> > > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> > > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..9c7e4ea4\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > @@ -0,0 +1,90 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> > > > > + * CameraBufferManager\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > +\n> > > > > +#include \"../camera_device.h\"\n> > > > > +#include \"../frame_buffer_allocator.h\"\n> > > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > +\n> > > > > +namespace {\n> > > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > > +public:\n> > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > > + {\n> > > > > + }\n> > > > > +\n> > > > > + ~CrosFrameBufferData() override = default;\n> > > > > +\n> > > > > +private:\n> > > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > > +};\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > > +\n> > > > > +public:\n> > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > > + {\n> > > > > + }\n> > > > > +\n> > > > > + ~Private() override = default;\n> > > > > +\n> > > > > + std::unique_ptr allocate(\n> > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > +};\n> > > > > +\n> > > > > +std::unique_ptr\n> > > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > > +{\n> > > > > + cros::ScopedBufferHandle scopedHandle =\n> > > > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > > > + size.width, size.height, halPixelFormat, usage);\n> > > > > + if (!scopedHandle) {\n> > > > > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + buffer_handle_t handle = *scopedHandle;\n> > > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > > > > + std::vector planes(numPlanes);\n> > > > > + FileDescriptor fd{ handle->data[0] };\n> > > > > + if (!fd.isValid()) {\n> > > > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + /* This code assumes all the planes are located in the same buffer. */\n> > > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > > + plane.fd = fd;\n> > > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > > > > + }\n> > > > > +\n> > > > > + return std::make_unique(\n> > > > > + std::make_unique(std::move(scopedHandle)),\n> > > > > + planes);\n> > > > > +}\n> > > > > +\n> > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..167cc1a5\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > @@ -0,0 +1,144 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"../camera_device.h\"\n> > > > > +#include \"../frame_buffer_allocator.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > +\n> > > > > +namespace {\n> > > > > +class GenericFrameBufferData : public FrameBuffer::Private\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > > +\n> > > > > +public:\n> > > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> > > > > + buffer_handle_t handle)\n> > > > > + : allocDevice_(allocDevice), handle_(handle)\n> > > > > + {\n> > > > > + ASSERT(allocDevice_);\n> > > > > + ASSERT(handle_);\n> > > > > + }\n> > > > > +\n> > > > > + ~GenericFrameBufferData() override\n> > > > > + {\n> > > > > + /*\n> > > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> > > > > + * owned by PlatformFrameBufferAllocator::Private.\n> > > > > + * GenericFrameBufferData must be destroyed before it is\n> > > > > + * destroyed.\n> > > > > + *\n> > > > > + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> > > > > + * if this is difficult to maintain.\n> > > > > + *\n> > > > > + * Q: Thread safety against alloc_device_t is not documented.\n> > > > > + * Is it no problem to call alloc/free in parallel?\n> > > > > + */\n> > > > > + ASSERT(allocDevice_);\n> > > > > + allocDevice_->free(allocDevice_, handle_);\n> > > > > + }\n> > > > > +\n> > > > > +private:\n> > > > > + struct alloc_device_t *allocDevice_;\n> > > > > + const buffer_handle_t handle_;\n> > > > > +};\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > > +\n> > > > > +public:\n> > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > > + CameraDevice *const cameraDevice)\n> > > > > + : cameraDevice_(cameraDevice),\n> > > > > + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> > > > > + allocDevice_(nullptr)\n> > > > > + {\n> > > > > + ASSERT(hardwareModule_);\n> > > > > + }\n> > > > > +\n> > > > > + ~Private() override;\n> > > > > +\n> > > > > + std::unique_ptr allocate(\n> > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > +\n> > > > > +private:\n> > > > > + const CameraDevice *const cameraDevice_;\n> > > > > + struct hw_module_t *const hardwareModule_;\n> > > > > + struct alloc_device_t *allocDevice_;\n> > > > > +};\n> > > > > +\n> > > > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > > > +{\n> > > > > + if (allocDevice_)\n> > > > > + gralloc_close(allocDevice_);\n> > > > > +}\n> > > > > +\n> > > > > +std::unique_ptr\n> > > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > > +{\n> > > > > + if (!allocDevice_) {\n> > > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > > > + if (ret) {\n> > > > > + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + int stride = 0;\n> > > > > + buffer_handle_t handle = nullptr;\n> > > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > > > + halPixelFormat, usage, &handle, &stride);\n> > > > > + if (ret) {\n> > > > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > + if (!handle) {\n> > > > > + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + const libcamera::PixelFormat pixelFormat =\n> > > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > > > > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > > + std::vector planes(numPlanes);\n> > > > > +\n> > > > > + /* This code assumes the planes are mapped consecutively. */\n> > > > > + FileDescriptor fd{ handle->data[0] };\n> > > > > + size_t offset = 0;\n> > > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > > > > +\n> > > > > + planes[i].fd = fd;\n> > > > > + planes[i].offset = offset;\n> > > > > + planes[i].length = planeSize;\n> > > > > + offset += planeSize;\n> > > > > + }\n> > > > > +\n> > > > > + return std::make_unique(\n> > > > > + std::make_unique(allocDevice_, handle),\n> > > > > + planes);\n> > > > > +}\n> > > > > +\n> > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > > > index eeb5cc2e..d40a3b0b 100644\n> > > > > --- a/src/android/mm/meson.build\n> > > > > +++ b/src/android/mm/meson.build\n> > > > > @@ -2,8 +2,10 @@\n> > > > >\n> > > > > platform = get_option('android_platform')\n> > > > > if platform == 'generic'\n> > > > > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > > > > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > > > > + 'generic_frame_buffer_allocator.cpp'])\n> > > > > elif platform == 'cros'\n> > > > > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > > > > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > > > > + 'cros_frame_buffer_allocator.cpp'])\n> > > > > android_deps += [dependency('libcros_camera')]\n> > > > > endif\n> > > > > --\n> > > > > 2.33.1.1089.g2158813163f-goog\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49141BF415\n\tfor ;\n\tTue, 9 Nov 2021 20:31:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 920526035D;\n\tTue, 9 Nov 2021 21:31:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83859600BF\n\tfor ;\n\tTue, 9 Nov 2021 21:31:23 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03398FD1;\n\tTue, 9 Nov 2021 21:31:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"b4VOHKJm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636489883;\n\tbh=c2RLZkzVcduZ7RZXZqPdnjKpivvs63+uJZ657k2r1yA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=b4VOHKJmAOyT5XiWYHYg3kNSEpdr7Zqll4keXdD6HZKarwbkpfe91nO7oXZG9yfnV\n\tgoHL10Td+FG+odm3U80D0DTyeu8O2IudjJvMwbrB+MZpLdyrxCjrAC4wC8v3uabjUD\n\tXI6ZUfxsgqgh3hS8PtMsI9q8hNFxmY2QJtAplY3w=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163645285905.1317171.1464624108423094618@Monstersaurus>","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>","From":"Kieran Bingham ","To":"Hirokazu Honda , Jacopo Mondi ","Date":"Tue, 09 Nov 2021 20:31:19 +0000","Message-ID":"<163648987990.1743947.14546877859839703594@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20777,"web_url":"https://patchwork.libcamera.org/comment/20777/","msgid":"","date":"2021-11-10T03:09:34","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\n\nOn Wed, Nov 10, 2021 at 5:31 AM Kieran Bingham\n wrote:\n>\n> Quoting Kieran Bingham (2021-11-09 10:14:19)\n> > Quoting Jacopo Mondi (2021-11-09 08:57:26)\n> > > Hello\n> > >\n> > > On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote:\n> > > > Hi Kieran,\n> > > >\n> > > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham\n> > > > wrote:\n> > > > >\n> > > > > Quoting Hirokazu Honda (2021-11-01 07:16:52)\n> > > > > > After we unify identical streams to one stream configuration, the\n> > > > > > capture requested by a HAL client can have been resolved as\n> > > > > > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > > > > > is not provided by a client. We would handle this case by\n> > > > > > dynamically allocating FrameBuffer.\n> > > > > >\n> > > > > > The existing FrameBufferAllocator cannot used because it is not\n> > > > > > allowed to allocate a new buffer while Camera is running.\n> > > > > >\n> > > > > > This introduces PlatformFrameBufferAllocator. It allocates\n> > > > > > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > > > > > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > > > > > the underlying buffer but must be destroyed before\n> > > > > > PlatformFrameBufferAllocator is destroyed.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > >\n> > > > > I'm finding this one harder to grasp. It's late so I think I might have\n> > > > > to continue tomorrow.\n> > > > >\n> > > > > I wish we had better infrastructure for unit tests on the Android layer,\n> > > > > as I'm sure some of this would benefit from more tests, but even then\n> > > > > we'd have to have an environment to run the unit tests against the\n> > > > > allocators...\n> > > > >\n> > > > >\n> > > > > > ---\n> > > > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> > > > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> > > > > > src/android/mm/meson.build | 6 +-\n> > > > > > 4 files changed, 292 insertions(+), 2 deletions(-)\n> > > > > > create mode 100644 src/android/frame_buffer_allocator.h\n> > > > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > >\n> > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> > > > > > new file mode 100644\n> > > > > > index 00000000..41ed4ae1\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/android/frame_buffer_allocator.h\n> > > > > > @@ -0,0 +1,54 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > > + *\n> > > > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > > > > > + * platform dependent way.\n> > > > > > + */\n> > > > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +class CameraDevice;\n> > > > > > +\n> > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\n> > > > >\n> > > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible?\n> > > > > Is it required somehow? Can't it just be a normal class that the CrOS\n> > > > > and Android variants inherit from?\n> > > > >\n> > > >\n> > > > I followed CameraBuffer design pattern here.\n> > > > I am fine to make them normal classes.\n> > > > How shall I decide which to be used in run time?\n> > > >\n> > >\n> > > I think compile time selection like it's done for CameraBuffer is\n> > > better and I don't see what benefit would bring removing the Extensible\n> > > pattern.\n> >\n> > Aha, that's the part I'd missed last night. Yes this is fine for\n> > Extensible I think.\n>\n> In fact, it doesn't matter too much - but I still don't see why this\n> needs to be Extensible.\n>\n> The implementation for PlatformFrameBufferAllocator can have an\n> identically named class in both\n> src/android/mm/cros_frame_buffer_allocator.cpp\n> src/android/mm/generic_frame_buffer_allocator.cpp\n>\n> And the one that gets used will be the one that is compiled in...?\n>\n\nThat's a good point. I couldn't find a reason for this.\nIf this is applicable for PlatformFrameBufferAllocator, I think we can\nchange CameraBuffer too.\n\n-Hiro\n>\n> > > Thanks\n> > > j\n> > >\n> > > > -Hiro\n> > > > >\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PRIVATE()\n> > > > > > +\n> > > > > > +public:\n> > > > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> > > > > > + ~PlatformFrameBufferAllocator();\n> > > > > > +\n> > > > > > + /*\n> > > > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> > > > > > + * Note: The returned FrameBuffer needs to be destroyed before\n> > > > > > + * PlatformFrameBufferAllocator is destroyed.\n> > > > > > + */\n> > > > > > + std::unique_ptr allocate(\n> > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > > +};\n> > > > > > +\n> > > > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \\\n> > > > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > > > > + CameraDevice *const cameraDevice) \\\n> > > > > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > > > > +{ \\\n> > > > > > +} \\\n> > > > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > > > > +{ \\\n> > > > > > +} \\\n> > > > > > +std::unique_ptr \\\n> > > > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > > > > + const libcamera::Size& size, \\\n> > > > > > + uint32_t usage) \\\n> > > > > > +{ \\\n> > > > > > + return _d()->allocate(halPixelFormat, size, usage); \\\n> > > > > > +}\n> > > > > > +\n> > > > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> > > > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..9c7e4ea4\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > > @@ -0,0 +1,90 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > > + *\n> > > > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> > > > > > + * CameraBufferManager\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > +\n> > > > > > +#include \"../camera_device.h\"\n> > > > > > +#include \"../frame_buffer_allocator.h\"\n> > > > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > > > +\n> > > > > > +using namespace libcamera;\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > > +\n> > > > > > +namespace {\n> > > > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > > > +public:\n> > > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > > > + {\n> > > > > > + }\n> > > > > > +\n> > > > > > + ~CrosFrameBufferData() override = default;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > > > +};\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > > > +\n> > > > > > +public:\n> > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > > > + {\n> > > > > > + }\n> > > > > > +\n> > > > > > + ~Private() override = default;\n> > > > > > +\n> > > > > > + std::unique_ptr allocate(\n> > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > > +};\n> > > > > > +\n> > > > > > +std::unique_ptr\n> > > > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > > > +{\n> > > > > > + cros::ScopedBufferHandle scopedHandle =\n> > > > > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > > > > + size.width, size.height, halPixelFormat, usage);\n> > > > > > + if (!scopedHandle) {\n> > > > > > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > > +\n> > > > > > + buffer_handle_t handle = *scopedHandle;\n> > > > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > > > > > + std::vector planes(numPlanes);\n> > > > > > + FileDescriptor fd{ handle->data[0] };\n> > > > > > + if (!fd.isValid()) {\n> > > > > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* This code assumes all the planes are located in the same buffer. */\n> > > > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > > > + plane.fd = fd;\n> > > > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > > > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > > > > > + }\n> > > > > > +\n> > > > > > + return std::make_unique(\n> > > > > > + std::make_unique(std::move(scopedHandle)),\n> > > > > > + planes);\n> > > > > > +}\n> > > > > > +\n> > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..167cc1a5\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > > @@ -0,0 +1,144 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > > + *\n> > > > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"../camera_device.h\"\n> > > > > > +#include \"../frame_buffer_allocator.h\"\n> > > > > > +\n> > > > > > +using namespace libcamera;\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > > +\n> > > > > > +namespace {\n> > > > > > +class GenericFrameBufferData : public FrameBuffer::Private\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > > > +\n> > > > > > +public:\n> > > > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice,\n> > > > > > + buffer_handle_t handle)\n> > > > > > + : allocDevice_(allocDevice), handle_(handle)\n> > > > > > + {\n> > > > > > + ASSERT(allocDevice_);\n> > > > > > + ASSERT(handle_);\n> > > > > > + }\n> > > > > > +\n> > > > > > + ~GenericFrameBufferData() override\n> > > > > > + {\n> > > > > > + /*\n> > > > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is\n> > > > > > + * owned by PlatformFrameBufferAllocator::Private.\n> > > > > > + * GenericFrameBufferData must be destroyed before it is\n> > > > > > + * destroyed.\n> > > > > > + *\n> > > > > > + * \\todo: Consider managing alloc_device_t with std::shared_ptr\n> > > > > > + * if this is difficult to maintain.\n> > > > > > + *\n> > > > > > + * Q: Thread safety against alloc_device_t is not documented.\n> > > > > > + * Is it no problem to call alloc/free in parallel?\n> > > > > > + */\n> > > > > > + ASSERT(allocDevice_);\n> > > > > > + allocDevice_->free(allocDevice_, handle_);\n> > > > > > + }\n> > > > > > +\n> > > > > > +private:\n> > > > > > + struct alloc_device_t *allocDevice_;\n> > > > > > + const buffer_handle_t handle_;\n> > > > > > +};\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > > > > > +\n> > > > > > +public:\n> > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > > > > > + CameraDevice *const cameraDevice)\n> > > > > > + : cameraDevice_(cameraDevice),\n> > > > > > + hardwareModule_(cameraDevice->camera3Device()->common.module),\n> > > > > > + allocDevice_(nullptr)\n> > > > > > + {\n> > > > > > + ASSERT(hardwareModule_);\n> > > > > > + }\n> > > > > > +\n> > > > > > + ~Private() override;\n> > > > > > +\n> > > > > > + std::unique_ptr allocate(\n> > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > > > > > +\n> > > > > > +private:\n> > > > > > + const CameraDevice *const cameraDevice_;\n> > > > > > + struct hw_module_t *const hardwareModule_;\n> > > > > > + struct alloc_device_t *allocDevice_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > > > > +{\n> > > > > > + if (allocDevice_)\n> > > > > > + gralloc_close(allocDevice_);\n> > > > > > +}\n> > > > > > +\n> > > > > > +std::unique_ptr\n> > > > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > > > > > +{\n> > > > > > + if (!allocDevice_) {\n> > > > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > > > > + if (ret) {\n> > > > > > + LOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + int stride = 0;\n> > > > > > + buffer_handle_t handle = nullptr;\n> > > > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > > > > + halPixelFormat, usage, &handle, &stride);\n> > > > > > + if (ret) {\n> > > > > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > > + if (!handle) {\n> > > > > > + LOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > > +\n> > > > > > + const libcamera::PixelFormat pixelFormat =\n> > > > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > > > > > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > > > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > > > + std::vector planes(numPlanes);\n> > > > > > +\n> > > > > > + /* This code assumes the planes are mapped consecutively. */\n> > > > > > + FileDescriptor fd{ handle->data[0] };\n> > > > > > + size_t offset = 0;\n> > > > > > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > > > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > > > > > +\n> > > > > > + planes[i].fd = fd;\n> > > > > > + planes[i].offset = offset;\n> > > > > > + planes[i].length = planeSize;\n> > > > > > + offset += planeSize;\n> > > > > > + }\n> > > > > > +\n> > > > > > + return std::make_unique(\n> > > > > > + std::make_unique(allocDevice_, handle),\n> > > > > > + planes);\n> > > > > > +}\n> > > > > > +\n> > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > > > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > > > > index eeb5cc2e..d40a3b0b 100644\n> > > > > > --- a/src/android/mm/meson.build\n> > > > > > +++ b/src/android/mm/meson.build\n> > > > > > @@ -2,8 +2,10 @@\n> > > > > >\n> > > > > > platform = get_option('android_platform')\n> > > > > > if platform == 'generic'\n> > > > > > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > > > > > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > > > > > + 'generic_frame_buffer_allocator.cpp'])\n> > > > > > elif platform == 'cros'\n> > > > > > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > > > > > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > > > > > + 'cros_frame_buffer_allocator.cpp'])\n> > > > > > android_deps += [dependency('libcros_camera')]\n> > > > > > endif\n> > > > > > --\n> > > > > > 2.33.1.1089.g2158813163f-goog\n> > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BBD83BF415\n\tfor ;\n\tWed, 10 Nov 2021 03:09:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CD4C6035D;\n\tWed, 10 Nov 2021 04:09:48 +0100 (CET)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A8B960234\n\tfor ;\n\tWed, 10 Nov 2021 04:09:46 +0100 (CET)","by mail-ed1-x531.google.com with SMTP id g14so4732482edz.2\n\tfor ;\n\tTue, 09 Nov 2021 19:09:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"MYReHs1d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=pBkdgvEu28A6yB6OLPVOTAlBdIBNNRiCaiLS2iPOT3Q=;\n\tb=MYReHs1dHEIFM6mOF7uCDBkqBjBStN2s5Id/d9vkjPIYrN95qph9duHu0J/xci0biD\n\t9X8CRCxjHJ66xAl8mV1LoDPDagCXt88zyJ6tZr7Qhz2kTK4QlqudZaiLkupD3t2hTUUD\n\tEodWupzh0TcRHZqqy0MamYupyjes2R2s/Rlm4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=pBkdgvEu28A6yB6OLPVOTAlBdIBNNRiCaiLS2iPOT3Q=;\n\tb=FwgnonD5HMsrPsB/V/yGMi85sv3qnXQQqMi5wgRWZdMUjx4jGmARszKuIoagtHjKgM\n\tGjPZNgSK0pg3GS3n3iLYIGtjx2spcap7/RKqfxaE8NLOmVP14oaqNJEDZ8zPnHrm9TQs\n\t7i1kEjX/7zlZcg4iQ0RW6EgVMkoef0DCi/ZhlYUj7Q61gBdVRedK04dMhTCeNMc75HNf\n\tp55IxjnzZQ54wyHE/vQ2cXjXRqkiGQFY/A9cnKrw7FsuTAvWry0BtFcUHKWtE70uNpK8\n\tpvame9pqgP9FxDJrodd+dPJTQqqCSk60rv9lCi0DYkfk3jfDw3kOex9jetswEITv8m3V\n\txZQQ==","X-Gm-Message-State":"AOAM530SgDtebaLf/AsNIM4YrLDfmJvdFyhQuR5uoTBzVknGSgoq0Y3B\n\tMIfVEIO5RJ0EGSFeZi3H7eiJNHhWmhf9FEGvh4q+dQ==","X-Google-Smtp-Source":"ABdhPJymL9meN/FzPrpDp0ZZUMEui0c5cX1Umn7ll34AtVW3yR50CnFVolh8r0OHyQKXpPeNSRTNnLlV3MODULzqxZk=","X-Received":"by 2002:a05:6402:2690:: with SMTP id\n\tw16mr17009116edd.220.1636513785734; \n\tTue, 09 Nov 2021 19:09:45 -0800 (PST)","MIME-Version":"1.0","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>","In-Reply-To":"<163648987990.1743947.14546877859839703594@Monstersaurus>","From":"Hirokazu Honda ","Date":"Wed, 10 Nov 2021 12:09:34 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20989,"web_url":"https://patchwork.libcamera.org/comment/20989/","msgid":"<163717212002.420308.10876199778410030316@Monstersaurus>","date":"2021-11-17T18:02:00","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-11-10 03:09:34)\n> Hi Kieran,\n> \n> > In fact, it doesn't matter too much - but I still don't see why this\n> > needs to be Extensible.\n> >\n> > The implementation for PlatformFrameBufferAllocator can have an\n> > identically named class in both\n> > src/android/mm/cros_frame_buffer_allocator.cpp\n> > src/android/mm/generic_frame_buffer_allocator.cpp\n> >\n> > And the one that gets used will be the one that is compiled in...?\n> >\n> \n> That's a good point. I couldn't find a reason for this.\n> If this is applicable for PlatformFrameBufferAllocator, I think we can\n> change CameraBuffer too.\n\nSure, if it simplifies things. The class is not a public API, and\ndoesn't need to be 'extensible' with a private implementation as far as\nI am aware. In this case, it's just that there are two possible\nimplementations which are selected at compile time anyway.\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 862D4BF415\n\tfor ;\n\tWed, 17 Nov 2021 18:02:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DDA76037A;\n\tWed, 17 Nov 2021 19:02:04 +0100 (CET)","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 D30F360121\n\tfor ;\n\tWed, 17 Nov 2021 19:02:02 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 687892CF;\n\tWed, 17 Nov 2021 19:02:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G7ca1raN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637172122;\n\tbh=Ms3nhDTUdmgw24nF1xsYSZ6GU4jriIpIKNtuYiOtW5E=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=G7ca1raNWXGFZ0SfmAcMdPvDZrvmrRETJsApTGbUXdEQjjPv6oNimLyXMQj+Lxxen\n\tsEklQOOEY22G0WysM0FJitBRsKNnpgb3dnEmw4YkO7jTjPfHPBDQ81e1PvAO+OXEJn\n\t66ZYVO+Q/Ey6NkzGfZ5x58jGSi41ZISvaySnUmj4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Hirokazu Honda ","Date":"Wed, 17 Nov 2021 18:02:00 +0000","Message-ID":"<163717212002.420308.10876199778410030316@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20991,"web_url":"https://patchwork.libcamera.org/comment/20991/","msgid":"","date":"2021-11-18T08:13:29","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham\n wrote:\n>\n> Quoting Hirokazu Honda (2021-11-10 03:09:34)\n> > Hi Kieran,\n> >\n> > > In fact, it doesn't matter too much - but I still don't see why this\n> > > needs to be Extensible.\n> > >\n> > > The implementation for PlatformFrameBufferAllocator can have an\n> > > identically named class in both\n> > > src/android/mm/cros_frame_buffer_allocator.cpp\n> > > src/android/mm/generic_frame_buffer_allocator.cpp\n> > >\n> > > And the one that gets used will be the one that is compiled in...?\n> > >\n> >\n> > That's a good point. I couldn't find a reason for this.\n> > If this is applicable for PlatformFrameBufferAllocator, I think we can\n> > change CameraBuffer too.\n>\n> Sure, if it simplifies things. The class is not a public API, and\n> doesn't need to be 'extensible' with a private implementation as far as\n> I am aware. In this case, it's just that there are two possible\n> implementations which are selected at compile time anyway.\n>\n\n+Laurent Pinchart , can I implement so?\nWhy is CameraBuffer designed so?\n\n-Hiro\n> --\n> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9BAA3BF415\n\tfor ;\n\tThu, 18 Nov 2021 08:13:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E32C36033C;\n\tThu, 18 Nov 2021 09:13:40 +0100 (CET)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64D2960230\n\tfor ;\n\tThu, 18 Nov 2021 09:13:39 +0100 (CET)","by mail-ed1-x52f.google.com with SMTP id e3so23291976edu.4\n\tfor ;\n\tThu, 18 Nov 2021 00:13:39 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"l+GccUyl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=m2FupPwLuSzKNzeAqkmpaJcXew93HWrVansaHaSBCl4=;\n\tb=l+GccUyldvlEqhzdxgPYq5YW01AYPfu1pc4Nuj3++QHLATPZy8Ul9HnYD0perb7dtM\n\tmQD1RtM2TbjGRcAeIXRU9cgG+5AnesYSRXUvA63o3x6akTjWA0XEUq0w4K+d5G2RX0Au\n\tLtdGen/ybCmQFRuFXqLAdgeiDVylc+KapEkH4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=m2FupPwLuSzKNzeAqkmpaJcXew93HWrVansaHaSBCl4=;\n\tb=aa3XtFAnitjxARtC7UMZ0dtD5jXgBAOg3p7MaQlstcYpztflKMLVUXurlvFTfeFKh0\n\tTmXC63eAjgnboOTixHVjc5Rq8JvbUWEaXJjQGaPUbK4SUnbbrW74BzhdbEbRQh6lRk/e\n\ts7leXrvPpg0xa2VdegYNk5wZNfn3RqQnPAg6/wUlTFGbGX/BxJXCLGw5uqU7khKlpE2l\n\t/HrbFb+JrfwX4XIe9zYDlj6r1rEAhqhtYL8H1RjgGga3WpawQXgKTXcwCY2Bdlzcch7m\n\taGtGAVP+bCIA3hv+mKi6im9ZFnoN/h0ICSmk6fqmfgdcivHXqPwljKt+uingnehK5+Uw\n\tDg5A==","X-Gm-Message-State":"AOAM530QH29L5sZl9qLkGS5zDFK2YCSbUxnzcRN0kXdW1SgtjM/OF4nQ\n\tGW15P5fHmOatyAuDnv/8TyfnwnfVTuzLsUdzFBrQpNPD4FA=","X-Google-Smtp-Source":"ABdhPJytlsl8hRd42SvB9v4Me0VPYA1/kMjVgfGJ0VSZuKxDLShRHcbvF+KEEeolRZMzZVnsOEhSWPQohg8c7pxpB1A=","X-Received":"by 2002:a17:907:1c82:: with SMTP id\n\tnb2mr30225837ejc.218.1637223218932; \n\tThu, 18 Nov 2021 00:13:38 -0800 (PST)","MIME-Version":"1.0","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>\n\t\n\t<163717212002.420308.10876199778410030316@Monstersaurus>","In-Reply-To":"<163717212002.420308.10876199778410030316@Monstersaurus>","From":"Hirokazu Honda ","Date":"Thu, 18 Nov 2021 17:13:29 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20992,"web_url":"https://patchwork.libcamera.org/comment/20992/","msgid":"<20211118084410.f7gd5uxymcbuunf4@uno.localdomain>","date":"2021-11-18T08:44:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote:\n> On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham\n> wrote:\n> >\n> > Quoting Hirokazu Honda (2021-11-10 03:09:34)\n> > > Hi Kieran,\n> > >\n> > > > In fact, it doesn't matter too much - but I still don't see why this\n> > > > needs to be Extensible.\n> > > >\n> > > > The implementation for PlatformFrameBufferAllocator can have an\n> > > > identically named class in both\n> > > > src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > src/android/mm/generic_frame_buffer_allocator.cpp\n> > > >\n> > > > And the one that gets used will be the one that is compiled in...?\n> > > >\n> > >\n> > > That's a good point. I couldn't find a reason for this.\n> > > If this is applicable for PlatformFrameBufferAllocator, I think we can\n> > > change CameraBuffer too.\n> >\n> > Sure, if it simplifies things. The class is not a public API, and\n> > doesn't need to be 'extensible' with a private implementation as far as\n> > I am aware. In this case, it's just that there are two possible\n> > implementations which are selected at compile time anyway.\n> >\n>\n> +Laurent Pinchart , can I implement so?\n> Why is CameraBuffer designed so?\n>\n\nAt the time I designed CameraBuffer using Extensible seemed like a\ngood idea. I had probably been lazy as there might be more opportune\npatterns.\n\nWhat I was looking for was a mechanism that\n\n- Allows to define a standard and unique interface for the\n CameraManager to interface with\n- Had a compile-time selectable implementation\n- Allowed to not pollute the shared interface with details of the\n implementation\n\nI evaluated making use of a more canonical class hierarchy, and I\nconsidered:\n\na) A single .h file that defines the interface, multiple .cpp that\n implement it selected at compile time.\n\n I considered that Extensible would have left more room for\n implementation-specificities in the Private class interface,\n allowing more flexibility and wouldn't have requried\n implementation-specific details to surface in the interface. This\n mostly apply to the implementation-specific data, which Extensible\n allows to easy model in the Private subclass.\n\n Different .h is not an option and keeping them in sync with the\n common user would be painful\n\nb) A canonical class hierarchy where the 'right' sub-class would be\n instantiated at run time, but I ditched that as it would have\n required #ifdefs in the code which I would prefer not to.\n\nThere surely are better solution, as your implementation shows\ndetails and requirement of a specific implementation are surfacing to\nthe interface as well, so this can be surely improved with some more\nopportune design pattern.\n\nThat said, I won't be too much concerned. The pattern is there and is\nin use, simply replacing it with a) won't bring much value, so I would\nsay do whatever is faster for you and if we find a more elegant\nsolution we'll change both BufferAllocator and CameraBuffer in one\ngo.\n\n-\n\n> -Hiro\n> > --\n> > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C085EBDB1C\n\tfor ;\n\tThu, 18 Nov 2021 08:43:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAE0B6033C;\n\tThu, 18 Nov 2021 09:43:16 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3602660230\n\tfor ;\n\tThu, 18 Nov 2021 09:43:16 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 568DC2000A;\n\tThu, 18 Nov 2021 08:43:15 +0000 (UTC)"],"Date":"Thu, 18 Nov 2021 09:44:10 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211118084410.f7gd5uxymcbuunf4@uno.localdomain>","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>\n\t\n\t<163717212002.420308.10876199778410030316@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20993,"web_url":"https://patchwork.libcamera.org/comment/20993/","msgid":"","date":"2021-11-18T08:50:15","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote:\n> > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Hirokazu Honda (2021-11-10 03:09:34)\n> > > > Hi Kieran,\n> > > >\n> > > > > In fact, it doesn't matter too much - but I still don't see why this\n> > > > > needs to be Extensible.\n> > > > >\n> > > > > The implementation for PlatformFrameBufferAllocator can have an\n> > > > > identically named class in both\n> > > > > src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > >\n> > > > > And the one that gets used will be the one that is compiled in...?\n> > > > >\n> > > >\n> > > > That's a good point. I couldn't find a reason for this.\n> > > > If this is applicable for PlatformFrameBufferAllocator, I think we can\n> > > > change CameraBuffer too.\n> > >\n> > > Sure, if it simplifies things. The class is not a public API, and\n> > > doesn't need to be 'extensible' with a private implementation as far as\n> > > I am aware. In this case, it's just that there are two possible\n> > > implementations which are selected at compile time anyway.\n> > >\n> >\n> > +Laurent Pinchart , can I implement so?\n> > Why is CameraBuffer designed so?\n> >\n>\n> At the time I designed CameraBuffer using Extensible seemed like a\n> good idea. I had probably been lazy as there might be more opportune\n> patterns.\n>\n> What I was looking for was a mechanism that\n>\n> - Allows to define a standard and unique interface for the\n> CameraManager to interface with\n> - Had a compile-time selectable implementation\n> - Allowed to not pollute the shared interface with details of the\n> implementation\n>\n> I evaluated making use of a more canonical class hierarchy, and I\n> considered:\n>\n> a) A single .h file that defines the interface, multiple .cpp that\n> implement it selected at compile time.\n>\n> I considered that Extensible would have left more room for\n> implementation-specificities in the Private class interface,\n> allowing more flexibility and wouldn't have requried\n> implementation-specific details to surface in the interface. This\n> mostly apply to the implementation-specific data, which Extensible\n> allows to easy model in the Private subclass.\n>\n> Different .h is not an option and keeping them in sync with the\n> common user would be painful\n>\n> b) A canonical class hierarchy where the 'right' sub-class would be\n> instantiated at run time, but I ditched that as it would have\n> required #ifdefs in the code which I would prefer not to.\n>\n> There surely are better solution, as your implementation shows\n> details and requirement of a specific implementation are surfacing to\n> the interface as well, so this can be surely improved with some more\n> opportune design pattern.\n>\n> That said, I won't be too much concerned. The pattern is there and is\n> in use, simply replacing it with a) won't bring much value, so I would\n> say do whatever is faster for you and if we find a more elegant\n> solution we'll change both BufferAllocator and CameraBuffer in one\n> go.\n>\n\nAh, it is a good point that with Extensible we can have the platform\ndependent class like allocDevice_ in the Extensible class.\nSo I would go along with Extensible.\n\n-Hiro\n> -\n>\n> > -Hiro\n> > > --\n> > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CEA3FBF415\n\tfor ;\n\tThu, 18 Nov 2021 08:50:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DCA36033C;\n\tThu, 18 Nov 2021 09:50:28 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B83960230\n\tfor ;\n\tThu, 18 Nov 2021 09:50:26 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id x6so11999003edr.5\n\tfor ;\n\tThu, 18 Nov 2021 00:50:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"kquHZprv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=U1gx50m0PQ7da4WQZeHlRdzP6dOPwtBb9Mtk8nTl+2g=;\n\tb=kquHZprvRVj7gYVUeo19zqhjZDX8w1lhjs2OVFp75ivl13wI411D/3m+a+hhD6H/0m\n\tRgDAPOL8jyDpw6MNlAmhObq8jXoNOVXckaIqPf9X9rx6CMvw93Rt398wbXUvnVlFmSQq\n\tKidn/Tkn0tiQjlz7oTRIgYRvXAfaLSp17gFXY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=U1gx50m0PQ7da4WQZeHlRdzP6dOPwtBb9Mtk8nTl+2g=;\n\tb=ZsQRZDxYnk3ojhmIKweebyREtDEQXwD8x6i3PbYX0osKQP/uc6469iJfX6r9B0rfyU\n\t9t1OXRTHcOsrzL1HOQuvkFcrU2q1KGn0NAybGGHK7RJ6CvvpJE9ia8K1Bq3GIIzqh+sD\n\t25yfNvpS9AqnWdECjHaFt6TAti/kf6dxJrmdvhjeF/dd+8Db5AC3r3bbTpKK3LOEyCSn\n\tuByrHhtdVuSKnoaSXSSwDQHb6nzRYLnC2/GyhfZgugsNtQeBuqBOhiX/5zdVYbjiSFaM\n\td4G5HG7k/sEa9eqAwI8w6R+qumC+WN7xm4f2TAquuNnbeeaKSJGmGvR1FAZwULbqzTWN\n\tCnmA==","X-Gm-Message-State":"AOAM531sRVhLHdExIxwR1IoxltOsYx9JwOE3ymv2nXnjQPU7oAHy3wlt\n\tM7zg80He7F3p9YIDJJ4ZwdZVaiSRrrI9K3UVJjhXdQ==","X-Google-Smtp-Source":"ABdhPJwYC5C6i9jN2nluUQaZNfm1kYuAFAR4tGItxU3stW7hwzjNUYoVc9YiZ2KhlGh+AxL18yf728kW3t3vlqgkUhQ=","X-Received":"by 2002:a05:6402:2026:: with SMTP id\n\tay6mr8777388edb.202.1637225425535; \n\tThu, 18 Nov 2021 00:50:25 -0800 (PST)","MIME-Version":"1.0","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>\n\t<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>\n\t\n\t<163717212002.420308.10876199778410030316@Monstersaurus>\n\t\n\t<20211118084410.f7gd5uxymcbuunf4@uno.localdomain>","In-Reply-To":"<20211118084410.f7gd5uxymcbuunf4@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 18 Nov 2021 17:50:15 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21086,"web_url":"https://patchwork.libcamera.org/comment/21086/","msgid":"","date":"2021-11-22T01:07:36","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Nov 18, 2021 at 05:50:15PM +0900, Hirokazu Honda wrote:\n> On Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi wrote:\n> > On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote:\n> > > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham wrote:\n> > > > Quoting Hirokazu Honda (2021-11-10 03:09:34)\n> > > > > Hi Kieran,\n> > > > >\n> > > > > > In fact, it doesn't matter too much - but I still don't see why this\n> > > > > > needs to be Extensible.\n> > > > > >\n> > > > > > The implementation for PlatformFrameBufferAllocator can have an\n> > > > > > identically named class in both\n> > > > > > src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > > src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > >\n> > > > > > And the one that gets used will be the one that is compiled in...?\n> > > > >\n> > > > > That's a good point. I couldn't find a reason for this.\n> > > > > If this is applicable for PlatformFrameBufferAllocator, I think we can\n> > > > > change CameraBuffer too.\n> > > >\n> > > > Sure, if it simplifies things. The class is not a public API, and\n> > > > doesn't need to be 'extensible' with a private implementation as far as\n> > > > I am aware. In this case, it's just that there are two possible\n> > > > implementations which are selected at compile time anyway.\n> > >\n> > > +Laurent Pinchart , can I implement so?\n> > > Why is CameraBuffer designed so?\n> >\n> > At the time I designed CameraBuffer using Extensible seemed like a\n> > good idea. I had probably been lazy as there might be more opportune\n> > patterns.\n\nUsage of the Extensible class for CameraBuffer was a bit of a hack I\nthink. In particular, I'm not fond of the\nPUBLIC_CAMERA_BUFFER_IMPLEMENTATION macro. I haven't taken the time to\ntry and find a better solution.\n\n> > What I was looking for was a mechanism that\n> >\n> > - Allows to define a standard and unique interface for the\n> > CameraManager to interface with\n> > - Had a compile-time selectable implementation\n> > - Allowed to not pollute the shared interface with details of the\n> > implementation\n> >\n> > I evaluated making use of a more canonical class hierarchy, and I\n> > considered:\n> >\n> > a) A single .h file that defines the interface, multiple .cpp that\n> > implement it selected at compile time.\n> >\n> > I considered that Extensible would have left more room for\n> > implementation-specificities in the Private class interface,\n> > allowing more flexibility and wouldn't have requried\n> > implementation-specific details to surface in the interface. This\n> > mostly apply to the implementation-specific data, which Extensible\n> > allows to easy model in the Private subclass.\n> >\n> > Different .h is not an option and keeping them in sync with the\n> > common user would be painful\n> >\n> > b) A canonical class hierarchy where the 'right' sub-class would be\n> > instantiated at run time, but I ditched that as it would have\n> > required #ifdefs in the code which I would prefer not to.\n> >\n> > There surely are better solution, as your implementation shows\n> > details and requirement of a specific implementation are surfacing to\n> > the interface as well, so this can be surely improved with some more\n> > opportune design pattern.\n> >\n> > That said, I won't be too much concerned. The pattern is there and is\n> > in use, simply replacing it with a) won't bring much value, so I would\n> > say do whatever is faster for you and if we find a more elegant\n> > solution we'll change both BufferAllocator and CameraBuffer in one\n> > go.\n> \n> Ah, it is a good point that with Extensible we can have the platform\n> dependent class like allocDevice_ in the Extensible class.\n> So I would go along with Extensible.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AB440BDB13\n\tfor ;\n\tMon, 22 Nov 2021 01:08:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBE286036F;\n\tMon, 22 Nov 2021 02:08:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E817960228\n\tfor ;\n\tMon, 22 Nov 2021 02:07:59 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E887A1B;\n\tMon, 22 Nov 2021 02:07:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GKohgIJX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637543279;\n\tbh=gu0gr2h7XGsBgVhqfzHG4ShFhkWBLnkTB+fnDc+EIYo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GKohgIJXIDJC340OIm2tfulcwhYF8xoaZ40SL7k0cWgX+H1+Yhcji2DU7ZgebjGt7\n\t/pZa6ZJS5IEyxeUSwAaTH9i/5RywciyS/bKf6oM8dlLt7ONYJX8HqmRiM1dnlwpAnB\n\tbRhTu00OaO27xWa7N0zxJqR2TX7zb4WHKXrNkL5I=","Date":"Mon, 22 Nov 2021 03:07:36 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<163641399493.948064.7503143066630957175@Monstersaurus>\n\t\n\t<20211109085726.x3xw7leybye4uwa4@uno.localdomain>\n\t<163645285905.1317171.1464624108423094618@Monstersaurus>\n\t<163648987990.1743947.14546877859839703594@Monstersaurus>\n\t\n\t<163717212002.420308.10876199778410030316@Monstersaurus>\n\t\n\t<20211118084410.f7gd5uxymcbuunf4@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21087,"web_url":"https://patchwork.libcamera.org/comment/21087/","msgid":"","date":"2021-11-22T02:01:14","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Nov 01, 2021 at 04:16:52PM +0900, Hirokazu Honda wrote:\n> After we unify identical streams to one stream configuration, the\n> capture requested by a HAL client can have been resolved as\n> CameraStream::Mapped. That is, a buffer to be written by a camera\n> is not provided by a client. We would handle this case by\n> dynamically allocating FrameBuffer.\n\nThis sounds quite confusing.\n\n> The existing FrameBufferAllocator cannot used because it is not\n> allowed to allocate a new buffer while Camera is running.\n> \n> This introduces PlatformFrameBufferAllocator. It allocates\n> FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> the underlying buffer but must be destroyed before\n> PlatformFrameBufferAllocator is destroyed.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/frame_buffer_allocator.h | 54 +++++++\n> .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++\n> .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 4 files changed, 292 insertions(+), 2 deletions(-)\n> create mode 100644 src/android/frame_buffer_allocator.h\n> create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp\n> create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp\n> \n> diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h\n> new file mode 100644\n> index 00000000..41ed4ae1\n> --- /dev/null\n> +++ b/src/android/frame_buffer_allocator.h\n> @@ -0,0 +1,54 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> + * platform dependent way.\n> + */\n> +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +class CameraDevice;\n> +\n> +class PlatformFrameBufferAllocator : libcamera::Extensible\n> +{\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> +public:\n> +\texplicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> +\t~PlatformFrameBufferAllocator();\n> +\n> +\t/*\n> +\t * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> +\t * Note: The returned FrameBuffer needs to be destroyed before\n> +\t * PlatformFrameBufferAllocator is destroyed.\n> +\t */\n> +\tstd::unique_ptr allocate(\n> +\t\tint halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +};\n> +\n> +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\t\t\t\\\n> +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(\t\t\\\n> +\tCameraDevice *const cameraDevice)\t\t\t\t\\\n> +\t: Extensible(std::make_unique(this, cameraDevice))\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +}\t\t\t\t\t\t\t\t\t\\\n> +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +}\t\t\t\t\t\t\t\t\t\\\n> +std::unique_ptr\t\t\t\t\t\\\n> +PlatformFrameBufferAllocator::allocate(int halPixelFormat,\t\t\\\n> +\t\t\t\t const libcamera::Size& size,\t\\\n> +\t\t\t\t uint32_t usage)\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\treturn _d()->allocate(halPixelFormat, size, usage);\t\t\\\n> +}\n> +\n> +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */\n> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> new file mode 100644\n> index 00000000..9c7e4ea4\n> --- /dev/null\n> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> @@ -0,0 +1,90 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using\n> + * CameraBufferManager\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/framebuffer.h\"\n> +\n> +#include \"../camera_device.h\"\n> +#include \"../frame_buffer_allocator.h\"\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n> +class CrosFrameBufferData : public FrameBuffer::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> +public:\n> +\tCrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> +\t\t: FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> +\t{\n> +\t}\n> +\n> +\t~CrosFrameBufferData() override = default;\n\nDo you need this ?\n\n> +\n> +private:\n> +\tcros::ScopedBufferHandle scopedHandle_;\n> +};\n> +} /* namespace */\n> +\n> +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> +\n> +public:\n> +\tPrivate([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> +\t\t[[maybe_unused]] CameraDevice *const cameraDevice)\n> +\t{\n> +\t}\n> +\n> +\t~Private() override = default;\n\nAnd this ?\n\n> +\n> +\tstd::unique_ptr allocate(\n> +\t\tint halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +};\n> +\n> +std::unique_ptr\n> +PlatformFrameBufferAllocator::Private::allocate(\n> +\tint halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> +{\n> +\tcros::ScopedBufferHandle scopedHandle =\n> +\t\tcros::CameraBufferManager::AllocateScopedBuffer(\n> +\t\t\tsize.width, size.height, halPixelFormat, usage);\n> +\tif (!scopedHandle) {\n> +\t\tLOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tbuffer_handle_t handle = *scopedHandle;\n> +\tconst size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> +\tstd::vector planes(numPlanes);\n> +\tFileDescriptor fd{ handle->data[0] };\n> +\tif (!fd.isValid()) {\n> +\t\tLOG(HAL, Fatal) << \"Invalid fd\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\t/* This code assumes all the planes are located in the same buffer. */\n> +\tfor (auto [i, plane] : utils::enumerate(planes)) {\n> +\t\tplane.fd = fd;\n> +\t\tplane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> +\t\tplane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> +\t}\n> +\n> +\treturn std::make_unique(\n> +\t\tstd::make_unique(std::move(scopedHandle)),\n> +\t\tplanes);\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> new file mode 100644\n> index 00000000..167cc1a5\n> --- /dev/null\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -0,0 +1,144 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../camera_device.h\"\n> +#include \"../frame_buffer_allocator.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n> +class GenericFrameBufferData : public FrameBuffer::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> +\n> +public:\n> +\tGenericFrameBufferData(struct alloc_device_t *allocDevice,\n> +\t\t\t buffer_handle_t handle)\n> +\t\t: allocDevice_(allocDevice), handle_(handle)\n> +\t{\n> +\t\tASSERT(allocDevice_);\n> +\t\tASSERT(handle_);\n> +\t}\n> +\n> +\t~GenericFrameBufferData() override\n> +\t{\n> +\t\t/*\n> +\t\t * allocDevice_ is used to destroy handle_. allocDevice_ is\n\nExtra space before allocDevice_.\n\n> +\t\t * owned by PlatformFrameBufferAllocator::Private.\n> +\t\t * GenericFrameBufferData must be destroyed before it is\n> +\t\t * destroyed.\n> +\t\t *\n> +\t\t * \\todo: Consider managing alloc_device_t with std::shared_ptr\n\ns/todo:/todo/\n\n> +\t\t * if this is difficult to maintain.\n> +\t\t *\n> +\t\t * Q: Thread safety against alloc_device_t is not documented.\n\ns/Q:/\\todo/\n\n> +\t\t * Is it no problem to call alloc/free in parallel?\n> +\t\t */\n> +\t\tASSERT(allocDevice_);\n\nYou can drop this, it's tested in the constructor.\n\n> +\t\tallocDevice_->free(allocDevice_, handle_);\n> +\t}\n> +\n> +private:\n> +\tstruct alloc_device_t *allocDevice_;\n> +\tconst buffer_handle_t handle_;\n> +};\n> +} /* namespace */\n> +\n> +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> +\n> +public:\n> +\tPrivate([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> +\t\tCameraDevice *const cameraDevice)\n> +\t\t: cameraDevice_(cameraDevice),\n> +\t\t hardwareModule_(cameraDevice->camera3Device()->common.module),\n> +\t\t allocDevice_(nullptr)\n> +\t{\n> +\t\tASSERT(hardwareModule_);\n> +\t}\n> +\n> +\t~Private() override;\n> +\n> +\tstd::unique_ptr allocate(\n> +\t\tint halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +\n> +private:\n> +\tconst CameraDevice *const cameraDevice_;\n> +\tstruct hw_module_t *const hardwareModule_;\n> +\tstruct alloc_device_t *allocDevice_;\n> +};\n> +\n> +PlatformFrameBufferAllocator::Private::~Private()\n> +{\n> +\tif (allocDevice_)\n> +\t\tgralloc_close(allocDevice_);\n> +}\n> +\n> +std::unique_ptr\n> +PlatformFrameBufferAllocator::Private::allocate(\n> +\tint halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> +{\n> +\tif (!allocDevice_) {\n> +\t\tint ret = gralloc_open(hardwareModule_, &allocDevice_);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Fatal) << \"gralloc_open() failed: \" << ret;\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\t}\n> +\n> +\tint stride = 0;\n> +\tbuffer_handle_t handle = nullptr;\n> +\tint ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> +\t\t\t\t halPixelFormat, usage, &handle, &stride);\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> +\t\treturn nullptr;\n> +\t}\n> +\tif (!handle) {\n> +\t\tLOG(HAL, Fatal) << \"buffer_handle_t is empty on success\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tconst libcamera::PixelFormat pixelFormat =\n> +\t\tcameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> +\tconst auto &info = PixelFormatInfo::info(pixelFormat);\n> +\tconst unsigned int numPlanes = info.numPlanes();\n> +\tstd::vector planes(numPlanes);\n\n\tstd::vector planes(info.numPlanes());\n\nI wonder how all this will interact with a generic way to map\nFrameBuffer objects, in the libcamera core. We'll see when we get there,\nit will be interesting.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\t/* This code assumes the planes are mapped consecutively. */\n> +\tFileDescriptor fd{ handle->data[0] };\n> +\tsize_t offset = 0;\n> +\tfor (auto [i, plane] : utils::enumerate(planes)) {\n> +\t\tconst size_t planeSize = info.planeSize(size.height, i, stride);\n> +\n> +\t\tplanes[i].fd = fd;\n> +\t\tplanes[i].offset = offset;\n> +\t\tplanes[i].length = planeSize;\n> +\t\toffset += planeSize;\n> +\t}\n> +\n> +\treturn std::make_unique(\n> +\t\tstd::make_unique(allocDevice_, handle),\n> +\t\tplanes);\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> index eeb5cc2e..d40a3b0b 100644\n> --- a/src/android/mm/meson.build\n> +++ b/src/android/mm/meson.build\n> @@ -2,8 +2,10 @@\n> \n> platform = get_option('android_platform')\n> if platform == 'generic'\n> - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> + android_hal_sources += files(['generic_camera_buffer.cpp',\n> + 'generic_frame_buffer_allocator.cpp'])\n> elif platform == 'cros'\n> - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> + android_hal_sources += files(['cros_camera_buffer.cpp',\n> + 'cros_frame_buffer_allocator.cpp'])\n> android_deps += [dependency('libcros_camera')]\n> endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A2CA7BF415\n\tfor ;\n\tMon, 22 Nov 2021 02:01:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9D526036F;\n\tMon, 22 Nov 2021 03:01:40 +0100 (CET)","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 E219360228\n\tfor ;\n\tMon, 22 Nov 2021 03:01:38 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47A18A1B;\n\tMon, 22 Nov 2021 03:01:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PNEK2oXf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637546498;\n\tbh=4kTTSJzx/gn3f6k+VAM5U7bHNwvNJB9a7kgdL4+8qPs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PNEK2oXfr21iYkNVlOnz9QpMXn27v0VED9ioisWZLf234mrdwrMFnF5048hUGpxka\n\trQMP6kxV+oi0AdXrk6722PDO+Y1mvzpJ+yMHH/lZoYUCvmn7ViMnydvwuKuLDHbCPf\n\tfXEAFkOjQO0Aw6MvE36Xrlgo1syQZZcur/CkbeWI=","Date":"Mon, 22 Nov 2021 04:01:14 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211101071652.107912-1-hiroh@chromium.org>\n\t<20211101071652.107912-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211101071652.107912-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Introduce\n\tPlatformFrameBufferAllocator","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]