[{"id":21214,"web_url":"https://patchwork.libcamera.org/comment/21214/","msgid":"<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>","date":"2021-11-25T00:10:13","subject":"Re: [libcamera-devel] [PATCH v3 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 Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> The existing FrameBufferAllocator is not allowed to allocate a\n> new buffer while Camera is running. This introduces\n> PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> platform. The allocated FrameBuffer owns the underlying buffer\n> but must be destroyed before PlatformFrameBufferAllocator is\n> destroyed.\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/frame_buffer_allocator.h | 54 +++++++\n> .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 4 files changed, 288 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\ncheckstyle should have suggested you a separate group ?\nIt didn't for me recently but I got told so :)\n\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..d6343e53\n> --- /dev/null\n> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> @@ -0,0 +1,88 @@\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\nBlank line ?\n\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n\nBlank line\n\n> +class CrosFrameBufferData : public FrameBuffer::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n\nBlank line\n\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\nDoes the compiler complains if you don't force the destructor to be\ngenerated ?\n\n> +\n> +private:\n> +\tcros::ScopedBufferHandle scopedHandle_;\n> +};\n\nBlank line\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\nallocator is not used anymore in any of the two subclasses ? Can it be\ndropped ?\n\n> +\t\t[[maybe_unused]] CameraDevice *const cameraDevice)\n> +\t{\n> +\t}\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\nafaict this duplicates the filedescriptor. If the\ncros:CameraBufferManager closes its copy on destruction, it's fine.\n\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\nDO you need to move scopedHandle ? You have a single constructor that\nwill bind to && and & if I'm not mistaken.\n\nWith minors and the file descriptor duplication clarified\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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..f00fd704\n> --- /dev/null\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -0,0 +1,142 @@\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> +\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> +\t\t * if this is difficult to maintain.\n> +\t\t *\n> +\t\t * \\todo Thread safety against alloc_device_t is not documented.\n> +\t\t * Is it no problem to call alloc/free in parallel?\n> +\t\t */\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> +\tstd::vector planes(info.numPlanes());\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\n> --\n> 2.34.0.rc2.393.gf8c9666880-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 99658BF415\n\tfor ;\n\tThu, 25 Nov 2021 00:09:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C61916038A;\n\tThu, 25 Nov 2021 01:09:22 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B76560228\n\tfor ;\n\tThu, 25 Nov 2021 01:09:21 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 16E9A1BF203;\n\tThu, 25 Nov 2021 00:09:20 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 01:10:13 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211123183947.46839-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 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":21262,"web_url":"https://patchwork.libcamera.org/comment/21262/","msgid":"","date":"2021-11-26T03:36:31","subject":"Re: [libcamera-devel] [PATCH v3 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, thank you for reviewing.\n\nOn Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > The existing FrameBufferAllocator is not allowed to allocate a\n> > new buffer while Camera is running. This introduces\n> > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > platform. The allocated FrameBuffer owns the underlying buffer\n> > but must be destroyed before PlatformFrameBufferAllocator is\n> > destroyed.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/android/frame_buffer_allocator.h | 54 +++++++\n> > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > src/android/mm/meson.build | 6 +-\n> > 4 files changed, 288 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>\n> checkstyle should have suggested you a separate group ?\n> It didn't for me recently but I got told so :)\n>\n\nI don't get any error.\n\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +class CameraDevice;\n> > +\n> > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > --- /dev/null\n> > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > @@ -0,0 +1,88 @@\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>\n> Blank line ?\n>\n> > +#include \"cros-camera/camera_buffer_manager.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > +namespace {\n>\n> Blank line\n>\n> > +class CrosFrameBufferData : public FrameBuffer::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n>\n> Blank line\n>\n> > +public:\n> > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > + {\n> > + }\n> > +\n> > + ~CrosFrameBufferData() override = default;\n>\n> Does the compiler complains if you don't force the destructor to be\n> generated ?\n>\n\nNo. Omitted.\n> > +\n> > +private:\n> > + cros::ScopedBufferHandle scopedHandle_;\n> > +};\n>\n> Blank line\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>\n> allocator is not used anymore in any of the two subclasses ? Can it be\n> dropped ?\n>\n\nI think having PlatformAllocator in argument is required for\nPFBAllocator::Private class constructor.\n\n> > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > + {\n> > + }\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>\n> afaict this duplicates the filedescriptor. If the\n> cros:CameraBufferManager closes its copy on destruction, it's fine.\n>\n\nYes, correct.\n\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>\n> DO you need to move scopedHandle ? You have a single constructor that\n> will bind to && and & if I'm not mistaken.\n>\n\nYes, I have to. cros::ScopedBufferHandle is move-only constructor.\nGoogle c++ coding style restricts using && in move constructor only.\nI think since CrosFrameBufferData is different from\ncros::ScopedBufferHandle this is not move constructor.\n\n-Hiro\n\n> With minors and the file descriptor duplication clarified\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\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..f00fd704\n> > --- /dev/null\n> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > @@ -0,0 +1,142 @@\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> > + * \\todo Thread safety against alloc_device_t is not documented.\n> > + * Is it no problem to call alloc/free in parallel?\n> > + */\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> > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 BD41FBF415\n\tfor ;\n\tFri, 26 Nov 2021 03:36:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 083F36049A;\n\tFri, 26 Nov 2021 04:36:45 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C24E6011D\n\tfor ;\n\tFri, 26 Nov 2021 04:36:43 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id l25so32873892eda.11\n\tfor ;\n\tThu, 25 Nov 2021 19:36:43 -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=\"Jrq10V1s\"; 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=JK8Ptsqqac1e9x9C1rc5B/Z6sCFhzB2i4FwHqCh0X4w=;\n\tb=Jrq10V1sLuQq5c0QYw45hNMv5pSIgW9dMkzx32jzkzIiaXrXBhMhxVUDRGBRO80AYx\n\trVq/MXnoeo5Qkq68IvhcGVenRSX800o7qNsxleDJ0Vit+qvRdnDSyYw6xj2sIY6AGqZ5\n\tnUQC9p5AuAyDQRmqHNmgygmc/eLKsPklfdys8=","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=JK8Ptsqqac1e9x9C1rc5B/Z6sCFhzB2i4FwHqCh0X4w=;\n\tb=FUC/oGd1MzAXWPT1VydAGlPrYON+btxOWHA1l5dwOBvM69HdP+zTw2ukTJZHOTLsKB\n\th6JUYFuaPtbySwvpFsLORZJ0MObtH3gFkdy/DSkDRJ/YvfVh0SJeG2jmVvUt0yYfe7G9\n\tH1nDtb/d5dCC+v8Fff7oWQ3HorM4ylTttPVIkS+fOYBZ63ow/uEzayFpxcxCCKId7TLX\n\tjD+FdxsDCMRae2NQkb8oBKzZtNig5arR0tjA2PxhVfmmZVTCrU1NNc9cSQFtsC9qYv5k\n\tXzndPrEWdK3Ko4frLUc3t0kAKxFxsmpBwx5OB5xM9IbtpPoPvFfSHKJ96e0C6OMYA4hE\n\tnrDQ==","X-Gm-Message-State":"AOAM533tpVcHrMn1KF54bgVzfqALpfgBWVfOtsb6mkVG+byLT/4wRQGd\n\tRAXaHW7gbAoPizGTjBgHSEhUGBBxziRgHthMWssvQzFuHeJFUw==","X-Google-Smtp-Source":"ABdhPJzgpkZdwpY9p++EuAK9ygGfGKM8mAtidNdRwRXCdp12TGkCYAYsJLply9A0To0VKftooGzj48Rd2bHzdQK/aio=","X-Received":"by 2002:a17:907:6ea1:: with SMTP id\n\tsh33mr36635524ejc.150.1637897802870; \n\tThu, 25 Nov 2021 19:36:42 -0800 (PST)","MIME-Version":"1.0","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>","In-Reply-To":"<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 26 Nov 2021 12:36:31 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":21263,"web_url":"https://patchwork.libcamera.org/comment/21263/","msgid":"","date":"2021-11-26T03:37:25","subject":"Re: [libcamera-devel] [PATCH v3 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 Fri, Nov 26, 2021 at 12:36 PM Hirokazu Honda wrote:\n>\n> Hi Jacopo, thank you for reviewing.\n>\n> On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > > The existing FrameBufferAllocator is not allowed to allocate a\n> > > new buffer while Camera is running. This introduces\n> > > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > > platform. The allocated FrameBuffer owns the underlying buffer\n> > > but must be destroyed before PlatformFrameBufferAllocator is\n> > > destroyed.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 4 files changed, 288 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> >\n> > checkstyle should have suggested you a separate group ?\n> > It didn't for me recently but I got told so :)\n> >\n>\n> I don't get any error.\n>\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > > --- /dev/null\n> > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,88 @@\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> >\n> > Blank line ?\n\nOh, checkstyle warns if I add blank line here.\n> >\n> > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\n> > > +\n> > > +namespace {\n> >\n> > Blank line\n> >\n> > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> >\n> > Blank line\n> >\n> > > +public:\n> > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > + {\n> > > + }\n> > > +\n> > > + ~CrosFrameBufferData() override = default;\n> >\n> > Does the compiler complains if you don't force the destructor to be\n> > generated ?\n> >\n>\n> No. Omitted.\n> > > +\n> > > +private:\n> > > + cros::ScopedBufferHandle scopedHandle_;\n> > > +};\n> >\n> > Blank line\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> >\n> > allocator is not used anymore in any of the two subclasses ? Can it be\n> > dropped ?\n> >\n>\n> I think having PlatformAllocator in argument is required for\n> PFBAllocator::Private class constructor.\n>\n> > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > + {\n> > > + }\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> >\n> > afaict this duplicates the filedescriptor. If the\n> > cros:CameraBufferManager closes its copy on destruction, it's fine.\n> >\n>\n> Yes, correct.\n>\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> >\n> > DO you need to move scopedHandle ? You have a single constructor that\n> > will bind to && and & if I'm not mistaken.\n> >\n>\n> Yes, I have to. cros::ScopedBufferHandle is move-only constructor.\n> Google c++ coding style restricts using && in move constructor only.\n> I think since CrosFrameBufferData is different from\n> cros::ScopedBufferHandle this is not move constructor.\n>\n> -Hiro\n>\n> > With minors and the file descriptor duplication clarified\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\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..f00fd704\n> > > --- /dev/null\n> > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,142 @@\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> > > + * \\todo Thread safety against alloc_device_t is not documented.\n> > > + * Is it no problem to call alloc/free in parallel?\n> > > + */\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> > > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 CDAABBF415\n\tfor ;\n\tFri, 26 Nov 2021 03:37:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 833FE6049F;\n\tFri, 26 Nov 2021 04:37:38 +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 847786011D\n\tfor ;\n\tFri, 26 Nov 2021 04:37:36 +0100 (CET)","by mail-ed1-x531.google.com with SMTP id r25so33068493edq.7\n\tfor ;\n\tThu, 25 Nov 2021 19:37:36 -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=\"FI6DOozg\"; 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=nfJvxtcTziETGZ8UvIz1/OR/qfPpjL+VGZrcAVFCnaY=;\n\tb=FI6DOozgYF1CeZxyU4/mBupHzIPaW0A43K3PFr/f3jDo46U6ZB2OhyNxzKnkxR/Xis\n\t57YTyf5Eg9UsaLTMWbRdF9PVFdj+BYciUbdqZXEDWquygrGhKE52s/pzD78JrLZpsl06\n\tNbn0HuaYSiuopoShhlsqqOCGxg/EB4NwF95QU=","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=nfJvxtcTziETGZ8UvIz1/OR/qfPpjL+VGZrcAVFCnaY=;\n\tb=e4Xav2nAcmktbvBkuOG+mzWi/cg+OsvSS7vUkW8QxNYGtjiowBgEtk7lUzYPLAh3tW\n\tDu31Jr4BOJryHa9CP585DAevs7AabglQKiU0ddUvcUtBjtLm2qYo7yMMbD8vtM9u0Ues\n\tk17eKoC+PBG0jDmDJ3T7GQX63pQCu1VzPJwuDMKjhNVrhiyMldhlLGIIX8Oe4/MBvCnO\n\t0PZ8yYVne62A3APFQm6NuzIpS2znuVQu8wE8wa3SAE6bfkLAHhZSiM3WVHAeQGXyPdOk\n\teud/86LuH6CCtXk/zDjaCHrYon4TFJYTOdYmy3ghYgOOkE+lUKUkhmbxzk3fJCNLPe9L\n\tCijA==","X-Gm-Message-State":"AOAM530TGt1pSFt2TGPKK1I04L0X8ftjpT/hR2h4kgTQg9+/hmzJr9Yf\n\t0ZfcpOGVchwm9ZndSzvANBMYHsae/mxgAB7Tbj5U5kB6h3k=","X-Google-Smtp-Source":"ABdhPJxyWOLrvTGe3a8K5+fAbaO5Ckp0vA08/yZXu8659Ta+NrgCobRRBPlhII/o6wWRD62OWnlK01Bie4GG7I8crKw=","X-Received":"by 2002:a17:907:1c82:: with SMTP id\n\tnb2mr35828825ejc.218.1637897856115; \n\tThu, 25 Nov 2021 19:37:36 -0800 (PST)","MIME-Version":"1.0","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 26 Nov 2021 12:37:25 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":21275,"web_url":"https://patchwork.libcamera.org/comment/21275/","msgid":"<20211126092039.j2nftfbxgf37yjk7@uno.localdomain>","date":"2021-11-26T09:20:39","subject":"Re: [libcamera-devel] [PATCH v3 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 Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n>\n> On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > > The existing FrameBufferAllocator is not allowed to allocate a\n> > > new buffer while Camera is running. This introduces\n> > > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > > platform. The allocated FrameBuffer owns the underlying buffer\n> > > but must be destroyed before PlatformFrameBufferAllocator is\n> > > destroyed.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 4 files changed, 288 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> >\n> > checkstyle should have suggested you a separate group ?\n> > It didn't for me recently but I got told so :)\n> >\n>\n> I don't get any error.\n>\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > > --- /dev/null\n> > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,88 @@\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> >\n> > Blank line ?\n> >\n> > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\n> > > +\n> > > +namespace {\n> >\n> > Blank line\n> >\n> > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> >\n> > Blank line\n> >\n> > > +public:\n> > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > + {\n> > > + }\n> > > +\n> > > + ~CrosFrameBufferData() override = default;\n> >\n> > Does the compiler complains if you don't force the destructor to be\n> > generated ?\n> >\n>\n> No. Omitted.\n> > > +\n> > > +private:\n> > > + cros::ScopedBufferHandle scopedHandle_;\n> > > +};\n> >\n> > Blank line\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> >\n> > allocator is not used anymore in any of the two subclasses ? Can it be\n> > dropped ?\n> >\n>\n> I think having PlatformAllocator in argument is required for\n> PFBAllocator::Private class constructor.\n>\n> > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > + {\n> > > + }\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> >\n> > afaict this duplicates the filedescriptor. If the\n> > cros:CameraBufferManager closes its copy on destruction, it's fine.\n> >\n>\n> Yes, correct.\n>\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> >\n> > DO you need to move scopedHandle ? You have a single constructor that\n> > will bind to && and & if I'm not mistaken.\n> >\n>\n> Yes, I have to. cros::ScopedBufferHandle is move-only constructor.\n> Google c++ coding style restricts using && in move constructor only.\n> I think since CrosFrameBufferData is different from\n> cros::ScopedBufferHandle this is not move constructor.\n\nOk but why constructing a new ScopedBufferHandle in the\nCrosFrameBufferData constructor ?\n\nYou currently have\n\n\tCrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n\t\t: FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n\t{\n\t}\n\n return std::make_unique(\n std::make_unique(std::move(scopedHandle)),\n planes);\n\nWhile you could\n\n\tCrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle)\n\t\t: FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n\t{\n\t}\n\n return std::make_unique(\n std::make_unique(scopedHandle), planes);\n\nWith your ack I can apply these changes.\n\nAlso, I wanted to have the allocator being used to replace\nFrameBufferAllocator in CameraStream before merging so it can be CTS\ntested. I'll do so and send a new version if you're not working on\ndoing so.\n\nThanks\n j\n\n>\n> -Hiro\n>\n> > With minors and the file descriptor duplication clarified\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\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..f00fd704\n> > > --- /dev/null\n> > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > @@ -0,0 +1,142 @@\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> > > + * \\todo Thread safety against alloc_device_t is not documented.\n> > > + * Is it no problem to call alloc/free in parallel?\n> > > + */\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> > > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 811F6BF415\n\tfor ;\n\tFri, 26 Nov 2021 09:19:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1D2F6049A;\n\tFri, 26 Nov 2021 10:19:50 +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 9A8FB60227\n\tfor ;\n\tFri, 26 Nov 2021 10:19:48 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 0C8292000C;\n\tFri, 26 Nov 2021 09:19:47 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 10:20:39 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211126092039.j2nftfbxgf37yjk7@uno.localdomain>","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@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 v3 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":21277,"web_url":"https://patchwork.libcamera.org/comment/21277/","msgid":"","date":"2021-11-26T09:35:43","subject":"Re: [libcamera-devel] [PATCH v3 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 Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi wrote:\n>\n> Hi Hiro\n>\n> On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for reviewing.\n> >\n> > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > > > The existing FrameBufferAllocator is not allowed to allocate a\n> > > > new buffer while Camera is running. This introduces\n> > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > > > platform. The allocated FrameBuffer owns the underlying buffer\n> > > > but must be destroyed before PlatformFrameBufferAllocator is\n> > > > destroyed.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Laurent Pinchart \n> > > > ---\n> > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > > > src/android/mm/meson.build | 6 +-\n> > > > 4 files changed, 288 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> > >\n> > > checkstyle should have suggested you a separate group ?\n> > > It didn't for me recently but I got told so :)\n> > >\n> >\n> > I don't get any error.\n> >\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +class CameraDevice;\n> > > > +\n> > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > @@ -0,0 +1,88 @@\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> > >\n> > > Blank line ?\n> > >\n> > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > +\n> > > > +namespace {\n> > >\n> > > Blank line\n> > >\n> > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > >\n> > > Blank line\n> > >\n> > > > +public:\n> > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + ~CrosFrameBufferData() override = default;\n> > >\n> > > Does the compiler complains if you don't force the destructor to be\n> > > generated ?\n> > >\n> >\n> > No. Omitted.\n> > > > +\n> > > > +private:\n> > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > +};\n> > >\n> > > Blank line\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> > >\n> > > allocator is not used anymore in any of the two subclasses ? Can it be\n> > > dropped ?\n> > >\n> >\n> > I think having PlatformAllocator in argument is required for\n> > PFBAllocator::Private class constructor.\n> >\n> > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > + {\n> > > > + }\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> > >\n> > > afaict this duplicates the filedescriptor. If the\n> > > cros:CameraBufferManager closes its copy on destruction, it's fine.\n> > >\n> >\n> > Yes, correct.\n> >\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> > >\n> > > DO you need to move scopedHandle ? You have a single constructor that\n> > > will bind to && and & if I'm not mistaken.\n> > >\n> >\n> > Yes, I have to. cros::ScopedBufferHandle is move-only constructor.\n> > Google c++ coding style restricts using && in move constructor only.\n> > I think since CrosFrameBufferData is different from\n> > cros::ScopedBufferHandle this is not move constructor.\n>\n> Ok but why constructing a new ScopedBufferHandle in the\n> CrosFrameBufferData constructor ?\n>\n> You currently have\n>\n> CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> {\n> }\n>\n> return std::make_unique(\n> std::make_unique(std::move(scopedHandle)),\n> planes);\n>\n> While you could\n>\n> CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle)\n> : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> {\n> }\n>\n> return std::make_unique(\n> std::make_unique(scopedHandle), planes);\n>\n\nHm, I think it is invalid or at least not recommended way to execute\nstd::move() against reference value.\nIf you would like to do that, I would do\nCrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle).\nYes, it doesn't follow Google C++ style guide, but I am fine in this\ntiny piece of code.\n\n> With your ack I can apply these changes.\n>\n> Also, I wanted to have the allocator being used to replace\n> FrameBufferAllocator in CameraStream before merging so it can be CTS\n> tested. I'll do so and send a new version if you're not working on\n> doing so.\n\nI confirmed replacing it with this allocator works in a few CTS cases.\nHowever, there is one small issue. I will send you my local patch.\n\n-Hiro\n>\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> >\n> > > With minors and the file descriptor duplication clarified\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > Thanks\n> > > j\n> > >\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..f00fd704\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > @@ -0,0 +1,142 @@\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> > > > + * \\todo Thread safety against alloc_device_t is not documented.\n> > > > + * Is it no problem to call alloc/free in parallel?\n> > > > + */\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> > > > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 4CC48BDB13\n\tfor ;\n\tFri, 26 Nov 2021 09:35:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEC866049A;\n\tFri, 26 Nov 2021 10:35:55 +0100 (CET)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2552660227\n\tfor ;\n\tFri, 26 Nov 2021 10:35:54 +0100 (CET)","by mail-ed1-x52a.google.com with SMTP id g14so36269407edb.8\n\tfor ;\n\tFri, 26 Nov 2021 01:35:54 -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=\"FIbjmZ0i\"; 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=uB1zth7mxb3fDqroflNhdwhnjIWfW4SLLPIXRimcO9A=;\n\tb=FIbjmZ0iOvyw3zhpcyEVWDTfYrorTUiCDvpYYzRzDc0IDeEmI3gXtfNEzo8WZVAKh2\n\tJZ/C+rTyjfsoPQNyLTQvTrjIp0R8W/KjkrlIr2xbzmKHUsPxtdw9Uuwqz7hTycGLid3y\n\t6k2OWtG2WRnex1iAjBSuLUcSECwp7D3M2xDF4=","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=uB1zth7mxb3fDqroflNhdwhnjIWfW4SLLPIXRimcO9A=;\n\tb=L1mPtfwbhGvE21DC0Z5p9HgZvRwc+NP6qmmhkggKP7qZCAWLqX1Ic/dndyTFQdi8XJ\n\tCj15hqv/Ub5ofU4GfAgPTbbuqq3p23irbQjqpWo6jiNMFxKAvpL09Iio+ewdFe+0d2lr\n\tfUmCn7vgTGwuvWqEQzUZMgRyTJd/OifRq/2TYjsb3Nolp980MOXhDsiHZmvGgBvnDQNR\n\tpKl6Xzt4OTd51zcvCAOLLtIl/4LEBQpNj5AA9iQzbNjwTPu/Dl+sRBdp64m2lYtweyX+\n\t2ZehiOFoxNahve997XzhNmXOlMDCW8uK5bYDgLi9UomOjxr5ETquYecT9JjUpH/uudFK\n\tdPIQ==","X-Gm-Message-State":"AOAM532duesTOrva/XZY/HPfAlDF03t8XGJwoqNux4Q33qnLhGanyzGS\n\tym5CvJpN+oTCO9xW4NL/AkNcioleONnL7AeKnDm3xRhWMYOFAQ==","X-Google-Smtp-Source":"ABdhPJyvglXAcscsQP5AC+srIowYA5cX6e48YwSbXV4/eHTC9E6/YLB/TaLMOMzYWl1FkdQymZumXvIQvmfnPaRZnhc=","X-Received":"by 2002:a17:907:1c82:: with SMTP id\n\tnb2mr36987573ejc.218.1637919353580; \n\tFri, 26 Nov 2021 01:35:53 -0800 (PST)","MIME-Version":"1.0","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>\n\t\n\t<20211126092039.j2nftfbxgf37yjk7@uno.localdomain>","In-Reply-To":"<20211126092039.j2nftfbxgf37yjk7@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 26 Nov 2021 18:35:43 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":21278,"web_url":"https://patchwork.libcamera.org/comment/21278/","msgid":"<20211126095224.g6bbjazcs7nu74ld@uno.localdomain>","date":"2021-11-26T09:52:24","subject":"Re: [libcamera-devel] [PATCH v3 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 Fri, Nov 26, 2021 at 06:35:43PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro\n> >\n> > On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote:\n> > > Hi Jacopo, thank you for reviewing.\n> > >\n> > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n> > > >\n> > > > Hi Hiro,\n> > > >\n> > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > > > > The existing FrameBufferAllocator is not allowed to allocate a\n> > > > > new buffer while Camera is running. This introduces\n> > > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > > > > platform. The allocated FrameBuffer owns the underlying buffer\n> > > > > but must be destroyed before PlatformFrameBufferAllocator is\n> > > > > destroyed.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > > > > src/android/mm/meson.build | 6 +-\n> > > > > 4 files changed, 288 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> > > >\n> > > > checkstyle should have suggested you a separate group ?\n> > > > It didn't for me recently but I got told so :)\n> > > >\n> > >\n> > > I don't get any error.\n> > >\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +class CameraDevice;\n> > > > > +\n> > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > @@ -0,0 +1,88 @@\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> > > >\n> > > > Blank line ?\n> > > >\n> > > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > +\n> > > > > +namespace {\n> > > >\n> > > > Blank line\n> > > >\n> > > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > > +{\n> > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > >\n> > > > Blank line\n> > > >\n> > > > > +public:\n> > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > > + {\n> > > > > + }\n> > > > > +\n> > > > > + ~CrosFrameBufferData() override = default;\n> > > >\n> > > > Does the compiler complains if you don't force the destructor to be\n> > > > generated ?\n> > > >\n> > >\n> > > No. Omitted.\n> > > > > +\n> > > > > +private:\n> > > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > > +};\n> > > >\n> > > > Blank line\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> > > >\n> > > > allocator is not used anymore in any of the two subclasses ? Can it be\n> > > > dropped ?\n> > > >\n> > >\n> > > I think having PlatformAllocator in argument is required for\n> > > PFBAllocator::Private class constructor.\n> > >\n> > > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > > + {\n> > > > > + }\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> > > >\n> > > > afaict this duplicates the filedescriptor. If the\n> > > > cros:CameraBufferManager closes its copy on destruction, it's fine.\n> > > >\n> > >\n> > > Yes, correct.\n> > >\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> > > >\n> > > > DO you need to move scopedHandle ? You have a single constructor that\n> > > > will bind to && and & if I'm not mistaken.\n> > > >\n> > >\n> > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor.\n> > > Google c++ coding style restricts using && in move constructor only.\n> > > I think since CrosFrameBufferData is different from\n> > > cros::ScopedBufferHandle this is not move constructor.\n> >\n> > Ok but why constructing a new ScopedBufferHandle in the\n> > CrosFrameBufferData constructor ?\n> >\n> > You currently have\n> >\n> > CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > {\n> > }\n> >\n> > return std::make_unique(\n> > std::make_unique(std::move(scopedHandle)),\n> > planes);\n> >\n> > While you could\n> >\n> > CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle)\n> > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > {\n> > }\n> >\n> > return std::make_unique(\n> > std::make_unique(scopedHandle), planes);\n> >\n>\n> Hm, I think it is invalid or at least not recommended way to execute\n> std::move() against reference value.\n\nUh, I didn't know!\nBut yeah, it hides from the caller that the parameter is stolen\n\nIs this reported in the C++ style guide ?\n\n> If you would like to do that, I would do\n> CrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle).\n> Yes, it doesn't follow Google C++ style guide, but I am fine in this\n> tiny piece of code.\n\na && in the CrosFrameBufferData constructor would be better\nThen you can keep the move in the caller\n\n>\n> > With your ack I can apply these changes.\n> >\n> > Also, I wanted to have the allocator being used to replace\n> > FrameBufferAllocator in CameraStream before merging so it can be CTS\n> > tested. I'll do so and send a new version if you're not working on\n> > doing so.\n>\n> I confirmed replacing it with this allocator works in a few CTS cases.\n> However, there is one small issue. I will send you my local patch.\n>\n\nOh thanks, I didn't want to steal work from you :)\nIf you're working on it please go ahead, I'll wait for it to be done\nbefore merging!\n\nThanks\n j\n\n> -Hiro\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > -Hiro\n> > >\n> > > > With minors and the file descriptor duplication clarified\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > Thanks\n> > > > j\n> > > >\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..f00fd704\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > @@ -0,0 +1,142 @@\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> > > > > + * \\todo Thread safety against alloc_device_t is not documented.\n> > > > > + * Is it no problem to call alloc/free in parallel?\n> > > > > + */\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> > > > > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 B0486BF415\n\tfor ;\n\tFri, 26 Nov 2021 09:51:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B83796049F;\n\tFri, 26 Nov 2021 10:51:34 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0345460227\n\tfor ;\n\tFri, 26 Nov 2021 10:51:32 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 6F284240010;\n\tFri, 26 Nov 2021 09:51:32 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 10:52:24 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211126095224.g6bbjazcs7nu74ld@uno.localdomain>","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>\n\t\n\t<20211126092039.j2nftfbxgf37yjk7@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 v3 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":21279,"web_url":"https://patchwork.libcamera.org/comment/21279/","msgid":"","date":"2021-11-26T09:54:21","subject":"Re: [libcamera-devel] [PATCH v3 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 Fri, Nov 26, 2021 at 6:51 PM Jacopo Mondi wrote:\n>\n> Hi Hiro\n>\n> On Fri, Nov 26, 2021 at 06:35:43PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo,\n> >\n> > On Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro\n> > >\n> > > On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote:\n> > > > Hi Jacopo, thank you for reviewing.\n> > > >\n> > > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Hiro,\n> > > > >\n> > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote:\n> > > > > > The existing FrameBufferAllocator is not allowed to allocate a\n> > > > > > new buffer while Camera is running. This introduces\n> > > > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using\n> > > > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS\n> > > > > > platform. The allocated FrameBuffer owns the underlying buffer\n> > > > > > but must be destroyed before PlatformFrameBufferAllocator is\n> > > > > > destroyed.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > ---\n> > > > > > src/android/frame_buffer_allocator.h | 54 +++++++\n> > > > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > > > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++\n> > > > > > src/android/mm/meson.build | 6 +-\n> > > > > > 4 files changed, 288 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> > > > >\n> > > > > checkstyle should have suggested you a separate group ?\n> > > > > It didn't for me recently but I got told so :)\n> > > > >\n> > > >\n> > > > I don't get any error.\n> > > >\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +class CameraDevice;\n> > > > > > +\n> > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible\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..d6343e53\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> > > > > > @@ -0,0 +1,88 @@\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> > > > >\n> > > > > Blank line ?\n> > > > >\n> > > > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > > > +\n> > > > > > +using namespace libcamera;\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > > > +\n> > > > > > +namespace {\n> > > > >\n> > > > > Blank line\n> > > > >\n> > > > > > +class CrosFrameBufferData : public FrameBuffer::Private\n> > > > > > +{\n> > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > > >\n> > > > > Blank line\n> > > > >\n> > > > > > +public:\n> > > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > > > > + {\n> > > > > > + }\n> > > > > > +\n> > > > > > + ~CrosFrameBufferData() override = default;\n> > > > >\n> > > > > Does the compiler complains if you don't force the destructor to be\n> > > > > generated ?\n> > > > >\n> > > >\n> > > > No. Omitted.\n> > > > > > +\n> > > > > > +private:\n> > > > > > + cros::ScopedBufferHandle scopedHandle_;\n> > > > > > +};\n> > > > >\n> > > > > Blank line\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> > > > >\n> > > > > allocator is not used anymore in any of the two subclasses ? Can it be\n> > > > > dropped ?\n> > > > >\n> > > >\n> > > > I think having PlatformAllocator in argument is required for\n> > > > PFBAllocator::Private class constructor.\n> > > >\n> > > > > > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > > > > > + {\n> > > > > > + }\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> > > > >\n> > > > > afaict this duplicates the filedescriptor. If the\n> > > > > cros:CameraBufferManager closes its copy on destruction, it's fine.\n> > > > >\n> > > >\n> > > > Yes, correct.\n> > > >\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> > > > >\n> > > > > DO you need to move scopedHandle ? You have a single constructor that\n> > > > > will bind to && and & if I'm not mistaken.\n> > > > >\n> > > >\n> > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor.\n> > > > Google c++ coding style restricts using && in move constructor only.\n> > > > I think since CrosFrameBufferData is different from\n> > > > cros::ScopedBufferHandle this is not move constructor.\n> > >\n> > > Ok but why constructing a new ScopedBufferHandle in the\n> > > CrosFrameBufferData constructor ?\n> > >\n> > > You currently have\n> > >\n> > > CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > {\n> > > }\n> > >\n> > > return std::make_unique(\n> > > std::make_unique(std::move(scopedHandle)),\n> > > planes);\n> > >\n> > > While you could\n> > >\n> > > CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle)\n> > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> > > {\n> > > }\n> > >\n> > > return std::make_unique(\n> > > std::make_unique(scopedHandle), planes);\n> > >\n> >\n> > Hm, I think it is invalid or at least not recommended way to execute\n> > std::move() against reference value.\n>\n> Uh, I didn't know!\n> But yeah, it hides from the caller that the parameter is stolen\n>\n> Is this reported in the C++ style guide ?\n>\n> > If you would like to do that, I would do\n> > CrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle).\n> > Yes, it doesn't follow Google C++ style guide, but I am fine in this\n> > tiny piece of code.\n>\n> a && in the CrosFrameBufferData constructor would be better\n> Then you can keep the move in the caller\n>\n> >\n> > > With your ack I can apply these changes.\n> > >\n> > > Also, I wanted to have the allocator being used to replace\n> > > FrameBufferAllocator in CameraStream before merging so it can be CTS\n> > > tested. I'll do so and send a new version if you're not working on\n> > > doing so.\n> >\n> > I confirmed replacing it with this allocator works in a few CTS cases.\n> > However, there is one small issue. I will send you my local patch.\n> >\n>\n> Oh thanks, I didn't want to steal work from you :)\n> If you're working on it please go ahead, I'll wait for it to be done\n> before merging!\n>\n\nI just sent my local patch to you. Please feel free to steal if you like. :-)\n\n-Hiro\n> Thanks\n> j\n>\n> > -Hiro\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > -Hiro\n> > > >\n> > > > > With minors and the file descriptor duplication clarified\n> > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\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..f00fd704\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > > > > > @@ -0,0 +1,142 @@\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> > > > > > + * \\todo Thread safety against alloc_device_t is not documented.\n> > > > > > + * Is it no problem to call alloc/free in parallel?\n> > > > > > + */\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> > > > > > + std::vector planes(info.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.34.0.rc2.393.gf8c9666880-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 2DF10BF415\n\tfor ;\n\tFri, 26 Nov 2021 09:54:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A64D6049A;\n\tFri, 26 Nov 2021 10:54:34 +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 EA42060227\n\tfor ;\n\tFri, 26 Nov 2021 10:54:32 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id e3so36525312edu.4\n\tfor ;\n\tFri, 26 Nov 2021 01:54:32 -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=\"Guxc3Mb8\"; 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=WTkTXngJ+276pAbjljic82FXNqu7wU00fgez6JYFlh4=;\n\tb=Guxc3Mb82pskDc2apSG3FT8kDID/azwuCW4SLeEh01Rtd4cndc57Bo/xqu/r4HuXNR\n\tpP9iHvmrH963CbSUzWuCRGkg3fi9K/sT0R7YtO4DY/nZNYxtIjvXGEh8O1SVxmHiLqt6\n\txxx+aKG2ba5k+5NqcgkrAvROQT4CoVAgsHJ9U=","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=WTkTXngJ+276pAbjljic82FXNqu7wU00fgez6JYFlh4=;\n\tb=LSzSRJsUH+b/FZGuYLHgRDSid5oYgPoK21lzpz5gEPP6Lo3O/mkAk3Eh/G/UkauPyE\n\tQ2KgQAxIQxMDU3lRy1tr+pc+EGwedMXvaOWA2DkUhOj8PipEI5/2w3/sVt23G8AU5BrO\n\taF0lNF1GXeLV8a5cII9M2BWB4l/skIeS70e3+xVNKJDYKu4ZplhVUseCYkqURk/m4cFN\n\tzf5DQHaRT7JWPlCkJ5azc5vlo+0r0gPx07B9LQvQvXGGTAmrVd9Fri5r359doWd8GswE\n\tQ0yAmFHzTseGU5/OllJG3gtL2+2Hi5FVSL50otiQmuccv2a+PKAQByof8o72uz3mZ4Ll\n\tPrOg==","X-Gm-Message-State":"AOAM5330QLHLRTBVmE+3jG+hFEqIycOW5SM/IGMwHiRc3NFvK4wNhbaU\n\toEnDXHNLtp3zCVkQUMIE+rNwUAfNTwAPFliWS25c9mDLrKY=","X-Google-Smtp-Source":"ABdhPJwRqQ4G048CgHyPaHb1R8mwhHFDc7mG5w6NMJZaoAjgzfFEhTKHong6u1syL00t1/bEZ7h6IXk4vaP3C44qm3k=","X-Received":"by 2002:a05:6402:b23:: with SMTP id\n\tbo3mr45931031edb.366.1637920472469; \n\tFri, 26 Nov 2021 01:54:32 -0800 (PST)","MIME-Version":"1.0","References":"<20211123183947.46839-1-hiroh@chromium.org>\n\t<20211123183947.46839-2-hiroh@chromium.org>\n\t<20211125001013.5g6bi7spw3kcu53j@uno.localdomain>\n\t\n\t<20211126092039.j2nftfbxgf37yjk7@uno.localdomain>\n\t\n\t<20211126095224.g6bbjazcs7nu74ld@uno.localdomain>","In-Reply-To":"<20211126095224.g6bbjazcs7nu74ld@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 26 Nov 2021 18:54:21 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]