[{"id":20162,"web_url":"https://patchwork.libcamera.org/comment/20162/","msgid":"","date":"2021-10-13T03:18:26","subject":"Re: [libcamera-devel] [RFC PATCH 1/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:\n> PlatformFrameBufferAllocator allocates FrameBuffer(s) using\n> cros::CameraBufferManager on ChromeOS and gralloc on non\n> ChromeOS platform. The allocated FrameBuffer(s) are owned by\n> PlatformFrameBufferAllocator an destroyed when\n\ns/an/and/\n\n> PlatformFrameBufferAllocator is destroyed.\n\nIt would also be useful to explain in the commit message why this is\nneeded.\n\nOverall the patch looks pretty good. I think we're reaching a point\nwhere we'll have to document Android classes such as this one though,\nbut it doesn't have to be part of this series.\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/frame_buffer.h | 53 +++++++++++\n> src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 4 files changed, 258 insertions(+), 2 deletions(-)\n> create mode 100644 src/android/frame_buffer.h\n> create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> \n> diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h\n> new file mode 100644\n> index 00000000..6aafeaf3\n> --- /dev/null\n> +++ b/src/android/frame_buffer.h\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * frame_buffer.h - Frame buffer allocating interface definition\n\ns/allocating/allocator/\n\n> + */\n> +#ifndef __ANDROID_FRAME_BUFFER_H__\n> +#define __ANDROID_FRAME_BUFFER_H__\n> +\n> +#include \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> +\tconst std::vector> &\n> +\tallocate(int halPixelFormat,\n> +\t\t const libcamera::Size &size,\n> +\t\t uint32_t usage,\n> +\t\t size_t numBuffers);\n> +};\n> +\n> +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION\t\t\t\t\\\n\nI'm not a big fan of this design pattern, but it already exists in\ncamera_buffer.h, so we can keep it for now and address both later.\n\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> +const std::vector>&\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 size_t numBuffers)\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\treturn _d()->allocate(halPixelFormat, size, usage, numBuffers);\t\\\n> +}\n> +\n> +#endif /* __ANDROID_FRAME_BUFFER_H__ */\n> diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp\n> new file mode 100644\n> index 00000000..114c739b\n> --- /dev/null\n> +++ b/src/android/mm/cros_frame_buffer.cpp\n> @@ -0,0 +1,85 @@\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 buffer backend\n\ns/allocate/Allocate/\n\n> + * using CameraBufferManager\n> + */\n> +\n> +#include \"../frame_buffer.h\"\n> +\n> +#include \n> +\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +#include \"../camera_device.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\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> +\tconst std::vector> &\n> +\tallocate(int halPixelFormat,\n> +\t\t const libcamera::Size &size,\n> +\t\t uint32_t usage,\n> +\t\t size_t numBuffers);\n> +\n> +private:\n> +\tstd::vector> allocatedBuffers_;\n> +\tstd::vector allocatedHandles_;\n> +};\n> +\n> +const std::vector> &\n> +PlatformFrameBufferAllocator::Private::allocate(\n> +\tint halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> +\tsize_t numBuffers)\n> +{\n> +\tASSERT(allocatedBuffers_.empty());\n> +\n> +\tstd::vector> buffers;\n> +\tfor (size_t i = 0; i < numBuffers; ++i) {\n> +\t\tcros::ScopedBufferHandle handle =\n> +\t\t\tcros::CameraBufferManager::AllocateScopedBuffer(\n> +\t\t\t\tsize.width, size.height, halPixelFormat, usage);\n> +\t\tif (!handle) {\n> +\t\t\tLOG(HAL, Error) << \"Failed allocate buffer_handle\";\n\ns/Failed/Failed to/\n\n> +\t\t\treturn allocatedBuffers_;\n\nI think\n\n\t\t\treturn {};\n\nwould be more explicit. For a moment I thought the function would return\na partial vector of some of the allocations succeeded. Same below.\n\n> +\t\t}\n> +\n> +\t\tconst size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);\n> +\t\tstd::vector planes(numPlanes);\n> +\t\tfor (size_t j = 0; j < numPlanes; ++j) {\n\nThis could also be written\n\n\t\tfor (auto [j, plane] : utils::enumerate(planes)) {\n\nUp to you.\n\n> +\t\t\tFileDescriptor fd{ (*handle)->data[0] };\n> +\t\t\tif (!fd.isValid()) {\n> +\t\t\t\tLOG(HAL, Fatal) << \"Invalid fd\";\n> +\t\t\t\treturn allocatedBuffers_;\n> +\t\t\t}\n\nYou can move this outside of the loop. FileDescriptor uses implicit\nsharing of the actual fd, so all instances of planes[j].fd will use the\nsame numerical fd, avoiding the dup() call and saving file descriptor\nspace.\n\n> +\n> +\t\t\tplanes[j].fd = fd;\n> +\t\t\tplanes[j].offset =\n> +\t\t\t\tcros::CameraBufferManager::GetPlaneOffset(*handle, j);\n> +\t\t\tplanes[j].length =\n> +\t\t\t\tcros::CameraBufferManager::GetPlaneSize(*handle, j);\n> +\t\t}\n> +\n> +\t\tbuffers.push_back(std::make_unique(planes));\n> +\t\tallocatedHandles_.push_back(std::move(handle));\n> +\t}\n> +\n> +\tallocatedBuffers_ = std::move(buffers);\n> +\treturn allocatedBuffers_;\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp\n> new file mode 100644\n> index 00000000..b387d5a2\n> --- /dev/null\n> +++ b/src/android/mm/generic_frame_buffer.cpp\n> @@ -0,0 +1,116 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame\n> + * buffer backend\n> + */\n> +\n> +#include \"../frame_buffer.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../camera_device.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\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\nallocDevice_ should be initialized to nullptr or the destructor could\nend up calling gralloc_close() on a random pointer.\n\n> +\t{\n> +\t\tASSERT(!!hardwareModule_);\n\nWhy not\n\n\t\tASSERT(hardwareModule_);\n\n?\n\n> +\t}\n> +\n> +\t~Private() override;\n> +\n> +\tconst std::vector> &\n> +\tallocate(int halPixelFormat,\n> +\t\t const libcamera::Size &size,\n> +\t\t uint32_t usage,\n> +\t\t size_t numBuffers);\n> +\n> +private:\n> +\tconst CameraDevice *const cameraDevice_;\n> +\tstruct hw_module_t *const hardwareModule_;\n> +\tstruct alloc_device_t *allocDevice_;\n> +\n> +\tstd::vector> allocatedBuffers_;\n> +\tstd::vector bufferHandles_;\n> +};\n> +\n> +PlatformFrameBufferAllocator::Private::~Private()\n> +{\n> +\tfor (buffer_handle_t &handle : bufferHandles_) {\n> +\t\tASSERT(allocDevice_);\n\nI would drop the assert, this really can't happen.\n\n> +\t\tallocDevice_->free(allocDevice_, handle);\n> +\t}\n\nA blank line would be nice.\n\n> +\tif (allocDevice_)\n> +\t\tgralloc_close(allocDevice_);\n> +}\n> +\n> +const std::vector> &\n> +PlatformFrameBufferAllocator::Private::allocate(\n> +\tint halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> +\tsize_t numBuffers)\n> +{\n> +\tASSERT(allocatedBuffers_.empty());\n> +\tASSERT(bufferHandles_.empty());\n> +\tASSERT(!allocDevice_);\n> +\n> +\tint ret = gralloc_open(hardwareModule_, &allocDevice_);\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"gralloc_open() failed: \" << ret;\n> +\t\treturn allocatedBuffers_;\n\nSame here,\n\n\t\treturn {};\n\n> +\t}\n> +\n> +\tint stride = 0;\n> +\tfor (size_t i = 0; i < numBuffers; ++i) {\n> +\t\tbuffer_handle_t handle{};\n> +\t\tret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> +\t\t\t\t\t halPixelFormat, usage, &handle, &stride);\n\nI suppose the stride is guaranteed to be the same for all allocations ?\n\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> +\t\t\treturn allocatedBuffers_;\n> +\t\t}\n> +\n> +\t\tbufferHandles_.push_back(handle);\n> +\t}\n> +\n> +\tconst libcamera::PixelFormat pixelFormat =\n> +\t\tcameraDevice_->capabilities()->toPixelFormat(halPixelFormat);\n> +\tconst auto &info = PixelFormatInfo::info(pixelFormat);\n> +\tconst unsigned int numPlanes = info.numPlanes();\n> +\n> +\tallocatedBuffers_.reserve(numBuffers);\n> +\tfor (const buffer_handle_t &handle : bufferHandles_) {\n> +\t\tstd::vector planes(numPlanes);\n> +\t\tsize_t offset = 0;\n> +\t\tfor (size_t i = 0; i < numPlanes; ++i) {\n\nHere too you could use utils::enumerate().\n\n> +\t\t\tplanes[i].fd = FileDescriptor(handle->data[i]);\n> +\t\t\tsize_t planeSize = info.planeSize(size.height, i, stride);\n> +\n> +\t\t\tplanes[i].offset = offset;\n\nDoes Android use a different dmabuf fd for each plane ? If not, I think\nthe offset should be 0, and if it does, we could move the FileDescriptor\nconstruction outside of the loop.\n\n> +\t\t\tplanes[i].length = planeSize;\n> +\t\t\toffset += planeSize;\n> +\t\t}\n> +\n> +\t\tallocatedBuffers_.push_back(std::make_unique(planes));\n> +\t}\n> +\n> +\treturn allocatedBuffers_;\n> +}\n> +\n> +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> index eeb5cc2e..d1ad64d9 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.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.cpp'])\n> android_deps += [dependency('libcros_camera')]\n> endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E178C323E\n\tfor ;\n\tWed, 13 Oct 2021 03:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5E7F68F4F;\n\tWed, 13 Oct 2021 05:18:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E2C8604FF\n\tfor ;\n\tWed, 13 Oct 2021 05:18:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C28C0291;\n\tWed, 13 Oct 2021 05:18:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VPVx5Syg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634095121;\n\tbh=QkgYTujDzVnAlFnMpftB/NXuSXmAkEjpCK5EeV9bawc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VPVx5Syget3j5HjjiBhykThF3vAfhQbs6Au9GkdreLlPWBm4baMNp0xR60kVcq0V+\n\t2ElX0znk0nNkU7cGLZF8eUPkP2hA2q+aQmFgfIMLEc4yhBLo/yA1lY0ve+il4Qa3d5\n\tAqyD34cjcnXSmKwseFjCn6MwagTNHTTBVaOjCAdQ=","Date":"Wed, 13 Oct 2021 06:18:26 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927104821.2526508-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/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":20325,"web_url":"https://patchwork.libcamera.org/comment/20325/","msgid":"","date":"2021-10-20T04:01:46","subject":"Re: [libcamera-devel] [RFC PATCH 1/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 Laurent, thank you for comments.\n\nOn Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:\n> > PlatformFrameBufferAllocator allocates FrameBuffer(s) using\n> > cros::CameraBufferManager on ChromeOS and gralloc on non\n> > ChromeOS platform. The allocated FrameBuffer(s) are owned by\n> > PlatformFrameBufferAllocator an destroyed when\n>\n> s/an/and/\n>\n> > PlatformFrameBufferAllocator is destroyed.\n>\n> It would also be useful to explain in the commit message why this is\n> needed.\n>\n> Overall the patch looks pretty good. I think we're reaching a point\n> where we'll have to document Android classes such as this one though,\n> but it doesn't have to be part of this series.\n>\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/frame_buffer.h | 53 +++++++++++\n> > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > src/android/mm/meson.build | 6 +-\n> > 4 files changed, 258 insertions(+), 2 deletions(-)\n> > create mode 100644 src/android/frame_buffer.h\n> > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> >\n> > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h\n> > new file mode 100644\n> > index 00000000..6aafeaf3\n> > --- /dev/null\n> > +++ b/src/android/frame_buffer.h\n> > @@ -0,0 +1,53 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * frame_buffer.h - Frame buffer allocating interface definition\n>\n> s/allocating/allocator/\n>\n> > + */\n> > +#ifndef __ANDROID_FRAME_BUFFER_H__\n> > +#define __ANDROID_FRAME_BUFFER_H__\n> > +\n> > +#include \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> > + const std::vector> &\n> > + allocate(int halPixelFormat,\n> > + const libcamera::Size &size,\n> > + uint32_t usage,\n> > + size_t numBuffers);\n> > +};\n> > +\n> > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION \\\n>\n> I'm not a big fan of this design pattern, but it already exists in\n> camera_buffer.h, so we can keep it for now and address both later.\n>\n> > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > + CameraDevice *const cameraDevice) \\\n> > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > +{ \\\n> > +} \\\n> > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > +{ \\\n> > +} \\\n> > +const std::vector>& \\\n> > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > + const libcamera::Size& size, \\\n> > + uint32_t usage, \\\n> > + size_t numBuffers) \\\n> > +{ \\\n> > + return _d()->allocate(halPixelFormat, size, usage, numBuffers); \\\n> > +}\n> > +\n> > +#endif /* __ANDROID_FRAME_BUFFER_H__ */\n> > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp\n> > new file mode 100644\n> > index 00000000..114c739b\n> > --- /dev/null\n> > +++ b/src/android/mm/cros_frame_buffer.cpp\n> > @@ -0,0 +1,85 @@\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 buffer backend\n>\n> s/allocate/Allocate/\n>\n> > + * using CameraBufferManager\n> > + */\n> > +\n> > +#include \"../frame_buffer.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"cros-camera/camera_buffer_manager.h\"\n> > +\n> > +#include \"../camera_device.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\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> > + const std::vector> &\n> > + allocate(int halPixelFormat,\n> > + const libcamera::Size &size,\n> > + uint32_t usage,\n> > + size_t numBuffers);\n\nI think to return std::unique_ptr so that we\ndon't have to consider proper numBuffers.\nIt is necessary to attach something libcamera::FrameBuffer, for\nexample, cros::ScopedBufferHandle in cros_frame_buffer.\nHowever, libcamera::FrameBuffer is marked final. How should I solve\nthis problem?\nIs it fine to remove the final mark?\n\n-Hiro\n> > +\n> > +private:\n> > + std::vector> allocatedBuffers_;\n> > + std::vector allocatedHandles_;\n> > +};\n> > +\n> > +const std::vector> &\n> > +PlatformFrameBufferAllocator::Private::allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > + size_t numBuffers)\n> > +{\n> > + ASSERT(allocatedBuffers_.empty());\n> > +\n> > + std::vector> buffers;\n> > + for (size_t i = 0; i < numBuffers; ++i) {\n> > + cros::ScopedBufferHandle handle =\n> > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > + size.width, size.height, halPixelFormat, usage);\n> > + if (!handle) {\n> > + LOG(HAL, Error) << \"Failed allocate buffer_handle\";\n>\n> s/Failed/Failed to/\n>\n> > + return allocatedBuffers_;\n>\n> I think\n>\n> return {};\n>\n> would be more explicit. For a moment I thought the function would return\n> a partial vector of some of the allocations succeeded. Same below.\n>\n> > + }\n> > +\n> > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);\n> > + std::vector planes(numPlanes);\n> > + for (size_t j = 0; j < numPlanes; ++j) {\n>\n> This could also be written\n>\n> for (auto [j, plane] : utils::enumerate(planes)) {\n>\n> Up to you.\n>\n> > + FileDescriptor fd{ (*handle)->data[0] };\n> > + if (!fd.isValid()) {\n> > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > + return allocatedBuffers_;\n> > + }\n>\n> You can move this outside of the loop. FileDescriptor uses implicit\n> sharing of the actual fd, so all instances of planes[j].fd will use the\n> same numerical fd, avoiding the dup() call and saving file descriptor\n> space.\n>\n> > +\n> > + planes[j].fd = fd;\n> > + planes[j].offset =\n> > + cros::CameraBufferManager::GetPlaneOffset(*handle, j);\n> > + planes[j].length =\n> > + cros::CameraBufferManager::GetPlaneSize(*handle, j);\n> > + }\n> > +\n> > + buffers.push_back(std::make_unique(planes));\n> > + allocatedHandles_.push_back(std::move(handle));\n> > + }\n> > +\n> > + allocatedBuffers_ = std::move(buffers);\n> > + return allocatedBuffers_;\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp\n> > new file mode 100644\n> > index 00000000..b387d5a2\n> > --- /dev/null\n> > +++ b/src/android/mm/generic_frame_buffer.cpp\n> > @@ -0,0 +1,116 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame\n> > + * buffer backend\n> > + */\n> > +\n> > +#include \"../frame_buffer.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"../camera_device.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL)\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>\n> allocDevice_ should be initialized to nullptr or the destructor could\n> end up calling gralloc_close() on a random pointer.\n>\n> > + {\n> > + ASSERT(!!hardwareModule_);\n>\n> Why not\n>\n> ASSERT(hardwareModule_);\n>\n> ?\n>\n> > + }\n> > +\n> > + ~Private() override;\n> > +\n> > + const std::vector> &\n> > + allocate(int halPixelFormat,\n> > + const libcamera::Size &size,\n> > + uint32_t usage,\n> > + size_t numBuffers);\n> > +\n> > +private:\n> > + const CameraDevice *const cameraDevice_;\n> > + struct hw_module_t *const hardwareModule_;\n> > + struct alloc_device_t *allocDevice_;\n> > +\n> > + std::vector> allocatedBuffers_;\n> > + std::vector bufferHandles_;\n> > +};\n> > +\n> > +PlatformFrameBufferAllocator::Private::~Private()\n> > +{\n> > + for (buffer_handle_t &handle : bufferHandles_) {\n> > + ASSERT(allocDevice_);\n>\n> I would drop the assert, this really can't happen.\n>\n> > + allocDevice_->free(allocDevice_, handle);\n> > + }\n>\n> A blank line would be nice.\n>\n> > + if (allocDevice_)\n> > + gralloc_close(allocDevice_);\n> > +}\n> > +\n> > +const std::vector> &\n> > +PlatformFrameBufferAllocator::Private::allocate(\n> > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > + size_t numBuffers)\n> > +{\n> > + ASSERT(allocatedBuffers_.empty());\n> > + ASSERT(bufferHandles_.empty());\n> > + ASSERT(!allocDevice_);\n> > +\n> > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"gralloc_open() failed: \" << ret;\n> > + return allocatedBuffers_;\n>\n> Same here,\n>\n> return {};\n>\n> > + }\n> > +\n> > + int stride = 0;\n> > + for (size_t i = 0; i < numBuffers; ++i) {\n> > + buffer_handle_t handle{};\n> > + ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > + halPixelFormat, usage, &handle, &stride);\n>\n> I suppose the stride is guaranteed to be the same for all allocations ?\n>\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > + return allocatedBuffers_;\n> > + }\n> > +\n> > + bufferHandles_.push_back(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> > +\n> > + allocatedBuffers_.reserve(numBuffers);\n> > + for (const buffer_handle_t &handle : bufferHandles_) {\n> > + std::vector planes(numPlanes);\n> > + size_t offset = 0;\n> > + for (size_t i = 0; i < numPlanes; ++i) {\n>\n> Here too you could use utils::enumerate().\n>\n> > + planes[i].fd = FileDescriptor(handle->data[i]);\n> > + size_t planeSize = info.planeSize(size.height, i, stride);\n> > +\n> > + planes[i].offset = offset;\n>\n> Does Android use a different dmabuf fd for each plane ? If not, I think\n> the offset should be 0, and if it does, we could move the FileDescriptor\n> construction outside of the loop.\n>\n> > + planes[i].length = planeSize;\n> > + offset += planeSize;\n> > + }\n> > +\n> > + allocatedBuffers_.push_back(std::make_unique(planes));\n> > + }\n> > +\n> > + return allocatedBuffers_;\n> > +}\n> > +\n> > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > index eeb5cc2e..d1ad64d9 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.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.cpp'])\n> > android_deps += [dependency('libcros_camera')]\n> > endif\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 34086BDB1C\n\tfor ;\n\tWed, 20 Oct 2021 04:02:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BC8868F59;\n\tWed, 20 Oct 2021 06:02:00 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DD9E60125\n\tfor ;\n\tWed, 20 Oct 2021 06:01:58 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id i20so20648500edj.10\n\tfor ;\n\tTue, 19 Oct 2021 21:01:58 -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=\"BmPoVmEZ\"; 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=ty+CHCmylHKYKxTYfMylJNuphtel9j7H+tOAVbwL4Mo=;\n\tb=BmPoVmEZVMmjhqNf3v6i3cmXmyA7X1ZzI7BJ9jiwkCBtD3tGd4Z3B+6uDZgW9cSTpI\n\tNGflfFCmu5s3j0LJsEP0yJWhBq99GzeKQdMjkfstTSf/uBVkTDmg9pfpM7itY6qmkzAH\n\tWNhcoTPHndpZ2e8w/w7HNPLJQEZKImtHrU0hI=","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=ty+CHCmylHKYKxTYfMylJNuphtel9j7H+tOAVbwL4Mo=;\n\tb=3gheHU8oll3D1BF2vtKoBhHCk6ac+kLEjgGXsEgqMF8WdqdyvHqXUGH1ZqYTyw1edH\n\t1tro076zy5eQ2mtj/WVi7F0WWVHPLrrB07VcdUsPA/6Ox37JX9V0Igu68wD3qwDVWxN9\n\tzkhemqlmUyXsshsn+POOR/atVZJf+8Z086eGLgm/AszDrrraPivSfGxT/QEyCAbQl1aX\n\tNoDjfjU1U1hUIoq9iQOkRMS/4pyhTaoF6EJw+ybU0CGxWHXd3sZExCADjN6mNJo1/Jer\n\tk08qyL8xdEDvFAa8S3OF+xSXvXnfEGjUE6UkUrtkJi2ODXnlhGMIzQm9ZaDhPs9bteRn\n\tThWA==","X-Gm-Message-State":"AOAM531SzBf5FOavKH2MJST3rP+QL741NtMcRkPad05r9IFEySF5SsWo\n\tDUt9TXcUbYpm6Es7ex+qOzlpDAU4ee1kOXuVGbNSWw==","X-Google-Smtp-Source":"ABdhPJzKHFZYrSd/0slwHSr2oNFCU3kLIlN9O4wjt40CRMMWZpHfp3iLO/ouGl8ziHkluHPCidgYVkUT655VzUs4Hpg=","X-Received":"by 2002:a50:cd97:: with SMTP id\n\tp23mr59387280edi.206.1634702517778; \n\tTue, 19 Oct 2021 21:01:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 20 Oct 2021 13:01:46 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20403,"web_url":"https://patchwork.libcamera.org/comment/20403/","msgid":"","date":"2021-10-25T03:31:45","subject":"Re: [libcamera-devel] [RFC PATCH 1/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Laurent, gentle ping.\n\nOn Wed, Oct 20, 2021 at 1:01 PM Hirokazu Honda wrote:\n>\n> Hi Laurent, thank you for comments.\n>\n> On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:\n> > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using\n> > > cros::CameraBufferManager on ChromeOS and gralloc on non\n> > > ChromeOS platform. The allocated FrameBuffer(s) are owned by\n> > > PlatformFrameBufferAllocator an destroyed when\n> >\n> > s/an/and/\n> >\n> > > PlatformFrameBufferAllocator is destroyed.\n> >\n> > It would also be useful to explain in the commit message why this is\n> > needed.\n> >\n> > Overall the patch looks pretty good. I think we're reaching a point\n> > where we'll have to document Android classes such as this one though,\n> > but it doesn't have to be part of this series.\n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/frame_buffer.h | 53 +++++++++++\n> > > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 4 files changed, 258 insertions(+), 2 deletions(-)\n> > > create mode 100644 src/android/frame_buffer.h\n> > > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> > >\n> > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h\n> > > new file mode 100644\n> > > index 00000000..6aafeaf3\n> > > --- /dev/null\n> > > +++ b/src/android/frame_buffer.h\n> > > @@ -0,0 +1,53 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * frame_buffer.h - Frame buffer allocating interface definition\n> >\n> > s/allocating/allocator/\n> >\n> > > + */\n> > > +#ifndef __ANDROID_FRAME_BUFFER_H__\n> > > +#define __ANDROID_FRAME_BUFFER_H__\n> > > +\n> > > +#include \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> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n> > > +};\n> > > +\n> > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION \\\n> >\n> > I'm not a big fan of this design pattern, but it already exists in\n> > camera_buffer.h, so we can keep it for now and address both later.\n> >\n> > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > + CameraDevice *const cameraDevice) \\\n> > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > +{ \\\n> > > +} \\\n> > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > +{ \\\n> > > +} \\\n> > > +const std::vector>& \\\n> > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > + const libcamera::Size& size, \\\n> > > + uint32_t usage, \\\n> > > + size_t numBuffers) \\\n> > > +{ \\\n> > > + return _d()->allocate(halPixelFormat, size, usage, numBuffers); \\\n> > > +}\n> > > +\n> > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */\n> > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp\n> > > new file mode 100644\n> > > index 00000000..114c739b\n> > > --- /dev/null\n> > > +++ b/src/android/mm/cros_frame_buffer.cpp\n> > > @@ -0,0 +1,85 @@\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 buffer backend\n> >\n> > s/allocate/Allocate/\n> >\n> > > + * using CameraBufferManager\n> > > + */\n> > > +\n> > > +#include \"../frame_buffer.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\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> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n>\n> I think to return std::unique_ptr so that we\n> don't have to consider proper numBuffers.\n> It is necessary to attach something libcamera::FrameBuffer, for\n> example, cros::ScopedBufferHandle in cros_frame_buffer.\n> However, libcamera::FrameBuffer is marked final. How should I solve\n> this problem?\n> Is it fine to remove the final mark?\n>\n> -Hiro\n> > > +\n> > > +private:\n> > > + std::vector> allocatedBuffers_;\n> > > + std::vector allocatedHandles_;\n> > > +};\n> > > +\n> > > +const std::vector> &\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > + size_t numBuffers)\n> > > +{\n> > > + ASSERT(allocatedBuffers_.empty());\n> > > +\n> > > + std::vector> buffers;\n> > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > + cros::ScopedBufferHandle handle =\n> > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > + size.width, size.height, halPixelFormat, usage);\n> > > + if (!handle) {\n> > > + LOG(HAL, Error) << \"Failed allocate buffer_handle\";\n> >\n> > s/Failed/Failed to/\n> >\n> > > + return allocatedBuffers_;\n> >\n> > I think\n> >\n> > return {};\n> >\n> > would be more explicit. For a moment I thought the function would return\n> > a partial vector of some of the allocations succeeded. Same below.\n> >\n> > > + }\n> > > +\n> > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);\n> > > + std::vector planes(numPlanes);\n> > > + for (size_t j = 0; j < numPlanes; ++j) {\n> >\n> > This could also be written\n> >\n> > for (auto [j, plane] : utils::enumerate(planes)) {\n> >\n> > Up to you.\n> >\n> > > + FileDescriptor fd{ (*handle)->data[0] };\n> > > + if (!fd.isValid()) {\n> > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > + return allocatedBuffers_;\n> > > + }\n> >\n> > You can move this outside of the loop. FileDescriptor uses implicit\n> > sharing of the actual fd, so all instances of planes[j].fd will use the\n> > same numerical fd, avoiding the dup() call and saving file descriptor\n> > space.\n> >\n> > > +\n> > > + planes[j].fd = fd;\n> > > + planes[j].offset =\n> > > + cros::CameraBufferManager::GetPlaneOffset(*handle, j);\n> > > + planes[j].length =\n> > > + cros::CameraBufferManager::GetPlaneSize(*handle, j);\n> > > + }\n> > > +\n> > > + buffers.push_back(std::make_unique(planes));\n> > > + allocatedHandles_.push_back(std::move(handle));\n> > > + }\n> > > +\n> > > + allocatedBuffers_ = std::move(buffers);\n> > > + return allocatedBuffers_;\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp\n> > > new file mode 100644\n> > > index 00000000..b387d5a2\n> > > --- /dev/null\n> > > +++ b/src/android/mm/generic_frame_buffer.cpp\n> > > @@ -0,0 +1,116 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame\n> > > + * buffer backend\n> > > + */\n> > > +\n> > > +#include \"../frame_buffer.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\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> >\n> > allocDevice_ should be initialized to nullptr or the destructor could\n> > end up calling gralloc_close() on a random pointer.\n> >\n> > > + {\n> > > + ASSERT(!!hardwareModule_);\n> >\n> > Why not\n> >\n> > ASSERT(hardwareModule_);\n> >\n> > ?\n> >\n> > > + }\n> > > +\n> > > + ~Private() override;\n> > > +\n> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n> > > +\n> > > +private:\n> > > + const CameraDevice *const cameraDevice_;\n> > > + struct hw_module_t *const hardwareModule_;\n> > > + struct alloc_device_t *allocDevice_;\n> > > +\n> > > + std::vector> allocatedBuffers_;\n> > > + std::vector bufferHandles_;\n> > > +};\n> > > +\n> > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > +{\n> > > + for (buffer_handle_t &handle : bufferHandles_) {\n> > > + ASSERT(allocDevice_);\n> >\n> > I would drop the assert, this really can't happen.\n> >\n> > > + allocDevice_->free(allocDevice_, handle);\n> > > + }\n> >\n> > A blank line would be nice.\n> >\n> > > + if (allocDevice_)\n> > > + gralloc_close(allocDevice_);\n> > > +}\n> > > +\n> > > +const std::vector> &\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > + size_t numBuffers)\n> > > +{\n> > > + ASSERT(allocatedBuffers_.empty());\n> > > + ASSERT(bufferHandles_.empty());\n> > > + ASSERT(!allocDevice_);\n> > > +\n> > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"gralloc_open() failed: \" << ret;\n> > > + return allocatedBuffers_;\n> >\n> > Same here,\n> >\n> > return {};\n> >\n> > > + }\n> > > +\n> > > + int stride = 0;\n> > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > + buffer_handle_t handle{};\n> > > + ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > + halPixelFormat, usage, &handle, &stride);\n> >\n> > I suppose the stride is guaranteed to be the same for all allocations ?\n> >\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > + return allocatedBuffers_;\n> > > + }\n> > > +\n> > > + bufferHandles_.push_back(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> > > +\n> > > + allocatedBuffers_.reserve(numBuffers);\n> > > + for (const buffer_handle_t &handle : bufferHandles_) {\n> > > + std::vector planes(numPlanes);\n> > > + size_t offset = 0;\n> > > + for (size_t i = 0; i < numPlanes; ++i) {\n> >\n> > Here too you could use utils::enumerate().\n> >\n> > > + planes[i].fd = FileDescriptor(handle->data[i]);\n> > > + size_t planeSize = info.planeSize(size.height, i, stride);\n> > > +\n> > > + planes[i].offset = offset;\n> >\n> > Does Android use a different dmabuf fd for each plane ? If not, I think\n> > the offset should be 0, and if it does, we could move the FileDescriptor\n> > construction outside of the loop.\n> >\n> > > + planes[i].length = planeSize;\n> > > + offset += planeSize;\n> > > + }\n> > > +\n> > > + allocatedBuffers_.push_back(std::make_unique(planes));\n> > > + }\n> > > +\n> > > + return allocatedBuffers_;\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > index eeb5cc2e..d1ad64d9 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.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.cpp'])\n> > > android_deps += [dependency('libcros_camera')]\n> > > endif\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 5723CBF415\n\tfor ;\n\tMon, 25 Oct 2021 03:32:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8F8964870;\n\tMon, 25 Oct 2021 05:32:09 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB32760237\n\tfor ;\n\tMon, 25 Oct 2021 05:31:54 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id l13so16272575edi.8\n\tfor ;\n\tSun, 24 Oct 2021 20:31:54 -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=\"MQKLklii\"; 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=1daOZj+UO5gi2dAXar5RhqRBxtbVYX6zmorQYttZ6Ec=;\n\tb=MQKLkliiTcewQGcjNIcoTlZiuasejnrE+27rVsJxGFh2h+hCCHJtOrDPLyzefL247h\n\tGPjBoFXOxPxFVFcLw/vDD2tIL0wPBxYAmKQT4fHo/oHFBN8XOBsOrbIjufU01mUdbsx9\n\tmaq9GiFjfR3gtqSZOICFXpHmtsxRlWbqmMvkM=","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=1daOZj+UO5gi2dAXar5RhqRBxtbVYX6zmorQYttZ6Ec=;\n\tb=yemrU+MJvoFuL7m0yo4pyxlsvW2z83IKHGMWp7kznXQsHoY0vfvsrucF417FVDT0Uu\n\t+Txs2Fu/37uk//dUAtzprDuz3XDELa4nLrenpzv8+xs8kx+Bm/LI3xOLM/fV+kbDNWaG\n\turuxYmyRmbpf/8YPTZkO6+qUQiNqbJ+uVMklngA7eRTwfx4Qg1aU52XZNrPcNkLLjrLB\n\tNoqFJSkVg45MBFdafU3yRiHi0o0BKtsGuqVNfsJcfy8zJbnx9HLf/VNPHdGDvhmQmrtE\n\tQAVdURt2iC6rTJRikosUfv4D4aqTs7w7vccOEQ1XpAMdKAtaLtv8sv1cYQtZmyRZj7Kt\n\tK8eg==","X-Gm-Message-State":"AOAM533hLYbbQUKjFCJsEkfpLQLud8Fe7E+CpJ4Wz1zGMST06LJOSmdt\n\tG1Qx44aTpjJA9ckoVrdjZiuZxipWJa0f4Nvkgui1ow==","X-Google-Smtp-Source":"ABdhPJzWL1PMd82YkHkvbPez5kM8d+T+/kqPpHywnM8BZ7s00OeNM39DFqNSVV09CBjvuHDW8Z0P5nFCDYnqfJ9fItU=","X-Received":"by 2002:a17:906:7313:: with SMTP id\n\tdi19mr18193611ejc.522.1635132714205; \n\tSun, 24 Oct 2021 20:31:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-2-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 12:31:45 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20598,"web_url":"https://patchwork.libcamera.org/comment/20598/","msgid":"","date":"2021-10-28T00:03:09","subject":"Re: [libcamera-devel] [RFC PATCH 1/2] android: Introduce\n\tPlatformFrameBufferAllocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Wed, Oct 20, 2021 at 01:01:46PM +0900, Hirokazu Honda wrote:\n> On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart wrote:\n> > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:\n> > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using\n> > > cros::CameraBufferManager on ChromeOS and gralloc on non\n> > > ChromeOS platform. The allocated FrameBuffer(s) are owned by\n> > > PlatformFrameBufferAllocator an destroyed when\n> >\n> > s/an/and/\n> >\n> > > PlatformFrameBufferAllocator is destroyed.\n> >\n> > It would also be useful to explain in the commit message why this is\n> > needed.\n> >\n> > Overall the patch looks pretty good. I think we're reaching a point\n> > where we'll have to document Android classes such as this one though,\n> > but it doesn't have to be part of this series.\n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/frame_buffer.h | 53 +++++++++++\n> > > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 4 files changed, 258 insertions(+), 2 deletions(-)\n> > > create mode 100644 src/android/frame_buffer.h\n> > > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> > >\n> > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h\n> > > new file mode 100644\n> > > index 00000000..6aafeaf3\n> > > --- /dev/null\n> > > +++ b/src/android/frame_buffer.h\n> > > @@ -0,0 +1,53 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * frame_buffer.h - Frame buffer allocating interface definition\n> >\n> > s/allocating/allocator/\n> >\n> > > + */\n> > > +#ifndef __ANDROID_FRAME_BUFFER_H__\n> > > +#define __ANDROID_FRAME_BUFFER_H__\n> > > +\n> > > +#include \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> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n> > > +};\n> > > +\n> > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION \\\n> >\n> > I'm not a big fan of this design pattern, but it already exists in\n> > camera_buffer.h, so we can keep it for now and address both later.\n> >\n> > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > + CameraDevice *const cameraDevice) \\\n> > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > +{ \\\n> > > +} \\\n> > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > +{ \\\n> > > +} \\\n> > > +const std::vector>& \\\n> > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > + const libcamera::Size& size, \\\n> > > + uint32_t usage, \\\n> > > + size_t numBuffers) \\\n> > > +{ \\\n> > > + return _d()->allocate(halPixelFormat, size, usage, numBuffers); \\\n> > > +}\n> > > +\n> > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */\n> > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp\n> > > new file mode 100644\n> > > index 00000000..114c739b\n> > > --- /dev/null\n> > > +++ b/src/android/mm/cros_frame_buffer.cpp\n> > > @@ -0,0 +1,85 @@\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 buffer backend\n> >\n> > s/allocate/Allocate/\n> >\n> > > + * using CameraBufferManager\n> > > + */\n> > > +\n> > > +#include \"../frame_buffer.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\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> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n> \n> I think to return std::unique_ptr so that we\n> don't have to consider proper numBuffers.\n\nAllocating buffers individually sounds good, I agree with you. Ideally\nwe should have the same API for the FrameBufferAllocator, but we can\nconsolidate the APIs later (FrameBufferAllocator will always have the\nlimitation that buffers can only be allocated between configure() and\nstart(), but hopefully in the future we will be able to drop this V4L2\nallocation backend and use dmabuf heaps).\n\n> It is necessary to attach something libcamera::FrameBuffer, for\n> example, cros::ScopedBufferHandle in cros_frame_buffer.\n> However, libcamera::FrameBuffer is marked final. How should I solve\n> this problem?\n> Is it fine to remove the final mark?\n\nFrameBuffer is an Extensible class, so you can subclass\nFrameBuffer::Private and add your custom data there without subclassing\nFrameBuffer. We do that with the Camera class already, it's declared as\nfinal, but pipeline handlers subclass Camera::Private.\n\n> > > +\n> > > +private:\n> > > + std::vector> allocatedBuffers_;\n> > > + std::vector allocatedHandles_;\n> > > +};\n> > > +\n> > > +const std::vector> &\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > + size_t numBuffers)\n> > > +{\n> > > + ASSERT(allocatedBuffers_.empty());\n> > > +\n> > > + std::vector> buffers;\n> > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > + cros::ScopedBufferHandle handle =\n> > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > + size.width, size.height, halPixelFormat, usage);\n> > > + if (!handle) {\n> > > + LOG(HAL, Error) << \"Failed allocate buffer_handle\";\n> >\n> > s/Failed/Failed to/\n> >\n> > > + return allocatedBuffers_;\n> >\n> > I think\n> >\n> > return {};\n> >\n> > would be more explicit. For a moment I thought the function would return\n> > a partial vector of some of the allocations succeeded. Same below.\n> >\n> > > + }\n> > > +\n> > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);\n> > > + std::vector planes(numPlanes);\n> > > + for (size_t j = 0; j < numPlanes; ++j) {\n> >\n> > This could also be written\n> >\n> > for (auto [j, plane] : utils::enumerate(planes)) {\n> >\n> > Up to you.\n> >\n> > > + FileDescriptor fd{ (*handle)->data[0] };\n> > > + if (!fd.isValid()) {\n> > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > + return allocatedBuffers_;\n> > > + }\n> >\n> > You can move this outside of the loop. FileDescriptor uses implicit\n> > sharing of the actual fd, so all instances of planes[j].fd will use the\n> > same numerical fd, avoiding the dup() call and saving file descriptor\n> > space.\n> >\n> > > +\n> > > + planes[j].fd = fd;\n> > > + planes[j].offset =\n> > > + cros::CameraBufferManager::GetPlaneOffset(*handle, j);\n> > > + planes[j].length =\n> > > + cros::CameraBufferManager::GetPlaneSize(*handle, j);\n> > > + }\n> > > +\n> > > + buffers.push_back(std::make_unique(planes));\n> > > + allocatedHandles_.push_back(std::move(handle));\n> > > + }\n> > > +\n> > > + allocatedBuffers_ = std::move(buffers);\n> > > + return allocatedBuffers_;\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp\n> > > new file mode 100644\n> > > index 00000000..b387d5a2\n> > > --- /dev/null\n> > > +++ b/src/android/mm/generic_frame_buffer.cpp\n> > > @@ -0,0 +1,116 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame\n> > > + * buffer backend\n> > > + */\n> > > +\n> > > +#include \"../frame_buffer.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"../camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL)\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> >\n> > allocDevice_ should be initialized to nullptr or the destructor could\n> > end up calling gralloc_close() on a random pointer.\n> >\n> > > + {\n> > > + ASSERT(!!hardwareModule_);\n> >\n> > Why not\n> >\n> > ASSERT(hardwareModule_);\n> >\n> > ?\n> >\n> > > + }\n> > > +\n> > > + ~Private() override;\n> > > +\n> > > + const std::vector> &\n> > > + allocate(int halPixelFormat,\n> > > + const libcamera::Size &size,\n> > > + uint32_t usage,\n> > > + size_t numBuffers);\n> > > +\n> > > +private:\n> > > + const CameraDevice *const cameraDevice_;\n> > > + struct hw_module_t *const hardwareModule_;\n> > > + struct alloc_device_t *allocDevice_;\n> > > +\n> > > + std::vector> allocatedBuffers_;\n> > > + std::vector bufferHandles_;\n> > > +};\n> > > +\n> > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > +{\n> > > + for (buffer_handle_t &handle : bufferHandles_) {\n> > > + ASSERT(allocDevice_);\n> >\n> > I would drop the assert, this really can't happen.\n> >\n> > > + allocDevice_->free(allocDevice_, handle);\n> > > + }\n> >\n> > A blank line would be nice.\n> >\n> > > + if (allocDevice_)\n> > > + gralloc_close(allocDevice_);\n> > > +}\n> > > +\n> > > +const std::vector> &\n> > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > + size_t numBuffers)\n> > > +{\n> > > + ASSERT(allocatedBuffers_.empty());\n> > > + ASSERT(bufferHandles_.empty());\n> > > + ASSERT(!allocDevice_);\n> > > +\n> > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"gralloc_open() failed: \" << ret;\n> > > + return allocatedBuffers_;\n> >\n> > Same here,\n> >\n> > return {};\n> >\n> > > + }\n> > > +\n> > > + int stride = 0;\n> > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > + buffer_handle_t handle{};\n> > > + ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > + halPixelFormat, usage, &handle, &stride);\n> >\n> > I suppose the stride is guaranteed to be the same for all allocations ?\n> >\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > + return allocatedBuffers_;\n> > > + }\n> > > +\n> > > + bufferHandles_.push_back(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> > > +\n> > > + allocatedBuffers_.reserve(numBuffers);\n> > > + for (const buffer_handle_t &handle : bufferHandles_) {\n> > > + std::vector planes(numPlanes);\n> > > + size_t offset = 0;\n> > > + for (size_t i = 0; i < numPlanes; ++i) {\n> >\n> > Here too you could use utils::enumerate().\n> >\n> > > + planes[i].fd = FileDescriptor(handle->data[i]);\n> > > + size_t planeSize = info.planeSize(size.height, i, stride);\n> > > +\n> > > + planes[i].offset = offset;\n> >\n> > Does Android use a different dmabuf fd for each plane ? If not, I think\n> > the offset should be 0, and if it does, we could move the FileDescriptor\n> > construction outside of the loop.\n> >\n> > > + planes[i].length = planeSize;\n> > > + offset += planeSize;\n> > > + }\n> > > +\n> > > + allocatedBuffers_.push_back(std::make_unique(planes));\n> > > + }\n> > > +\n> > > + return allocatedBuffers_;\n> > > +}\n> > > +\n> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > index eeb5cc2e..d1ad64d9 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.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.cpp'])\n> > > android_deps += [dependency('libcros_camera')]\n> > > endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 33F9FBF415\n\tfor ;\n\tThu, 28 Oct 2021 00:03:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3084E60009;\n\tThu, 28 Oct 2021 02:03:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0A7760006\n\tfor ;\n\tThu, 28 Oct 2021 02:03:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18007E7;\n\tThu, 28 Oct 2021 02:03:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hGL9KblE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635379413;\n\tbh=R7ZLQjSLUm4YLXuVNstJQAQqYssClzJewMYc60C9yXM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hGL9KblEqtkzfjjCNjnUxDvII4eTbP6bghb4acxVElINSc4ADQvA6Ny3EhtbMTh3r\n\tNMv5x8jWnpYUFRE4ApOhHN5haKsteRtlloIFY+kcevCi0BTI/v1QPUpEd82f0cwa3f\n\t8fzmDkbcCCPGWrxurMHvoTO0IQmHDR6GZEvUNIU8=","Date":"Thu, 28 Oct 2021 03:03:09 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-2-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20600,"web_url":"https://patchwork.libcamera.org/comment/20600/","msgid":"","date":"2021-10-28T04:38:14","subject":"Re: [libcamera-devel] [RFC PATCH 1/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 Laurent,\n\nOn Thu, Oct 28, 2021 at 9:03 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Oct 20, 2021 at 01:01:46PM +0900, Hirokazu Honda wrote:\n> > On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart wrote:\n> > > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:\n> > > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using\n> > > > cros::CameraBufferManager on ChromeOS and gralloc on non\n> > > > ChromeOS platform. The allocated FrameBuffer(s) are owned by\n> > > > PlatformFrameBufferAllocator an destroyed when\n> > >\n> > > s/an/and/\n> > >\n> > > > PlatformFrameBufferAllocator is destroyed.\n> > >\n> > > It would also be useful to explain in the commit message why this is\n> > > needed.\n> > >\n> > > Overall the patch looks pretty good. I think we're reaching a point\n> > > where we'll have to document Android classes such as this one though,\n> > > but it doesn't have to be part of this series.\n> > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/frame_buffer.h | 53 +++++++++++\n> > > > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > > > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > > > src/android/mm/meson.build | 6 +-\n> > > > 4 files changed, 258 insertions(+), 2 deletions(-)\n> > > > create mode 100644 src/android/frame_buffer.h\n> > > > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > > > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> > > >\n> > > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h\n> > > > new file mode 100644\n> > > > index 00000000..6aafeaf3\n> > > > --- /dev/null\n> > > > +++ b/src/android/frame_buffer.h\n> > > > @@ -0,0 +1,53 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * frame_buffer.h - Frame buffer allocating interface definition\n> > >\n> > > s/allocating/allocator/\n> > >\n> > > > + */\n> > > > +#ifndef __ANDROID_FRAME_BUFFER_H__\n> > > > +#define __ANDROID_FRAME_BUFFER_H__\n> > > > +\n> > > > +#include \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> > > > + const std::vector> &\n> > > > + allocate(int halPixelFormat,\n> > > > + const libcamera::Size &size,\n> > > > + uint32_t usage,\n> > > > + size_t numBuffers);\n> > > > +};\n> > > > +\n> > > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION \\\n> > >\n> > > I'm not a big fan of this design pattern, but it already exists in\n> > > camera_buffer.h, so we can keep it for now and address both later.\n> > >\n> > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \\\n> > > > + CameraDevice *const cameraDevice) \\\n> > > > + : Extensible(std::make_unique(this, cameraDevice)) \\\n> > > > +{ \\\n> > > > +} \\\n> > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \\\n> > > > +{ \\\n> > > > +} \\\n> > > > +const std::vector>& \\\n> > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \\\n> > > > + const libcamera::Size& size, \\\n> > > > + uint32_t usage, \\\n> > > > + size_t numBuffers) \\\n> > > > +{ \\\n> > > > + return _d()->allocate(halPixelFormat, size, usage, numBuffers); \\\n> > > > +}\n> > > > +\n> > > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */\n> > > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp\n> > > > new file mode 100644\n> > > > index 00000000..114c739b\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/cros_frame_buffer.cpp\n> > > > @@ -0,0 +1,85 @@\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 buffer backend\n> > >\n> > > s/allocate/Allocate/\n> > >\n> > > > + * using CameraBufferManager\n> > > > + */\n> > > > +\n> > > > +#include \"../frame_buffer.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"cros-camera/camera_buffer_manager.h\"\n> > > > +\n> > > > +#include \"../camera_device.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(HAL)\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> > > > + const std::vector> &\n> > > > + allocate(int halPixelFormat,\n> > > > + const libcamera::Size &size,\n> > > > + uint32_t usage,\n> > > > + size_t numBuffers);\n> >\n> > I think to return std::unique_ptr so that we\n> > don't have to consider proper numBuffers.\n>\n> Allocating buffers individually sounds good, I agree with you. Ideally\n> we should have the same API for the FrameBufferAllocator, but we can\n> consolidate the APIs later (FrameBufferAllocator will always have the\n> limitation that buffers can only be allocated between configure() and\n> start(), but hopefully in the future we will be able to drop this V4L2\n> allocation backend and use dmabuf heaps).\n>\n> > It is necessary to attach something libcamera::FrameBuffer, for\n> > example, cros::ScopedBufferHandle in cros_frame_buffer.\n> > However, libcamera::FrameBuffer is marked final. How should I solve\n> > this problem?\n> > Is it fine to remove the final mark?\n>\n> FrameBuffer is an Extensible class, so you can subclass\n> FrameBuffer::Private and add your custom data there without subclassing\n> FrameBuffer. We do that with the Camera class already, it's declared as\n> final, but pipeline handlers subclass Camera::Private.\n>\n\nThanks. I didn't notice that. Seems like I am still not get used to\nthe D-pointer design.\n\n-Hiro\n\n> > > > +\n> > > > +private:\n> > > > + std::vector> allocatedBuffers_;\n> > > > + std::vector allocatedHandles_;\n> > > > +};\n> > > > +\n> > > > +const std::vector> &\n> > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > > + size_t numBuffers)\n> > > > +{\n> > > > + ASSERT(allocatedBuffers_.empty());\n> > > > +\n> > > > + std::vector> buffers;\n> > > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > > + cros::ScopedBufferHandle handle =\n> > > > + cros::CameraBufferManager::AllocateScopedBuffer(\n> > > > + size.width, size.height, halPixelFormat, usage);\n> > > > + if (!handle) {\n> > > > + LOG(HAL, Error) << \"Failed allocate buffer_handle\";\n> > >\n> > > s/Failed/Failed to/\n> > >\n> > > > + return allocatedBuffers_;\n> > >\n> > > I think\n> > >\n> > > return {};\n> > >\n> > > would be more explicit. For a moment I thought the function would return\n> > > a partial vector of some of the allocations succeeded. Same below.\n> > >\n> > > > + }\n> > > > +\n> > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);\n> > > > + std::vector planes(numPlanes);\n> > > > + for (size_t j = 0; j < numPlanes; ++j) {\n> > >\n> > > This could also be written\n> > >\n> > > for (auto [j, plane] : utils::enumerate(planes)) {\n> > >\n> > > Up to you.\n> > >\n> > > > + FileDescriptor fd{ (*handle)->data[0] };\n> > > > + if (!fd.isValid()) {\n> > > > + LOG(HAL, Fatal) << \"Invalid fd\";\n> > > > + return allocatedBuffers_;\n> > > > + }\n> > >\n> > > You can move this outside of the loop. FileDescriptor uses implicit\n> > > sharing of the actual fd, so all instances of planes[j].fd will use the\n> > > same numerical fd, avoiding the dup() call and saving file descriptor\n> > > space.\n> > >\n> > > > +\n> > > > + planes[j].fd = fd;\n> > > > + planes[j].offset =\n> > > > + cros::CameraBufferManager::GetPlaneOffset(*handle, j);\n> > > > + planes[j].length =\n> > > > + cros::CameraBufferManager::GetPlaneSize(*handle, j);\n> > > > + }\n> > > > +\n> > > > + buffers.push_back(std::make_unique(planes));\n> > > > + allocatedHandles_.push_back(std::move(handle));\n> > > > + }\n> > > > +\n> > > > + allocatedBuffers_ = std::move(buffers);\n> > > > + return allocatedBuffers_;\n> > > > +}\n> > > > +\n> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp\n> > > > new file mode 100644\n> > > > index 00000000..b387d5a2\n> > > > --- /dev/null\n> > > > +++ b/src/android/mm/generic_frame_buffer.cpp\n> > > > @@ -0,0 +1,116 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame\n> > > > + * buffer backend\n> > > > + */\n> > > > +\n> > > > +#include \"../frame_buffer.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"../camera_device.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(HAL)\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> > >\n> > > allocDevice_ should be initialized to nullptr or the destructor could\n> > > end up calling gralloc_close() on a random pointer.\n> > >\n> > > > + {\n> > > > + ASSERT(!!hardwareModule_);\n> > >\n> > > Why not\n> > >\n> > > ASSERT(hardwareModule_);\n> > >\n> > > ?\n> > >\n> > > > + }\n> > > > +\n> > > > + ~Private() override;\n> > > > +\n> > > > + const std::vector> &\n> > > > + allocate(int halPixelFormat,\n> > > > + const libcamera::Size &size,\n> > > > + uint32_t usage,\n> > > > + size_t numBuffers);\n> > > > +\n> > > > +private:\n> > > > + const CameraDevice *const cameraDevice_;\n> > > > + struct hw_module_t *const hardwareModule_;\n> > > > + struct alloc_device_t *allocDevice_;\n> > > > +\n> > > > + std::vector> allocatedBuffers_;\n> > > > + std::vector bufferHandles_;\n> > > > +};\n> > > > +\n> > > > +PlatformFrameBufferAllocator::Private::~Private()\n> > > > +{\n> > > > + for (buffer_handle_t &handle : bufferHandles_) {\n> > > > + ASSERT(allocDevice_);\n> > >\n> > > I would drop the assert, this really can't happen.\n> > >\n> > > > + allocDevice_->free(allocDevice_, handle);\n> > > > + }\n> > >\n> > > A blank line would be nice.\n> > >\n> > > > + if (allocDevice_)\n> > > > + gralloc_close(allocDevice_);\n> > > > +}\n> > > > +\n> > > > +const std::vector> &\n> > > > +PlatformFrameBufferAllocator::Private::allocate(\n> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,\n> > > > + size_t numBuffers)\n> > > > +{\n> > > > + ASSERT(allocatedBuffers_.empty());\n> > > > + ASSERT(bufferHandles_.empty());\n> > > > + ASSERT(!allocDevice_);\n> > > > +\n> > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);\n> > > > + if (ret) {\n> > > > + LOG(HAL, Error) << \"gralloc_open() failed: \" << ret;\n> > > > + return allocatedBuffers_;\n> > >\n> > > Same here,\n> > >\n> > > return {};\n> > >\n> > > > + }\n> > > > +\n> > > > + int stride = 0;\n> > > > + for (size_t i = 0; i < numBuffers; ++i) {\n> > > > + buffer_handle_t handle{};\n> > > > + ret = allocDevice_->alloc(allocDevice_, size.width, size.height,\n> > > > + halPixelFormat, usage, &handle, &stride);\n> > >\n> > > I suppose the stride is guaranteed to be the same for all allocations ?\n> > >\n> > > > + if (ret) {\n> > > > + LOG(HAL, Error) << \"failed buffer allocating: \" << ret;\n> > > > + return allocatedBuffers_;\n> > > > + }\n> > > > +\n> > > > + bufferHandles_.push_back(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> > > > +\n> > > > + allocatedBuffers_.reserve(numBuffers);\n> > > > + for (const buffer_handle_t &handle : bufferHandles_) {\n> > > > + std::vector planes(numPlanes);\n> > > > + size_t offset = 0;\n> > > > + for (size_t i = 0; i < numPlanes; ++i) {\n> > >\n> > > Here too you could use utils::enumerate().\n> > >\n> > > > + planes[i].fd = FileDescriptor(handle->data[i]);\n> > > > + size_t planeSize = info.planeSize(size.height, i, stride);\n> > > > +\n> > > > + planes[i].offset = offset;\n> > >\n> > > Does Android use a different dmabuf fd for each plane ? If not, I think\n> > > the offset should be 0, and if it does, we could move the FileDescriptor\n> > > construction outside of the loop.\n> > >\n> > > > + planes[i].length = planeSize;\n> > > > + offset += planeSize;\n> > > > + }\n> > > > +\n> > > > + allocatedBuffers_.push_back(std::make_unique(planes));\n> > > > + }\n> > > > +\n> > > > + return allocatedBuffers_;\n> > > > +}\n> > > > +\n> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> > > > index eeb5cc2e..d1ad64d9 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.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.cpp'])\n> > > > android_deps += [dependency('libcros_camera')]\n> > > > endif\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 A6463BDB1C\n\tfor ;\n\tThu, 28 Oct 2021 04:38:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF9D4600B6;\n\tThu, 28 Oct 2021 06:38:27 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F139600B1\n\tfor ;\n\tThu, 28 Oct 2021 06:38:25 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id g10so19189386edj.1\n\tfor ;\n\tWed, 27 Oct 2021 21:38:25 -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=\"RW5o2iVy\"; 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=xUu2hASf7uwYxVZfTqFgeekqsMUAM59Zeob6Mkp7pZE=;\n\tb=RW5o2iVyQVRtaEu28oYD++xfReITMfYi7qIYShN4MjXspBkt8ofbM5Y2q4LEIIocfZ\n\tqEW5GW1oals2Dz0bjKB1D2Q9bQJYMro4KmsO810Ta06+QGR5q5ohpsjqmaIq0fQB3eCV\n\tSKlYQNJLah0rxkDHLEVnj5Q7T7ntzwg8GblVQ=","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=xUu2hASf7uwYxVZfTqFgeekqsMUAM59Zeob6Mkp7pZE=;\n\tb=cmXZwG2IDXqeKHg05KRY09osayz9P0qgLLvjAWiSliPRmxFrxilaQ0jd9AcWsnIoDD\n\tO+6Aihi7Eh/gCpGFcyjDFiXWr6dETaPG+Z8Bsytz/EL3B48Cu2ppSqlSE5k3Fevatmzu\n\tJRr4YtgNCFDaCrWuuGzEV+3xjW+aibrm77yRhHljk6L+WSgqejTjjIwwf/ofRXj+2iTS\n\tROIleUh6ozvQBuY+gBmG+zIEFuLdK9a6ECwhO6BwHNNOKqurosMWtUyR1Ey573xWt/vv\n\tWA0/HQ9Ux47Ty0xsKbBlGOfebi7MKPVWsMe1saM8RTT075K4llgryl74NGU6FVV5S34d\n\tjZDg==","X-Gm-Message-State":"AOAM532wxLtooAuxFdhX8Xr2NgW2GJpUS0FhiLzkQs7HGY26wEFtCPvS\n\tEOLTb7COD6c++9A55f5Z40D5RVe5tpwc6lkQYqBGWw==","X-Google-Smtp-Source":"ABdhPJwxiA7MWUDEuEg0xML5A894BT7nPgJinZ2QcgYfu75nXeDVa481/YY+6ufL6qQmE9R6cvBg5hdVX0t5UwenDEU=","X-Received":"by 2002:a17:906:1456:: with SMTP id\n\tq22mr2291913ejc.291.1635395904851; \n\tWed, 27 Oct 2021 21:38:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-2-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 28 Oct 2021 13:38:14 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]