[{"id":20641,"web_url":"https://patchwork.libcamera.org/comment/20641/","msgid":"<20211029150933.qia7ti3vgicl2uoj@uno.localdomain>","date":"2021-10-29T15:09:33","subject":"Re: [libcamera-devel] [PATCH 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":"On Thu, Oct 28, 2021 at 04:30:38PM +0900, Hirokazu Honda wrote:\n> After we unify identical streams to one stream configuration, the\n> capture requested by a HAL client can have been resolved as\n> CameraStream::Mapped. That is, a buffer to be written by a camera\n> is not provided by a client. We would handle this case by\n> dynamically allocating FrameBuffer.\n>\n> The existing FrameBufferAllocator cannot used because it is not\n> allowed to allocate a new buffer while Camera is running.\n\nThat's a nice context but seems slightly unrelated to this patch :)\n>\n> This CL introduces PlatformFrameBufferAllocator. It allocates\n\nAfter years, I still don't know what CL means\n\n> FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> the underlying buffer but must be destroyed before\n> PlatformFrameBufferAllocator is destroyed.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/frame_buffer_allocator.h | 55 +++++++\n> .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> .../mm/generic_frame_buffer_allocator.cpp | 140 ++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 4 files changed, 287 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..e46a43b5\n> --- /dev/null\n> +++ b/src/android/frame_buffer_allocator.h\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> + * platform dependent way.\n> + */\n> +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +class CameraDevice;\n> +\n> +class PlatformFrameBufferAllocator : libcamera::Extensible\n> +{\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> +public:\n> +\texplicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);\n> +\t~PlatformFrameBufferAllocator();\n> +\n> +\t/*\n> +\t * FrameBuffer owns the underlying buffer. Returns nullptr on failure.\n> +\t * Note: The returned FrameBuffer needs to be destroyed before\n> +\t * PlatformFrameBufferAllocator is destroyed.\n> +\t */\n\nThis applies to both backends I assume\n\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> +\t\t\t\t\t\t\t\t\t\\\n\nNo empty line if you have none between the other declarations\n\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..9d7e3c88\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> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +namespace {\n> +class CrosFrameBufferData : public FrameBuffer::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n\nempty 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\nempty line\n\n> +private:\n> +\tcros::ScopedBufferHandle scopedHandle_;\n> +};\n> +} /* namespace */\n> +\n> +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> +\n> +public:\n> +\tPrivate([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> +\t\t[[maybe_unused]] CameraDevice *const cameraDevice)\n> +\t{\n> +\t}\n> +\n> +\t~Private() = default;\n> +\n> +\tstd::unique_ptr allocate(\n> +\t\tint halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> +};\n> +\n> +std::unique_ptr\n> +PlatformFrameBufferAllocator::Private::allocate(\n> +\tint halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> +{\n> +\tcros::ScopedBufferHandle scopedHandle =\n> +\t\tcros::CameraBufferManager::AllocateScopedBuffer(\n> +\t\t\tsize.width, size.height, halPixelFormat, usage);\n> +\tif (!scopedHandle) {\n> +\t\tLOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tbuffer_handle_t handle = *scopedHandle;\n> +\tconst size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> +\tstd::vector planes(numPlanes);\n> +\tFileDescriptor fd{ handle->data[0] };\n> +\tif (!fd.isValid()) {\n> +\t\tLOG(HAL, Fatal) << \"Invalid fd\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n\n Also this assumes all planes are contigous, right ?\n\n> +\tfor (auto [i, plane] : utils::enumerate(planes)) {\n> +\t\tplane.fd = fd;\n> +\t\tplane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> +\t\tplane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> +\t}\n> +\n> +\treturn std::make_unique(\n> +\t\tstd::make_unique(std::move(scopedHandle)),\n> +\t\tplanes);\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> new file mode 100644\n> index 00000000..1bc3c405\n> --- /dev/null\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -0,0 +1,140 @@\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\nempty line\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\nIn the other implementation you had an empty line here\n\n> +\t~GenericFrameBufferData()\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\nIn the meantime I would be tempted to ASSERT(allocDevice_) to catch\npotential issues early.\n\n> +\t\t *\n> +\t\t * Q: 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\nempty line\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\noverride ?\n\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, Error) << \"gralloc_open() failed: \" << ret;\n\nShould this be Fatal ?\n\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\nI don't know the gralloc API but can (!ret && !handle)\n\n> +\n> +\tconst libcamera::PixelFormat pixelFormat =\n> +\t\tcameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> +\tconst auto &info = PixelFormatInfo::info(pixelFormat);\n> +\tconst unsigned int numPlanes = info.numPlanes();\n> +\tstd::vector planes(numPlanes);\n> +\n> +\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\nVery nice overall!\n\nBefore merging, do you think it would be possible to replace the usage\nof FrameBufferAllocator in CameraStream with this new API and run CTS\non it ?\n\nThanks\n j\n\n> android_deps += [dependency('libcros_camera')]\n> endif\n> --\n> 2.33.1.1089.g2158813163f-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A4110BDB1C\n\tfor ;\n\tFri, 29 Oct 2021 15:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2BD1600BD;\n\tFri, 29 Oct 2021 17:08:44 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B60CB600B8\n\tfor ;\n\tFri, 29 Oct 2021 17:08:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id D2F79E0006;\n\tFri, 29 Oct 2021 15:08:42 +0000 (UTC)"],"Date":"Fri, 29 Oct 2021 17:09:33 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211029150933.qia7ti3vgicl2uoj@uno.localdomain>","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211028073038.653786-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 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":20645,"web_url":"https://patchwork.libcamera.org/comment/20645/","msgid":"","date":"2021-11-01T05:09:04","subject":"Re: [libcamera-devel] [PATCH 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 Sat, Oct 30, 2021 at 12:08 AM Jacopo Mondi wrote:\n>\n> On Thu, Oct 28, 2021 at 04:30:38PM +0900, Hirokazu Honda wrote:\n> > After we unify identical streams to one stream configuration, the\n> > capture requested by a HAL client can have been resolved as\n> > CameraStream::Mapped. That is, a buffer to be written by a camera\n> > is not provided by a client. We would handle this case by\n> > dynamically allocating FrameBuffer.\n> >\n> > The existing FrameBufferAllocator cannot used because it is not\n> > allowed to allocate a new buffer while Camera is running.\n>\n> That's a nice context but seems slightly unrelated to this patch :)\n> >\n> > This CL introduces PlatformFrameBufferAllocator. It allocates\n>\n> After years, I still don't know what CL means\n>\n\nOops.. CL stands for change list. It is Google idom. Removed.\nhttps://stackoverflow.com/questions/25716920/what-does-cl-mean-in-a-commit-message-what-does-it-stand-for\n\n> > FrameBuffer using cros::CameraBufferManager on ChromeOS and\n> > gralloc on non ChromeOS platform. The allocated FrameBuffer owns\n> > the underlying buffer but must be destroyed before\n> > PlatformFrameBufferAllocator is destroyed.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/frame_buffer_allocator.h | 55 +++++++\n> > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++\n> > .../mm/generic_frame_buffer_allocator.cpp | 140 ++++++++++++++++++\n> > src/android/mm/meson.build | 6 +-\n> > 4 files changed, 287 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..e46a43b5\n> > --- /dev/null\n> > +++ b/src/android/frame_buffer_allocator.h\n> > @@ -0,0 +1,55 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in\n> > + * platform dependent way.\n> > + */\n> > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +class CameraDevice;\n> > +\n> > +class PlatformFrameBufferAllocator : libcamera::Extensible\n> > +{\n> > + 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>\n> This applies to both backends I assume\n>\n\nActually this restriction doesn't apply cros_frame_buffer_allocator.\ncros::ScopedBufferHandle will be destroyed by using the leaky\ncros::CameraBufferManager instance.\nSee https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=593;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n\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> > + \\\n>\n> No empty line if you have none between the other declarations\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..9d7e3c88\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> > +#include \"cros-camera/camera_buffer_manager.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > +namespace {\n> > +class CrosFrameBufferData : public FrameBuffer::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n>\n> empty 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> empty line\n>\n> > +private:\n> > + cros::ScopedBufferHandle scopedHandle_;\n> > +};\n> > +} /* namespace */\n> > +\n> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)\n> > +\n> > +public:\n> > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,\n> > + [[maybe_unused]] CameraDevice *const cameraDevice)\n> > + {\n> > + }\n> > +\n> > + ~Private() = default;\n> > +\n> > + std::unique_ptr allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage);\n> > +};\n> > +\n> > +std::unique_ptr\n> > +PlatformFrameBufferAllocator::Private::allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage)\n> > +{\n> > + cros::ScopedBufferHandle scopedHandle =\n> > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > + size.width, size.height, halPixelFormat, usage);\n> > + if (!scopedHandle) {\n> > + LOG(HAL, Error) << \"Failed to allocate buffer handle\";\n> > + return nullptr;\n> > + }\n> > +\n> > + buffer_handle_t handle = *scopedHandle;\n> > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);\n> > + std::vector planes(numPlanes);\n> > + FileDescriptor fd{ handle->data[0] };\n> > + if (!fd.isValid()) {\n> > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > + return nullptr;\n> > + }\n> > +\n>\n> Also this assumes all planes are contigous, right ?\n>\n> > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > + plane.fd = fd;\n> > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);\n> > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);\n> > + }\n> > +\n> > + return std::make_unique(\n> > + std::make_unique(std::move(scopedHandle)),\n> > + planes);\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > new file mode 100644\n> > index 00000000..1bc3c405\n> > --- /dev/null\n> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> > @@ -0,0 +1,140 @@\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> empty line\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> In the other implementation you had an empty line here\n>\n> > + ~GenericFrameBufferData()\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> In the meantime I would be tempted to ASSERT(allocDevice_) to catch\n> potential issues early.\n>\n> > + *\n> > + * Q: Thread safety against alloc_device_t is not documented.\n> > + * Is it no problem to call alloc/free in parallel?\n> > + */\n> > + allocDevice_->free(allocDevice_, handle_);\n> > + }\n>\n> empty line\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> override ?\n\nWe don't need override because allocate() is not Extensible::Private function.\n\n>\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, Error) << \"gralloc_open() failed: \" << ret;\n>\n> Should this be Fatal ?\n>\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> I don't know the gralloc API but can (!ret && !handle)\n>\n\nI would like to check independently. Or this can be ASSERT(!handle).\n\n> > +\n> > + const libcamera::PixelFormat pixelFormat =\n> > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> > + const auto &info = PixelFormatInfo::info(pixelFormat);\n> > + const unsigned int numPlanes = info.numPlanes();\n> > + std::vector planes(numPlanes);\n> > +\n> > + /* This code assumes the planes are mapped consecutively. */\n> > + FileDescriptor fd{ handle->data[0] };\n> > + size_t offset = 0;\n> > + for (auto [i, plane] : utils::enumerate(planes)) {\n> > + const size_t planeSize = info.planeSize(size.height, i, stride);\n> > +\n> > + planes[i].fd = fd;\n> > + planes[i].offset = offset;\n> > + planes[i].length = planeSize;\n> > + offset += planeSize;\n> > + }\n> > +\n> > + return std::make_unique(\n> > + std::make_unique(allocDevice_, handle),\n> > + planes);\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > index eeb5cc2e..d40a3b0b 100644\n> > --- a/src/android/mm/meson.build\n> > +++ b/src/android/mm/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > platform = get_option('android_platform')\n> > if platform == 'generic'\n> > - android_hal_sources += files(['generic_camera_buffer.cpp'])\n> > + android_hal_sources += files(['generic_camera_buffer.cpp',\n> > + 'generic_frame_buffer_allocator.cpp'])\n> > elif platform == 'cros'\n> > - android_hal_sources += files(['cros_camera_buffer.cpp'])\n> > + android_hal_sources += files(['cros_camera_buffer.cpp',\n> > + 'cros_frame_buffer_allocator.cpp'])\n>\n> Very nice overall!\n>\n> Before merging, do you think it would be possible to replace the usage\n> of FrameBufferAllocator in CameraStream with this new API and run CTS\n> on it ?\n>\n\nSure. I will do.\n\n-Hiro\n> Thanks\n> j\n>\n> > android_deps += [dependency('libcros_camera')]\n> > endif\n> > --\n> > 2.33.1.1089.g2158813163f-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DFA6ABDB1C\n\tfor ;\n\tMon, 1 Nov 2021 05:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 419E0600BD;\n\tMon, 1 Nov 2021 06:09:17 +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 DE081600B6\n\tfor ;\n\tMon, 1 Nov 2021 06:09:15 +0100 (CET)","by mail-ed1-x531.google.com with SMTP id w1so6513478edd.10\n\tfor ;\n\tSun, 31 Oct 2021 22:09:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"CX4kmZut\"; 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=wFSD6DB61hY76xfjySpBADvOjKVc/bu1gcU3uW1iM0g=;\n\tb=CX4kmZut5bzG/PGG18FLfL4ZuJJLFcRAFP6LS+MTomamWd706cj/ZURldjq2w77wAa\n\tnCVp2y7JGumxno/me5sGHO/SHxUv7iUrgMsElGaFs2J/soDq4IqK3SxIxEINyo5JAJnI\n\twFCmctNd8Smr4V7GrLeMNrxHk4+rOwPpO+33U=","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=wFSD6DB61hY76xfjySpBADvOjKVc/bu1gcU3uW1iM0g=;\n\tb=AK728mQCuFjo4U2qTLeiddKPciGWNlIB7nXz+eNElp6pXBM1Zxc66H5pBe63Iw7kPn\n\t/8n85W4EVs78amWr2lDa8jaSfk6nktdL0tXkPQ0f6S3BEeKVo0AH2ti1h7ekw4xgiGJb\n\tSL1jZYkNZlpzvLvPkFLsBfyY+VQCLvbOXjQj69sa//449M1j7fL7DGICjAhQLb/ajMIC\n\trVumTD9aMa9mUN/AMC4ROVXAeLQ9rB+RnCMmqeDDc9PZTn9gCTvJ86WYYWZrjmaWefyv\n\t4rpEUY6W11n5o6aMK1RkuoaaEkHf+LV1KBRaWMY6IcZJcFP7dh+WLs3G4CvIuwGoWwhQ\n\tbUog==","X-Gm-Message-State":"AOAM532h0TSIHs6uQRPkr9HBC1Ab3TLD3AImS+JhQOwpgSITCNsgDFm1\n\tjXv6EhuMO0h5eDZLfG0MUEiHaLZCLS3pbNaqbRfSsw==","X-Google-Smtp-Source":"ABdhPJwy58makLRBqToqgpeG2T79gAKVGzo+ldfZx6eqCGugwqjWfjVr5gzq0k2X6J2hBvxLOp/eEIsLE0d1WkPSle0=","X-Received":"by 2002:a17:906:1456:: with SMTP id\n\tq22mr33181787ejc.291.1635743355372; \n\tSun, 31 Oct 2021 22:09:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-3-hiroh@chromium.org>\n\t<20211029150933.qia7ti3vgicl2uoj@uno.localdomain>","In-Reply-To":"<20211029150933.qia7ti3vgicl2uoj@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 1 Nov 2021 14:09:04 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]