| Message ID | 20211123183947.46839-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > The existing FrameBufferAllocator is not allowed to allocate a > new buffer while Camera is running. This introduces > PlatformFrameBufferAllocator. It allocates FrameBuffer using > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > platform. The allocated FrameBuffer owns the underlying buffer > but must be destroyed before PlatformFrameBufferAllocator is > destroyed. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/frame_buffer_allocator.h | 54 +++++++ > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > src/android/mm/meson.build | 6 +- > 4 files changed, 288 insertions(+), 2 deletions(-) > create mode 100644 src/android/frame_buffer_allocator.h > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > new file mode 100644 > index 00000000..41ed4ae1 > --- /dev/null > +++ b/src/android/frame_buffer_allocator.h > @@ -0,0 +1,54 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > + * platform dependent way. > + */ > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > + > +#include <memory> > + > +#include <libcamera/base/class.h> checkstyle should have suggested you a separate group ? It didn't for me recently but I got told so :) > +#include <libcamera/camera.h> > +#include <libcamera/framebuffer.h> > +#include <libcamera/geometry.h> > + > +class CameraDevice; > + > +class PlatformFrameBufferAllocator : libcamera::Extensible > +{ > + LIBCAMERA_DECLARE_PRIVATE() > + > +public: > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > + ~PlatformFrameBufferAllocator(); > + > + /* > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > + * Note: The returned FrameBuffer needs to be destroyed before > + * PlatformFrameBufferAllocator is destroyed. > + */ > + std::unique_ptr<libcamera::FrameBuffer> allocate( > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > +}; > + > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > + CameraDevice *const cameraDevice) \ > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > +{ \ > +} \ > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > +{ \ > +} \ > +std::unique_ptr<libcamera::FrameBuffer> \ > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > + const libcamera::Size& size, \ > + uint32_t usage) \ > +{ \ > + return _d()->allocate(halPixelFormat, size, usage); \ > +} > + > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > new file mode 100644 > index 00000000..d6343e53 > --- /dev/null > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > @@ -0,0 +1,88 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > + * CameraBufferManager > + */ > + > +#include <memory> > +#include <vector> > + > +#include <libcamera/base/log.h> > + > +#include "libcamera/internal/framebuffer.h" > + > +#include "../camera_device.h" > +#include "../frame_buffer_allocator.h" Blank line ? > +#include "cros-camera/camera_buffer_manager.h" > + > +using namespace libcamera; > + > +LOG_DECLARE_CATEGORY(HAL) > + > +namespace { Blank line > +class CrosFrameBufferData : public FrameBuffer::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) Blank line > +public: > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > + { > + } > + > + ~CrosFrameBufferData() override = default; Does the compiler complains if you don't force the destructor to be generated ? > + > +private: > + cros::ScopedBufferHandle scopedHandle_; > +}; Blank line > +} /* namespace */ > + > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > + > +public: > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, allocator is not used anymore in any of the two subclasses ? Can it be dropped ? > + [[maybe_unused]] CameraDevice *const cameraDevice) > + { > + } > + > + std::unique_ptr<libcamera::FrameBuffer> allocate( > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > +}; > + > +std::unique_ptr<libcamera::FrameBuffer> > +PlatformFrameBufferAllocator::Private::allocate( > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > +{ > + cros::ScopedBufferHandle scopedHandle = > + cros::CameraBufferManager::AllocateScopedBuffer( > + size.width, size.height, halPixelFormat, usage); > + if (!scopedHandle) { > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > + return nullptr; > + } > + > + buffer_handle_t handle = *scopedHandle; > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > + std::vector<FrameBuffer::Plane> planes(numPlanes); > + FileDescriptor fd{ handle->data[0] }; afaict this duplicates the filedescriptor. If the cros:CameraBufferManager closes its copy on destruction, it's fine. > + if (!fd.isValid()) { > + LOG(HAL, Fatal) << "Invalid fd"; > + return nullptr; > + } > + > + /* This code assumes all the planes are located in the same buffer. */ > + for (auto [i, plane] : utils::enumerate(planes)) { > + plane.fd = fd; > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > + } > + > + return std::make_unique<FrameBuffer>( > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), DO you need to move scopedHandle ? You have a single constructor that will bind to && and & if I'm not mistaken. With minors and the file descriptor duplication clarified Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + planes); > +} > + > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > new file mode 100644 > index 00000000..f00fd704 > --- /dev/null > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > @@ -0,0 +1,142 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > + */ > + > +#include <memory> > +#include <vector> > + > +#include <libcamera/base/log.h> > + > +#include "libcamera/internal/formats.h" > +#include "libcamera/internal/framebuffer.h" > + > +#include <hardware/camera3.h> > +#include <hardware/gralloc.h> > +#include <hardware/hardware.h> > + > +#include "../camera_device.h" > +#include "../frame_buffer_allocator.h" > + > +using namespace libcamera; > + > +LOG_DECLARE_CATEGORY(HAL) > + > +namespace { > +class GenericFrameBufferData : public FrameBuffer::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > + > +public: > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > + buffer_handle_t handle) > + : allocDevice_(allocDevice), handle_(handle) > + { > + ASSERT(allocDevice_); > + ASSERT(handle_); > + } > + > + ~GenericFrameBufferData() override > + { > + /* > + * allocDevice_ is used to destroy handle_. allocDevice_ is > + * owned by PlatformFrameBufferAllocator::Private. > + * GenericFrameBufferData must be destroyed before it is > + * destroyed. > + * > + * \todo Consider managing alloc_device_t with std::shared_ptr > + * if this is difficult to maintain. > + * > + * \todo Thread safety against alloc_device_t is not documented. > + * Is it no problem to call alloc/free in parallel? > + */ > + allocDevice_->free(allocDevice_, handle_); > + } > + > +private: > + struct alloc_device_t *allocDevice_; > + const buffer_handle_t handle_; > +}; > +} /* namespace */ > + > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > +{ > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > + > +public: > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > + CameraDevice *const cameraDevice) > + : cameraDevice_(cameraDevice), > + hardwareModule_(cameraDevice->camera3Device()->common.module), > + allocDevice_(nullptr) > + { > + ASSERT(hardwareModule_); > + } > + > + ~Private() override; > + > + std::unique_ptr<libcamera::FrameBuffer> allocate( > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > + > +private: > + const CameraDevice *const cameraDevice_; > + struct hw_module_t *const hardwareModule_; > + struct alloc_device_t *allocDevice_; > +}; > + > +PlatformFrameBufferAllocator::Private::~Private() > +{ > + if (allocDevice_) > + gralloc_close(allocDevice_); > +} > + > +std::unique_ptr<libcamera::FrameBuffer> > +PlatformFrameBufferAllocator::Private::allocate( > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > +{ > + if (!allocDevice_) { > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > + if (ret) { > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > + return nullptr; > + } > + } > + > + int stride = 0; > + buffer_handle_t handle = nullptr; > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > + halPixelFormat, usage, &handle, &stride); > + if (ret) { > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > + return nullptr; > + } > + if (!handle) { > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > + return nullptr; > + } > + > + const libcamera::PixelFormat pixelFormat = > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > + const auto &info = PixelFormatInfo::info(pixelFormat); > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > + > + /* This code assumes the planes are mapped consecutively. */ > + FileDescriptor fd{ handle->data[0] }; > + size_t offset = 0; > + for (auto [i, plane] : utils::enumerate(planes)) { > + const size_t planeSize = info.planeSize(size.height, i, stride); > + > + planes[i].fd = fd; > + planes[i].offset = offset; > + planes[i].length = planeSize; > + offset += planeSize; > + } > + > + return std::make_unique<FrameBuffer>( > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > + planes); > +} > + > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > index eeb5cc2e..d40a3b0b 100644 > --- a/src/android/mm/meson.build > +++ b/src/android/mm/meson.build > @@ -2,8 +2,10 @@ > > platform = get_option('android_platform') > if platform == 'generic' > - android_hal_sources += files(['generic_camera_buffer.cpp']) > + android_hal_sources += files(['generic_camera_buffer.cpp', > + 'generic_frame_buffer_allocator.cpp']) > elif platform == 'cros' > - android_hal_sources += files(['cros_camera_buffer.cpp']) > + android_hal_sources += files(['cros_camera_buffer.cpp', > + 'cros_frame_buffer_allocator.cpp']) > android_deps += [dependency('libcros_camera')] > endif > -- > 2.34.0.rc2.393.gf8c9666880-goog >
Hi Jacopo, thank you for reviewing. On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > The existing FrameBufferAllocator is not allowed to allocate a > > new buffer while Camera is running. This introduces > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > platform. The allocated FrameBuffer owns the underlying buffer > > but must be destroyed before PlatformFrameBufferAllocator is > > destroyed. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/android/frame_buffer_allocator.h | 54 +++++++ > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > src/android/mm/meson.build | 6 +- > > 4 files changed, 288 insertions(+), 2 deletions(-) > > create mode 100644 src/android/frame_buffer_allocator.h > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > new file mode 100644 > > index 00000000..41ed4ae1 > > --- /dev/null > > +++ b/src/android/frame_buffer_allocator.h > > @@ -0,0 +1,54 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > + * platform dependent way. > > + */ > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > + > > +#include <memory> > > + > > +#include <libcamera/base/class.h> > > checkstyle should have suggested you a separate group ? > It didn't for me recently but I got told so :) > I don't get any error. > > +#include <libcamera/camera.h> > > +#include <libcamera/framebuffer.h> > > +#include <libcamera/geometry.h> > > + > > +class CameraDevice; > > + > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > +{ > > + LIBCAMERA_DECLARE_PRIVATE() > > + > > +public: > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > + ~PlatformFrameBufferAllocator(); > > + > > + /* > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > + * Note: The returned FrameBuffer needs to be destroyed before > > + * PlatformFrameBufferAllocator is destroyed. > > + */ > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > +}; > > + > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > + CameraDevice *const cameraDevice) \ > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > +{ \ > > +} \ > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > +{ \ > > +} \ > > +std::unique_ptr<libcamera::FrameBuffer> \ > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > + const libcamera::Size& size, \ > > + uint32_t usage) \ > > +{ \ > > + return _d()->allocate(halPixelFormat, size, usage); \ > > +} > > + > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > new file mode 100644 > > index 00000000..d6343e53 > > --- /dev/null > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > @@ -0,0 +1,88 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > + * CameraBufferManager > > + */ > > + > > +#include <memory> > > +#include <vector> > > + > > +#include <libcamera/base/log.h> > > + > > +#include "libcamera/internal/framebuffer.h" > > + > > +#include "../camera_device.h" > > +#include "../frame_buffer_allocator.h" > > Blank line ? > > > +#include "cros-camera/camera_buffer_manager.h" > > + > > +using namespace libcamera; > > + > > +LOG_DECLARE_CATEGORY(HAL) > > + > > +namespace { > > Blank line > > > +class CrosFrameBufferData : public FrameBuffer::Private > > +{ > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > Blank line > > > +public: > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > + { > > + } > > + > > + ~CrosFrameBufferData() override = default; > > Does the compiler complains if you don't force the destructor to be > generated ? > No. Omitted. > > + > > +private: > > + cros::ScopedBufferHandle scopedHandle_; > > +}; > > Blank line > > > +} /* namespace */ > > + > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > +{ > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > + > > +public: > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > allocator is not used anymore in any of the two subclasses ? Can it be > dropped ? > I think having PlatformAllocator in argument is required for PFBAllocator::Private class constructor. > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > + { > > + } > > + > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > +}; > > + > > +std::unique_ptr<libcamera::FrameBuffer> > > +PlatformFrameBufferAllocator::Private::allocate( > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > +{ > > + cros::ScopedBufferHandle scopedHandle = > > + cros::CameraBufferManager::AllocateScopedBuffer( > > + size.width, size.height, halPixelFormat, usage); > > + if (!scopedHandle) { > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > + return nullptr; > > + } > > + > > + buffer_handle_t handle = *scopedHandle; > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > + FileDescriptor fd{ handle->data[0] }; > > afaict this duplicates the filedescriptor. If the > cros:CameraBufferManager closes its copy on destruction, it's fine. > Yes, correct. > > + if (!fd.isValid()) { > > + LOG(HAL, Fatal) << "Invalid fd"; > > + return nullptr; > > + } > > + > > + /* This code assumes all the planes are located in the same buffer. */ > > + for (auto [i, plane] : utils::enumerate(planes)) { > > + plane.fd = fd; > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > + } > > + > > + return std::make_unique<FrameBuffer>( > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > DO you need to move scopedHandle ? You have a single constructor that > will bind to && and & if I'm not mistaken. > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. Google c++ coding style restricts using && in move constructor only. I think since CrosFrameBufferData is different from cros::ScopedBufferHandle this is not move constructor. -Hiro > With minors and the file descriptor duplication clarified > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > + planes); > > +} > > + > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > new file mode 100644 > > index 00000000..f00fd704 > > --- /dev/null > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > @@ -0,0 +1,142 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > + */ > > + > > +#include <memory> > > +#include <vector> > > + > > +#include <libcamera/base/log.h> > > + > > +#include "libcamera/internal/formats.h" > > +#include "libcamera/internal/framebuffer.h" > > + > > +#include <hardware/camera3.h> > > +#include <hardware/gralloc.h> > > +#include <hardware/hardware.h> > > + > > +#include "../camera_device.h" > > +#include "../frame_buffer_allocator.h" > > + > > +using namespace libcamera; > > + > > +LOG_DECLARE_CATEGORY(HAL) > > + > > +namespace { > > +class GenericFrameBufferData : public FrameBuffer::Private > > +{ > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > + > > +public: > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > + buffer_handle_t handle) > > + : allocDevice_(allocDevice), handle_(handle) > > + { > > + ASSERT(allocDevice_); > > + ASSERT(handle_); > > + } > > + > > + ~GenericFrameBufferData() override > > + { > > + /* > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > + * owned by PlatformFrameBufferAllocator::Private. > > + * GenericFrameBufferData must be destroyed before it is > > + * destroyed. > > + * > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > + * if this is difficult to maintain. > > + * > > + * \todo Thread safety against alloc_device_t is not documented. > > + * Is it no problem to call alloc/free in parallel? > > + */ > > + allocDevice_->free(allocDevice_, handle_); > > + } > > + > > +private: > > + struct alloc_device_t *allocDevice_; > > + const buffer_handle_t handle_; > > +}; > > +} /* namespace */ > > + > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > +{ > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > + > > +public: > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > + CameraDevice *const cameraDevice) > > + : cameraDevice_(cameraDevice), > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > + allocDevice_(nullptr) > > + { > > + ASSERT(hardwareModule_); > > + } > > + > > + ~Private() override; > > + > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > + > > +private: > > + const CameraDevice *const cameraDevice_; > > + struct hw_module_t *const hardwareModule_; > > + struct alloc_device_t *allocDevice_; > > +}; > > + > > +PlatformFrameBufferAllocator::Private::~Private() > > +{ > > + if (allocDevice_) > > + gralloc_close(allocDevice_); > > +} > > + > > +std::unique_ptr<libcamera::FrameBuffer> > > +PlatformFrameBufferAllocator::Private::allocate( > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > +{ > > + if (!allocDevice_) { > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > + if (ret) { > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > + return nullptr; > > + } > > + } > > + > > + int stride = 0; > > + buffer_handle_t handle = nullptr; > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > + halPixelFormat, usage, &handle, &stride); > > + if (ret) { > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > + return nullptr; > > + } > > + if (!handle) { > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > + return nullptr; > > + } > > + > > + const libcamera::PixelFormat pixelFormat = > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > + > > + /* This code assumes the planes are mapped consecutively. */ > > + FileDescriptor fd{ handle->data[0] }; > > + size_t offset = 0; > > + for (auto [i, plane] : utils::enumerate(planes)) { > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > + > > + planes[i].fd = fd; > > + planes[i].offset = offset; > > + planes[i].length = planeSize; > > + offset += planeSize; > > + } > > + > > + return std::make_unique<FrameBuffer>( > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > + planes); > > +} > > + > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > index eeb5cc2e..d40a3b0b 100644 > > --- a/src/android/mm/meson.build > > +++ b/src/android/mm/meson.build > > @@ -2,8 +2,10 @@ > > > > platform = get_option('android_platform') > > if platform == 'generic' > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > + 'generic_frame_buffer_allocator.cpp']) > > elif platform == 'cros' > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > + 'cros_frame_buffer_allocator.cpp']) > > android_deps += [dependency('libcros_camera')] > > endif > > -- > > 2.34.0.rc2.393.gf8c9666880-goog > >
On Fri, Nov 26, 2021 at 12:36 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > Hi Jacopo, thank you for reviewing. > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro, > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > > The existing FrameBufferAllocator is not allowed to allocate a > > > new buffer while Camera is running. This introduces > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > > platform. The allocated FrameBuffer owns the underlying buffer > > > but must be destroyed before PlatformFrameBufferAllocator is > > > destroyed. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > > src/android/mm/meson.build | 6 +- > > > 4 files changed, 288 insertions(+), 2 deletions(-) > > > create mode 100644 src/android/frame_buffer_allocator.h > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > > new file mode 100644 > > > index 00000000..41ed4ae1 > > > --- /dev/null > > > +++ b/src/android/frame_buffer_allocator.h > > > @@ -0,0 +1,54 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > > + * platform dependent way. > > > + */ > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > + > > > +#include <memory> > > > + > > > +#include <libcamera/base/class.h> > > > > checkstyle should have suggested you a separate group ? > > It didn't for me recently but I got told so :) > > > > I don't get any error. > > > > +#include <libcamera/camera.h> > > > +#include <libcamera/framebuffer.h> > > > +#include <libcamera/geometry.h> > > > + > > > +class CameraDevice; > > > + > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > +{ > > > + LIBCAMERA_DECLARE_PRIVATE() > > > + > > > +public: > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > > + ~PlatformFrameBufferAllocator(); > > > + > > > + /* > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > > + * Note: The returned FrameBuffer needs to be destroyed before > > > + * PlatformFrameBufferAllocator is destroyed. > > > + */ > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > +}; > > > + > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > > + CameraDevice *const cameraDevice) \ > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > > +{ \ > > > +} \ > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > > +{ \ > > > +} \ > > > +std::unique_ptr<libcamera::FrameBuffer> \ > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > > + const libcamera::Size& size, \ > > > + uint32_t usage) \ > > > +{ \ > > > + return _d()->allocate(halPixelFormat, size, usage); \ > > > +} > > > + > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > > new file mode 100644 > > > index 00000000..d6343e53 > > > --- /dev/null > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > @@ -0,0 +1,88 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > > + * CameraBufferManager > > > + */ > > > + > > > +#include <memory> > > > +#include <vector> > > > + > > > +#include <libcamera/base/log.h> > > > + > > > +#include "libcamera/internal/framebuffer.h" > > > + > > > +#include "../camera_device.h" > > > +#include "../frame_buffer_allocator.h" > > > > Blank line ? Oh, checkstyle warns if I add blank line here. > > > > > +#include "cros-camera/camera_buffer_manager.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DECLARE_CATEGORY(HAL) > > > + > > > +namespace { > > > > Blank line > > > > > +class CrosFrameBufferData : public FrameBuffer::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > Blank line > > > > > +public: > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > + { > > > + } > > > + > > > + ~CrosFrameBufferData() override = default; > > > > Does the compiler complains if you don't force the destructor to be > > generated ? > > > > No. Omitted. > > > + > > > +private: > > > + cros::ScopedBufferHandle scopedHandle_; > > > +}; > > > > Blank line > > > > > +} /* namespace */ > > > + > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > + > > > +public: > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > allocator is not used anymore in any of the two subclasses ? Can it be > > dropped ? > > > > I think having PlatformAllocator in argument is required for > PFBAllocator::Private class constructor. > > > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > > + { > > > + } > > > + > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > +}; > > > + > > > +std::unique_ptr<libcamera::FrameBuffer> > > > +PlatformFrameBufferAllocator::Private::allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > +{ > > > + cros::ScopedBufferHandle scopedHandle = > > > + cros::CameraBufferManager::AllocateScopedBuffer( > > > + size.width, size.height, halPixelFormat, usage); > > > + if (!scopedHandle) { > > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > > + return nullptr; > > > + } > > > + > > > + buffer_handle_t handle = *scopedHandle; > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > > + FileDescriptor fd{ handle->data[0] }; > > > > afaict this duplicates the filedescriptor. If the > > cros:CameraBufferManager closes its copy on destruction, it's fine. > > > > Yes, correct. > > > > + if (!fd.isValid()) { > > > + LOG(HAL, Fatal) << "Invalid fd"; > > > + return nullptr; > > > + } > > > + > > > + /* This code assumes all the planes are located in the same buffer. */ > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > + plane.fd = fd; > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > > + } > > > + > > > + return std::make_unique<FrameBuffer>( > > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > > DO you need to move scopedHandle ? You have a single constructor that > > will bind to && and & if I'm not mistaken. > > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. > Google c++ coding style restricts using && in move constructor only. > I think since CrosFrameBufferData is different from > cros::ScopedBufferHandle this is not move constructor. > > -Hiro > > > With minors and the file descriptor duplication clarified > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Thanks > > j > > > > > + planes); > > > +} > > > + > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > > new file mode 100644 > > > index 00000000..f00fd704 > > > --- /dev/null > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > @@ -0,0 +1,142 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > > + */ > > > + > > > +#include <memory> > > > +#include <vector> > > > + > > > +#include <libcamera/base/log.h> > > > + > > > +#include "libcamera/internal/formats.h" > > > +#include "libcamera/internal/framebuffer.h" > > > + > > > +#include <hardware/camera3.h> > > > +#include <hardware/gralloc.h> > > > +#include <hardware/hardware.h> > > > + > > > +#include "../camera_device.h" > > > +#include "../frame_buffer_allocator.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DECLARE_CATEGORY(HAL) > > > + > > > +namespace { > > > +class GenericFrameBufferData : public FrameBuffer::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > + > > > +public: > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > > + buffer_handle_t handle) > > > + : allocDevice_(allocDevice), handle_(handle) > > > + { > > > + ASSERT(allocDevice_); > > > + ASSERT(handle_); > > > + } > > > + > > > + ~GenericFrameBufferData() override > > > + { > > > + /* > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > > + * owned by PlatformFrameBufferAllocator::Private. > > > + * GenericFrameBufferData must be destroyed before it is > > > + * destroyed. > > > + * > > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > > + * if this is difficult to maintain. > > > + * > > > + * \todo Thread safety against alloc_device_t is not documented. > > > + * Is it no problem to call alloc/free in parallel? > > > + */ > > > + allocDevice_->free(allocDevice_, handle_); > > > + } > > > + > > > +private: > > > + struct alloc_device_t *allocDevice_; > > > + const buffer_handle_t handle_; > > > +}; > > > +} /* namespace */ > > > + > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > + > > > +public: > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > + CameraDevice *const cameraDevice) > > > + : cameraDevice_(cameraDevice), > > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > > + allocDevice_(nullptr) > > > + { > > > + ASSERT(hardwareModule_); > > > + } > > > + > > > + ~Private() override; > > > + > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > + > > > +private: > > > + const CameraDevice *const cameraDevice_; > > > + struct hw_module_t *const hardwareModule_; > > > + struct alloc_device_t *allocDevice_; > > > +}; > > > + > > > +PlatformFrameBufferAllocator::Private::~Private() > > > +{ > > > + if (allocDevice_) > > > + gralloc_close(allocDevice_); > > > +} > > > + > > > +std::unique_ptr<libcamera::FrameBuffer> > > > +PlatformFrameBufferAllocator::Private::allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > +{ > > > + if (!allocDevice_) { > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > > + if (ret) { > > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > > + return nullptr; > > > + } > > > + } > > > + > > > + int stride = 0; > > > + buffer_handle_t handle = nullptr; > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > > + halPixelFormat, usage, &handle, &stride); > > > + if (ret) { > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > > + return nullptr; > > > + } > > > + if (!handle) { > > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > > + return nullptr; > > > + } > > > + > > > + const libcamera::PixelFormat pixelFormat = > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > > + > > > + /* This code assumes the planes are mapped consecutively. */ > > > + FileDescriptor fd{ handle->data[0] }; > > > + size_t offset = 0; > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > > + > > > + planes[i].fd = fd; > > > + planes[i].offset = offset; > > > + planes[i].length = planeSize; > > > + offset += planeSize; > > > + } > > > + > > > + return std::make_unique<FrameBuffer>( > > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > > + planes); > > > +} > > > + > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > > index eeb5cc2e..d40a3b0b 100644 > > > --- a/src/android/mm/meson.build > > > +++ b/src/android/mm/meson.build > > > @@ -2,8 +2,10 @@ > > > > > > platform = get_option('android_platform') > > > if platform == 'generic' > > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > > + 'generic_frame_buffer_allocator.cpp']) > > > elif platform == 'cros' > > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > > + 'cros_frame_buffer_allocator.cpp']) > > > android_deps += [dependency('libcros_camera')] > > > endif > > > -- > > > 2.34.0.rc2.393.gf8c9666880-goog > > >
Hi Hiro On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for reviewing. > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro, > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > > The existing FrameBufferAllocator is not allowed to allocate a > > > new buffer while Camera is running. This introduces > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > > platform. The allocated FrameBuffer owns the underlying buffer > > > but must be destroyed before PlatformFrameBufferAllocator is > > > destroyed. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > > src/android/mm/meson.build | 6 +- > > > 4 files changed, 288 insertions(+), 2 deletions(-) > > > create mode 100644 src/android/frame_buffer_allocator.h > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > > new file mode 100644 > > > index 00000000..41ed4ae1 > > > --- /dev/null > > > +++ b/src/android/frame_buffer_allocator.h > > > @@ -0,0 +1,54 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > > + * platform dependent way. > > > + */ > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > + > > > +#include <memory> > > > + > > > +#include <libcamera/base/class.h> > > > > checkstyle should have suggested you a separate group ? > > It didn't for me recently but I got told so :) > > > > I don't get any error. > > > > +#include <libcamera/camera.h> > > > +#include <libcamera/framebuffer.h> > > > +#include <libcamera/geometry.h> > > > + > > > +class CameraDevice; > > > + > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > +{ > > > + LIBCAMERA_DECLARE_PRIVATE() > > > + > > > +public: > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > > + ~PlatformFrameBufferAllocator(); > > > + > > > + /* > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > > + * Note: The returned FrameBuffer needs to be destroyed before > > > + * PlatformFrameBufferAllocator is destroyed. > > > + */ > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > +}; > > > + > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > > + CameraDevice *const cameraDevice) \ > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > > +{ \ > > > +} \ > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > > +{ \ > > > +} \ > > > +std::unique_ptr<libcamera::FrameBuffer> \ > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > > + const libcamera::Size& size, \ > > > + uint32_t usage) \ > > > +{ \ > > > + return _d()->allocate(halPixelFormat, size, usage); \ > > > +} > > > + > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > > new file mode 100644 > > > index 00000000..d6343e53 > > > --- /dev/null > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > @@ -0,0 +1,88 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > > + * CameraBufferManager > > > + */ > > > + > > > +#include <memory> > > > +#include <vector> > > > + > > > +#include <libcamera/base/log.h> > > > + > > > +#include "libcamera/internal/framebuffer.h" > > > + > > > +#include "../camera_device.h" > > > +#include "../frame_buffer_allocator.h" > > > > Blank line ? > > > > > +#include "cros-camera/camera_buffer_manager.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DECLARE_CATEGORY(HAL) > > > + > > > +namespace { > > > > Blank line > > > > > +class CrosFrameBufferData : public FrameBuffer::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > Blank line > > > > > +public: > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > + { > > > + } > > > + > > > + ~CrosFrameBufferData() override = default; > > > > Does the compiler complains if you don't force the destructor to be > > generated ? > > > > No. Omitted. > > > + > > > +private: > > > + cros::ScopedBufferHandle scopedHandle_; > > > +}; > > > > Blank line > > > > > +} /* namespace */ > > > + > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > + > > > +public: > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > allocator is not used anymore in any of the two subclasses ? Can it be > > dropped ? > > > > I think having PlatformAllocator in argument is required for > PFBAllocator::Private class constructor. > > > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > > + { > > > + } > > > + > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > +}; > > > + > > > +std::unique_ptr<libcamera::FrameBuffer> > > > +PlatformFrameBufferAllocator::Private::allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > +{ > > > + cros::ScopedBufferHandle scopedHandle = > > > + cros::CameraBufferManager::AllocateScopedBuffer( > > > + size.width, size.height, halPixelFormat, usage); > > > + if (!scopedHandle) { > > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > > + return nullptr; > > > + } > > > + > > > + buffer_handle_t handle = *scopedHandle; > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > > + FileDescriptor fd{ handle->data[0] }; > > > > afaict this duplicates the filedescriptor. If the > > cros:CameraBufferManager closes its copy on destruction, it's fine. > > > > Yes, correct. > > > > + if (!fd.isValid()) { > > > + LOG(HAL, Fatal) << "Invalid fd"; > > > + return nullptr; > > > + } > > > + > > > + /* This code assumes all the planes are located in the same buffer. */ > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > + plane.fd = fd; > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > > + } > > > + > > > + return std::make_unique<FrameBuffer>( > > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > > DO you need to move scopedHandle ? You have a single constructor that > > will bind to && and & if I'm not mistaken. > > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. > Google c++ coding style restricts using && in move constructor only. > I think since CrosFrameBufferData is different from > cros::ScopedBufferHandle this is not move constructor. Ok but why constructing a new ScopedBufferHandle in the CrosFrameBufferData constructor ? You currently have CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) { } return std::make_unique<FrameBuffer>( std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), planes); While you could CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle) : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) { } return std::make_unique<FrameBuffer>( std::make_unique<CrosFrameBufferData>(scopedHandle), planes); With your ack I can apply these changes. Also, I wanted to have the allocator being used to replace FrameBufferAllocator in CameraStream before merging so it can be CTS tested. I'll do so and send a new version if you're not working on doing so. Thanks j > > -Hiro > > > With minors and the file descriptor duplication clarified > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Thanks > > j > > > > > + planes); > > > +} > > > + > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > > new file mode 100644 > > > index 00000000..f00fd704 > > > --- /dev/null > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > @@ -0,0 +1,142 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > > + */ > > > + > > > +#include <memory> > > > +#include <vector> > > > + > > > +#include <libcamera/base/log.h> > > > + > > > +#include "libcamera/internal/formats.h" > > > +#include "libcamera/internal/framebuffer.h" > > > + > > > +#include <hardware/camera3.h> > > > +#include <hardware/gralloc.h> > > > +#include <hardware/hardware.h> > > > + > > > +#include "../camera_device.h" > > > +#include "../frame_buffer_allocator.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DECLARE_CATEGORY(HAL) > > > + > > > +namespace { > > > +class GenericFrameBufferData : public FrameBuffer::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > + > > > +public: > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > > + buffer_handle_t handle) > > > + : allocDevice_(allocDevice), handle_(handle) > > > + { > > > + ASSERT(allocDevice_); > > > + ASSERT(handle_); > > > + } > > > + > > > + ~GenericFrameBufferData() override > > > + { > > > + /* > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > > + * owned by PlatformFrameBufferAllocator::Private. > > > + * GenericFrameBufferData must be destroyed before it is > > > + * destroyed. > > > + * > > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > > + * if this is difficult to maintain. > > > + * > > > + * \todo Thread safety against alloc_device_t is not documented. > > > + * Is it no problem to call alloc/free in parallel? > > > + */ > > > + allocDevice_->free(allocDevice_, handle_); > > > + } > > > + > > > +private: > > > + struct alloc_device_t *allocDevice_; > > > + const buffer_handle_t handle_; > > > +}; > > > +} /* namespace */ > > > + > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > +{ > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > + > > > +public: > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > + CameraDevice *const cameraDevice) > > > + : cameraDevice_(cameraDevice), > > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > > + allocDevice_(nullptr) > > > + { > > > + ASSERT(hardwareModule_); > > > + } > > > + > > > + ~Private() override; > > > + > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > + > > > +private: > > > + const CameraDevice *const cameraDevice_; > > > + struct hw_module_t *const hardwareModule_; > > > + struct alloc_device_t *allocDevice_; > > > +}; > > > + > > > +PlatformFrameBufferAllocator::Private::~Private() > > > +{ > > > + if (allocDevice_) > > > + gralloc_close(allocDevice_); > > > +} > > > + > > > +std::unique_ptr<libcamera::FrameBuffer> > > > +PlatformFrameBufferAllocator::Private::allocate( > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > +{ > > > + if (!allocDevice_) { > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > > + if (ret) { > > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > > + return nullptr; > > > + } > > > + } > > > + > > > + int stride = 0; > > > + buffer_handle_t handle = nullptr; > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > > + halPixelFormat, usage, &handle, &stride); > > > + if (ret) { > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > > + return nullptr; > > > + } > > > + if (!handle) { > > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > > + return nullptr; > > > + } > > > + > > > + const libcamera::PixelFormat pixelFormat = > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > > + > > > + /* This code assumes the planes are mapped consecutively. */ > > > + FileDescriptor fd{ handle->data[0] }; > > > + size_t offset = 0; > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > > + > > > + planes[i].fd = fd; > > > + planes[i].offset = offset; > > > + planes[i].length = planeSize; > > > + offset += planeSize; > > > + } > > > + > > > + return std::make_unique<FrameBuffer>( > > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > > + planes); > > > +} > > > + > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > > index eeb5cc2e..d40a3b0b 100644 > > > --- a/src/android/mm/meson.build > > > +++ b/src/android/mm/meson.build > > > @@ -2,8 +2,10 @@ > > > > > > platform = get_option('android_platform') > > > if platform == 'generic' > > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > > + 'generic_frame_buffer_allocator.cpp']) > > > elif platform == 'cros' > > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > > + 'cros_frame_buffer_allocator.cpp']) > > > android_deps += [dependency('libcros_camera')] > > > endif > > > -- > > > 2.34.0.rc2.393.gf8c9666880-goog > > >
Hi Jacopo, On Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro > > On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for reviewing. > > > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Hiro, > > > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > > > The existing FrameBufferAllocator is not allowed to allocate a > > > > new buffer while Camera is running. This introduces > > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > > > platform. The allocated FrameBuffer owns the underlying buffer > > > > but must be destroyed before PlatformFrameBufferAllocator is > > > > destroyed. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > > > src/android/mm/meson.build | 6 +- > > > > 4 files changed, 288 insertions(+), 2 deletions(-) > > > > create mode 100644 src/android/frame_buffer_allocator.h > > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > > > new file mode 100644 > > > > index 00000000..41ed4ae1 > > > > --- /dev/null > > > > +++ b/src/android/frame_buffer_allocator.h > > > > @@ -0,0 +1,54 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2021, Google Inc. > > > > + * > > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > > > + * platform dependent way. > > > > + */ > > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > + > > > > +#include <memory> > > > > + > > > > +#include <libcamera/base/class.h> > > > > > > checkstyle should have suggested you a separate group ? > > > It didn't for me recently but I got told so :) > > > > > > > I don't get any error. > > > > > > +#include <libcamera/camera.h> > > > > +#include <libcamera/framebuffer.h> > > > > +#include <libcamera/geometry.h> > > > > + > > > > +class CameraDevice; > > > > + > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > +{ > > > > + LIBCAMERA_DECLARE_PRIVATE() > > > > + > > > > +public: > > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > > > + ~PlatformFrameBufferAllocator(); > > > > + > > > > + /* > > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > > > + * Note: The returned FrameBuffer needs to be destroyed before > > > > + * PlatformFrameBufferAllocator is destroyed. > > > > + */ > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > +}; > > > > + > > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > > > + CameraDevice *const cameraDevice) \ > > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > > > +{ \ > > > > +} \ > > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > > > +{ \ > > > > +} \ > > > > +std::unique_ptr<libcamera::FrameBuffer> \ > > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > > > + const libcamera::Size& size, \ > > > > + uint32_t usage) \ > > > > +{ \ > > > > + return _d()->allocate(halPixelFormat, size, usage); \ > > > > +} > > > > + > > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > new file mode 100644 > > > > index 00000000..d6343e53 > > > > --- /dev/null > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > @@ -0,0 +1,88 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2021, Google Inc. > > > > + * > > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > > > + * CameraBufferManager > > > > + */ > > > > + > > > > +#include <memory> > > > > +#include <vector> > > > > + > > > > +#include <libcamera/base/log.h> > > > > + > > > > +#include "libcamera/internal/framebuffer.h" > > > > + > > > > +#include "../camera_device.h" > > > > +#include "../frame_buffer_allocator.h" > > > > > > Blank line ? > > > > > > > +#include "cros-camera/camera_buffer_manager.h" > > > > + > > > > +using namespace libcamera; > > > > + > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > + > > > > +namespace { > > > > > > Blank line > > > > > > > +class CrosFrameBufferData : public FrameBuffer::Private > > > > +{ > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > > Blank line > > > > > > > +public: > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > > + { > > > > + } > > > > + > > > > + ~CrosFrameBufferData() override = default; > > > > > > Does the compiler complains if you don't force the destructor to be > > > generated ? > > > > > > > No. Omitted. > > > > + > > > > +private: > > > > + cros::ScopedBufferHandle scopedHandle_; > > > > +}; > > > > > > Blank line > > > > > > > +} /* namespace */ > > > > + > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > +{ > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > + > > > > +public: > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > > > allocator is not used anymore in any of the two subclasses ? Can it be > > > dropped ? > > > > > > > I think having PlatformAllocator in argument is required for > > PFBAllocator::Private class constructor. > > > > > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > > > + { > > > > + } > > > > + > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > +}; > > > > + > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > +{ > > > > + cros::ScopedBufferHandle scopedHandle = > > > > + cros::CameraBufferManager::AllocateScopedBuffer( > > > > + size.width, size.height, halPixelFormat, usage); > > > > + if (!scopedHandle) { > > > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > > > + return nullptr; > > > > + } > > > > + > > > > + buffer_handle_t handle = *scopedHandle; > > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > > > + FileDescriptor fd{ handle->data[0] }; > > > > > > afaict this duplicates the filedescriptor. If the > > > cros:CameraBufferManager closes its copy on destruction, it's fine. > > > > > > > Yes, correct. > > > > > > + if (!fd.isValid()) { > > > > + LOG(HAL, Fatal) << "Invalid fd"; > > > > + return nullptr; > > > > + } > > > > + > > > > + /* This code assumes all the planes are located in the same buffer. */ > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > + plane.fd = fd; > > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > > > + } > > > > + > > > > + return std::make_unique<FrameBuffer>( > > > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > > > > DO you need to move scopedHandle ? You have a single constructor that > > > will bind to && and & if I'm not mistaken. > > > > > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. > > Google c++ coding style restricts using && in move constructor only. > > I think since CrosFrameBufferData is different from > > cros::ScopedBufferHandle this is not move constructor. > > Ok but why constructing a new ScopedBufferHandle in the > CrosFrameBufferData constructor ? > > You currently have > > CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > { > } > > return std::make_unique<FrameBuffer>( > std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > planes); > > While you could > > CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle) > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > { > } > > return std::make_unique<FrameBuffer>( > std::make_unique<CrosFrameBufferData>(scopedHandle), planes); > Hm, I think it is invalid or at least not recommended way to execute std::move() against reference value. If you would like to do that, I would do CrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle). Yes, it doesn't follow Google C++ style guide, but I am fine in this tiny piece of code. > With your ack I can apply these changes. > > Also, I wanted to have the allocator being used to replace > FrameBufferAllocator in CameraStream before merging so it can be CTS > tested. I'll do so and send a new version if you're not working on > doing so. I confirmed replacing it with this allocator works in a few CTS cases. However, there is one small issue. I will send you my local patch. -Hiro > > Thanks > j > > > > > -Hiro > > > > > With minors and the file descriptor duplication clarified > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Thanks > > > j > > > > > > > + planes); > > > > +} > > > > + > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > new file mode 100644 > > > > index 00000000..f00fd704 > > > > --- /dev/null > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > @@ -0,0 +1,142 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2021, Google Inc. > > > > + * > > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > > > + */ > > > > + > > > > +#include <memory> > > > > +#include <vector> > > > > + > > > > +#include <libcamera/base/log.h> > > > > + > > > > +#include "libcamera/internal/formats.h" > > > > +#include "libcamera/internal/framebuffer.h" > > > > + > > > > +#include <hardware/camera3.h> > > > > +#include <hardware/gralloc.h> > > > > +#include <hardware/hardware.h> > > > > + > > > > +#include "../camera_device.h" > > > > +#include "../frame_buffer_allocator.h" > > > > + > > > > +using namespace libcamera; > > > > + > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > + > > > > +namespace { > > > > +class GenericFrameBufferData : public FrameBuffer::Private > > > > +{ > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > + > > > > +public: > > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > > > + buffer_handle_t handle) > > > > + : allocDevice_(allocDevice), handle_(handle) > > > > + { > > > > + ASSERT(allocDevice_); > > > > + ASSERT(handle_); > > > > + } > > > > + > > > > + ~GenericFrameBufferData() override > > > > + { > > > > + /* > > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > > > + * owned by PlatformFrameBufferAllocator::Private. > > > > + * GenericFrameBufferData must be destroyed before it is > > > > + * destroyed. > > > > + * > > > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > > > + * if this is difficult to maintain. > > > > + * > > > > + * \todo Thread safety against alloc_device_t is not documented. > > > > + * Is it no problem to call alloc/free in parallel? > > > > + */ > > > > + allocDevice_->free(allocDevice_, handle_); > > > > + } > > > > + > > > > +private: > > > > + struct alloc_device_t *allocDevice_; > > > > + const buffer_handle_t handle_; > > > > +}; > > > > +} /* namespace */ > > > > + > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > +{ > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > + > > > > +public: > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > + CameraDevice *const cameraDevice) > > > > + : cameraDevice_(cameraDevice), > > > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > > > + allocDevice_(nullptr) > > > > + { > > > > + ASSERT(hardwareModule_); > > > > + } > > > > + > > > > + ~Private() override; > > > > + > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > + > > > > +private: > > > > + const CameraDevice *const cameraDevice_; > > > > + struct hw_module_t *const hardwareModule_; > > > > + struct alloc_device_t *allocDevice_; > > > > +}; > > > > + > > > > +PlatformFrameBufferAllocator::Private::~Private() > > > > +{ > > > > + if (allocDevice_) > > > > + gralloc_close(allocDevice_); > > > > +} > > > > + > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > +{ > > > > + if (!allocDevice_) { > > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > > > + if (ret) { > > > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > > > + return nullptr; > > > > + } > > > > + } > > > > + > > > > + int stride = 0; > > > > + buffer_handle_t handle = nullptr; > > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > > > + halPixelFormat, usage, &handle, &stride); > > > > + if (ret) { > > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > > > + return nullptr; > > > > + } > > > > + if (!handle) { > > > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > > > + return nullptr; > > > > + } > > > > + > > > > + const libcamera::PixelFormat pixelFormat = > > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > > > + > > > > + /* This code assumes the planes are mapped consecutively. */ > > > > + FileDescriptor fd{ handle->data[0] }; > > > > + size_t offset = 0; > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > > > + > > > > + planes[i].fd = fd; > > > > + planes[i].offset = offset; > > > > + planes[i].length = planeSize; > > > > + offset += planeSize; > > > > + } > > > > + > > > > + return std::make_unique<FrameBuffer>( > > > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > > > + planes); > > > > +} > > > > + > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > > > index eeb5cc2e..d40a3b0b 100644 > > > > --- a/src/android/mm/meson.build > > > > +++ b/src/android/mm/meson.build > > > > @@ -2,8 +2,10 @@ > > > > > > > > platform = get_option('android_platform') > > > > if platform == 'generic' > > > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > > > + 'generic_frame_buffer_allocator.cpp']) > > > > elif platform == 'cros' > > > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > > > + 'cros_frame_buffer_allocator.cpp']) > > > > android_deps += [dependency('libcros_camera')] > > > > endif > > > > -- > > > > 2.34.0.rc2.393.gf8c9666880-goog > > > >
Hi Hiro On Fri, Nov 26, 2021 at 06:35:43PM +0900, Hirokazu Honda wrote: > Hi Jacopo, > > On Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro > > > > On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote: > > > Hi Jacopo, thank you for reviewing. > > > > > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi Hiro, > > > > > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > > > > The existing FrameBufferAllocator is not allowed to allocate a > > > > > new buffer while Camera is running. This introduces > > > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > > > > platform. The allocated FrameBuffer owns the underlying buffer > > > > > but must be destroyed before PlatformFrameBufferAllocator is > > > > > destroyed. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > > > > src/android/mm/meson.build | 6 +- > > > > > 4 files changed, 288 insertions(+), 2 deletions(-) > > > > > create mode 100644 src/android/frame_buffer_allocator.h > > > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > > > > new file mode 100644 > > > > > index 00000000..41ed4ae1 > > > > > --- /dev/null > > > > > +++ b/src/android/frame_buffer_allocator.h > > > > > @@ -0,0 +1,54 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2021, Google Inc. > > > > > + * > > > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > > > > + * platform dependent way. > > > > > + */ > > > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > > + > > > > > +#include <memory> > > > > > + > > > > > +#include <libcamera/base/class.h> > > > > > > > > checkstyle should have suggested you a separate group ? > > > > It didn't for me recently but I got told so :) > > > > > > > > > > I don't get any error. > > > > > > > > +#include <libcamera/camera.h> > > > > > +#include <libcamera/framebuffer.h> > > > > > +#include <libcamera/geometry.h> > > > > > + > > > > > +class CameraDevice; > > > > > + > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > > +{ > > > > > + LIBCAMERA_DECLARE_PRIVATE() > > > > > + > > > > > +public: > > > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > > > > + ~PlatformFrameBufferAllocator(); > > > > > + > > > > > + /* > > > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > > > > + * Note: The returned FrameBuffer needs to be destroyed before > > > > > + * PlatformFrameBufferAllocator is destroyed. > > > > > + */ > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > +}; > > > > > + > > > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > > > > + CameraDevice *const cameraDevice) \ > > > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > > > > +{ \ > > > > > +} \ > > > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > > > > +{ \ > > > > > +} \ > > > > > +std::unique_ptr<libcamera::FrameBuffer> \ > > > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > > > > + const libcamera::Size& size, \ > > > > > + uint32_t usage) \ > > > > > +{ \ > > > > > + return _d()->allocate(halPixelFormat, size, usage); \ > > > > > +} > > > > > + > > > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > new file mode 100644 > > > > > index 00000000..d6343e53 > > > > > --- /dev/null > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > @@ -0,0 +1,88 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2021, Google Inc. > > > > > + * > > > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > > > > + * CameraBufferManager > > > > > + */ > > > > > + > > > > > +#include <memory> > > > > > +#include <vector> > > > > > + > > > > > +#include <libcamera/base/log.h> > > > > > + > > > > > +#include "libcamera/internal/framebuffer.h" > > > > > + > > > > > +#include "../camera_device.h" > > > > > +#include "../frame_buffer_allocator.h" > > > > > > > > Blank line ? > > > > > > > > > +#include "cros-camera/camera_buffer_manager.h" > > > > > + > > > > > +using namespace libcamera; > > > > > + > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > > + > > > > > +namespace { > > > > > > > > Blank line > > > > > > > > > +class CrosFrameBufferData : public FrameBuffer::Private > > > > > +{ > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > > > > Blank line > > > > > > > > > +public: > > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > > > + { > > > > > + } > > > > > + > > > > > + ~CrosFrameBufferData() override = default; > > > > > > > > Does the compiler complains if you don't force the destructor to be > > > > generated ? > > > > > > > > > > No. Omitted. > > > > > + > > > > > +private: > > > > > + cros::ScopedBufferHandle scopedHandle_; > > > > > +}; > > > > > > > > Blank line > > > > > > > > > +} /* namespace */ > > > > > + > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > > +{ > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > > + > > > > > +public: > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > > > > > allocator is not used anymore in any of the two subclasses ? Can it be > > > > dropped ? > > > > > > > > > > I think having PlatformAllocator in argument is required for > > > PFBAllocator::Private class constructor. > > > > > > > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > > > > + { > > > > > + } > > > > > + > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > +}; > > > > > + > > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > > +{ > > > > > + cros::ScopedBufferHandle scopedHandle = > > > > > + cros::CameraBufferManager::AllocateScopedBuffer( > > > > > + size.width, size.height, halPixelFormat, usage); > > > > > + if (!scopedHandle) { > > > > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + buffer_handle_t handle = *scopedHandle; > > > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > > > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > > > > + FileDescriptor fd{ handle->data[0] }; > > > > > > > > afaict this duplicates the filedescriptor. If the > > > > cros:CameraBufferManager closes its copy on destruction, it's fine. > > > > > > > > > > Yes, correct. > > > > > > > > + if (!fd.isValid()) { > > > > > + LOG(HAL, Fatal) << "Invalid fd"; > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + /* This code assumes all the planes are located in the same buffer. */ > > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > > + plane.fd = fd; > > > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > > > > + } > > > > > + > > > > > + return std::make_unique<FrameBuffer>( > > > > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > > > > > > DO you need to move scopedHandle ? You have a single constructor that > > > > will bind to && and & if I'm not mistaken. > > > > > > > > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. > > > Google c++ coding style restricts using && in move constructor only. > > > I think since CrosFrameBufferData is different from > > > cros::ScopedBufferHandle this is not move constructor. > > > > Ok but why constructing a new ScopedBufferHandle in the > > CrosFrameBufferData constructor ? > > > > You currently have > > > > CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > { > > } > > > > return std::make_unique<FrameBuffer>( > > std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > planes); > > > > While you could > > > > CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle) > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > { > > } > > > > return std::make_unique<FrameBuffer>( > > std::make_unique<CrosFrameBufferData>(scopedHandle), planes); > > > > Hm, I think it is invalid or at least not recommended way to execute > std::move() against reference value. Uh, I didn't know! But yeah, it hides from the caller that the parameter is stolen Is this reported in the C++ style guide ? > If you would like to do that, I would do > CrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle). > Yes, it doesn't follow Google C++ style guide, but I am fine in this > tiny piece of code. a && in the CrosFrameBufferData constructor would be better Then you can keep the move in the caller > > > With your ack I can apply these changes. > > > > Also, I wanted to have the allocator being used to replace > > FrameBufferAllocator in CameraStream before merging so it can be CTS > > tested. I'll do so and send a new version if you're not working on > > doing so. > > I confirmed replacing it with this allocator works in a few CTS cases. > However, there is one small issue. I will send you my local patch. > Oh thanks, I didn't want to steal work from you :) If you're working on it please go ahead, I'll wait for it to be done before merging! Thanks j > -Hiro > > > > Thanks > > j > > > > > > > > -Hiro > > > > > > > With minors and the file descriptor duplication clarified > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Thanks > > > > j > > > > > > > > > + planes); > > > > > +} > > > > > + > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > new file mode 100644 > > > > > index 00000000..f00fd704 > > > > > --- /dev/null > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > @@ -0,0 +1,142 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2021, Google Inc. > > > > > + * > > > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > > > > + */ > > > > > + > > > > > +#include <memory> > > > > > +#include <vector> > > > > > + > > > > > +#include <libcamera/base/log.h> > > > > > + > > > > > +#include "libcamera/internal/formats.h" > > > > > +#include "libcamera/internal/framebuffer.h" > > > > > + > > > > > +#include <hardware/camera3.h> > > > > > +#include <hardware/gralloc.h> > > > > > +#include <hardware/hardware.h> > > > > > + > > > > > +#include "../camera_device.h" > > > > > +#include "../frame_buffer_allocator.h" > > > > > + > > > > > +using namespace libcamera; > > > > > + > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > > + > > > > > +namespace { > > > > > +class GenericFrameBufferData : public FrameBuffer::Private > > > > > +{ > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > + > > > > > +public: > > > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > > > > + buffer_handle_t handle) > > > > > + : allocDevice_(allocDevice), handle_(handle) > > > > > + { > > > > > + ASSERT(allocDevice_); > > > > > + ASSERT(handle_); > > > > > + } > > > > > + > > > > > + ~GenericFrameBufferData() override > > > > > + { > > > > > + /* > > > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > > > > + * owned by PlatformFrameBufferAllocator::Private. > > > > > + * GenericFrameBufferData must be destroyed before it is > > > > > + * destroyed. > > > > > + * > > > > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > > > > + * if this is difficult to maintain. > > > > > + * > > > > > + * \todo Thread safety against alloc_device_t is not documented. > > > > > + * Is it no problem to call alloc/free in parallel? > > > > > + */ > > > > > + allocDevice_->free(allocDevice_, handle_); > > > > > + } > > > > > + > > > > > +private: > > > > > + struct alloc_device_t *allocDevice_; > > > > > + const buffer_handle_t handle_; > > > > > +}; > > > > > +} /* namespace */ > > > > > + > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > > +{ > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > > + > > > > > +public: > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > > + CameraDevice *const cameraDevice) > > > > > + : cameraDevice_(cameraDevice), > > > > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > > > > + allocDevice_(nullptr) > > > > > + { > > > > > + ASSERT(hardwareModule_); > > > > > + } > > > > > + > > > > > + ~Private() override; > > > > > + > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > + > > > > > +private: > > > > > + const CameraDevice *const cameraDevice_; > > > > > + struct hw_module_t *const hardwareModule_; > > > > > + struct alloc_device_t *allocDevice_; > > > > > +}; > > > > > + > > > > > +PlatformFrameBufferAllocator::Private::~Private() > > > > > +{ > > > > > + if (allocDevice_) > > > > > + gralloc_close(allocDevice_); > > > > > +} > > > > > + > > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > > +{ > > > > > + if (!allocDevice_) { > > > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > > > > + if (ret) { > > > > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > > > > + return nullptr; > > > > > + } > > > > > + } > > > > > + > > > > > + int stride = 0; > > > > > + buffer_handle_t handle = nullptr; > > > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > > > > + halPixelFormat, usage, &handle, &stride); > > > > > + if (ret) { > > > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > > > > + return nullptr; > > > > > + } > > > > > + if (!handle) { > > > > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + const libcamera::PixelFormat pixelFormat = > > > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > > > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > > > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > > > > + > > > > > + /* This code assumes the planes are mapped consecutively. */ > > > > > + FileDescriptor fd{ handle->data[0] }; > > > > > + size_t offset = 0; > > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > > > > + > > > > > + planes[i].fd = fd; > > > > > + planes[i].offset = offset; > > > > > + planes[i].length = planeSize; > > > > > + offset += planeSize; > > > > > + } > > > > > + > > > > > + return std::make_unique<FrameBuffer>( > > > > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > > > > + planes); > > > > > +} > > > > > + > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > > > > index eeb5cc2e..d40a3b0b 100644 > > > > > --- a/src/android/mm/meson.build > > > > > +++ b/src/android/mm/meson.build > > > > > @@ -2,8 +2,10 @@ > > > > > > > > > > platform = get_option('android_platform') > > > > > if platform == 'generic' > > > > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > > > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > > > > + 'generic_frame_buffer_allocator.cpp']) > > > > > elif platform == 'cros' > > > > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > > > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > > > > + 'cros_frame_buffer_allocator.cpp']) > > > > > android_deps += [dependency('libcros_camera')] > > > > > endif > > > > > -- > > > > > 2.34.0.rc2.393.gf8c9666880-goog > > > > >
Hi Jacopo, On Fri, Nov 26, 2021 at 6:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro > > On Fri, Nov 26, 2021 at 06:35:43PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, > > > > On Fri, Nov 26, 2021 at 6:19 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Hiro > > > > > > On Fri, Nov 26, 2021 at 12:36:31PM +0900, Hirokazu Honda wrote: > > > > Hi Jacopo, thank you for reviewing. > > > > > > > > On Thu, Nov 25, 2021 at 9:09 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > Hi Hiro, > > > > > > > > > > On Wed, Nov 24, 2021 at 03:39:47AM +0900, Hirokazu Honda wrote: > > > > > > The existing FrameBufferAllocator is not allowed to allocate a > > > > > > new buffer while Camera is running. This introduces > > > > > > PlatformFrameBufferAllocator. It allocates FrameBuffer using > > > > > > cros::CameraBufferManager on ChromeOS and gralloc on non ChromeOS > > > > > > platform. The allocated FrameBuffer owns the underlying buffer > > > > > > but must be destroyed before PlatformFrameBufferAllocator is > > > > > > destroyed. > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > > > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++ > > > > > > .../mm/generic_frame_buffer_allocator.cpp | 142 ++++++++++++++++++ > > > > > > src/android/mm/meson.build | 6 +- > > > > > > 4 files changed, 288 insertions(+), 2 deletions(-) > > > > > > create mode 100644 src/android/frame_buffer_allocator.h > > > > > > create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp > > > > > > create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > > > > > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h > > > > > > new file mode 100644 > > > > > > index 00000000..41ed4ae1 > > > > > > --- /dev/null > > > > > > +++ b/src/android/frame_buffer_allocator.h > > > > > > @@ -0,0 +1,54 @@ > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > +/* > > > > > > + * Copyright (C) 2021, Google Inc. > > > > > > + * > > > > > > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in > > > > > > + * platform dependent way. > > > > > > + */ > > > > > > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > > > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ > > > > > > + > > > > > > +#include <memory> > > > > > > + > > > > > > +#include <libcamera/base/class.h> > > > > > > > > > > checkstyle should have suggested you a separate group ? > > > > > It didn't for me recently but I got told so :) > > > > > > > > > > > > > I don't get any error. > > > > > > > > > > +#include <libcamera/camera.h> > > > > > > +#include <libcamera/framebuffer.h> > > > > > > +#include <libcamera/geometry.h> > > > > > > + > > > > > > +class CameraDevice; > > > > > > + > > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > > > +{ > > > > > > + LIBCAMERA_DECLARE_PRIVATE() > > > > > > + > > > > > > +public: > > > > > > + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); > > > > > > + ~PlatformFrameBufferAllocator(); > > > > > > + > > > > > > + /* > > > > > > + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. > > > > > > + * Note: The returned FrameBuffer needs to be destroyed before > > > > > > + * PlatformFrameBufferAllocator is destroyed. > > > > > > + */ > > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > > +}; > > > > > > + > > > > > > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ > > > > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ > > > > > > + CameraDevice *const cameraDevice) \ > > > > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ > > > > > > +{ \ > > > > > > +} \ > > > > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ > > > > > > +{ \ > > > > > > +} \ > > > > > > +std::unique_ptr<libcamera::FrameBuffer> \ > > > > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ > > > > > > + const libcamera::Size& size, \ > > > > > > + uint32_t usage) \ > > > > > > +{ \ > > > > > > + return _d()->allocate(halPixelFormat, size, usage); \ > > > > > > +} > > > > > > + > > > > > > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ > > > > > > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > > new file mode 100644 > > > > > > index 00000000..d6343e53 > > > > > > --- /dev/null > > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > > @@ -0,0 +1,88 @@ > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > +/* > > > > > > + * Copyright (C) 2021, Google Inc. > > > > > > + * > > > > > > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using > > > > > > + * CameraBufferManager > > > > > > + */ > > > > > > + > > > > > > +#include <memory> > > > > > > +#include <vector> > > > > > > + > > > > > > +#include <libcamera/base/log.h> > > > > > > + > > > > > > +#include "libcamera/internal/framebuffer.h" > > > > > > + > > > > > > +#include "../camera_device.h" > > > > > > +#include "../frame_buffer_allocator.h" > > > > > > > > > > Blank line ? > > > > > > > > > > > +#include "cros-camera/camera_buffer_manager.h" > > > > > > + > > > > > > +using namespace libcamera; > > > > > > + > > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > > > + > > > > > > +namespace { > > > > > > > > > > Blank line > > > > > > > > > > > +class CrosFrameBufferData : public FrameBuffer::Private > > > > > > +{ > > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > > > > > > Blank line > > > > > > > > > > > +public: > > > > > > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > > > > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > > > > + { > > > > > > + } > > > > > > + > > > > > > + ~CrosFrameBufferData() override = default; > > > > > > > > > > Does the compiler complains if you don't force the destructor to be > > > > > generated ? > > > > > > > > > > > > > No. Omitted. > > > > > > + > > > > > > +private: > > > > > > + cros::ScopedBufferHandle scopedHandle_; > > > > > > +}; > > > > > > > > > > Blank line > > > > > > > > > > > +} /* namespace */ > > > > > > + > > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > > > +{ > > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > > > + > > > > > > +public: > > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > > > > > > > allocator is not used anymore in any of the two subclasses ? Can it be > > > > > dropped ? > > > > > > > > > > > > > I think having PlatformAllocator in argument is required for > > > > PFBAllocator::Private class constructor. > > > > > > > > > > + [[maybe_unused]] CameraDevice *const cameraDevice) > > > > > > + { > > > > > > + } > > > > > > + > > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > > +}; > > > > > > + > > > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > > > +{ > > > > > > + cros::ScopedBufferHandle scopedHandle = > > > > > > + cros::CameraBufferManager::AllocateScopedBuffer( > > > > > > + size.width, size.height, halPixelFormat, usage); > > > > > > + if (!scopedHandle) { > > > > > > + LOG(HAL, Error) << "Failed to allocate buffer handle"; > > > > > > + return nullptr; > > > > > > + } > > > > > > + > > > > > > + buffer_handle_t handle = *scopedHandle; > > > > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); > > > > > > + std::vector<FrameBuffer::Plane> planes(numPlanes); > > > > > > + FileDescriptor fd{ handle->data[0] }; > > > > > > > > > > afaict this duplicates the filedescriptor. If the > > > > > cros:CameraBufferManager closes its copy on destruction, it's fine. > > > > > > > > > > > > > Yes, correct. > > > > > > > > > > + if (!fd.isValid()) { > > > > > > + LOG(HAL, Fatal) << "Invalid fd"; > > > > > > + return nullptr; > > > > > > + } > > > > > > + > > > > > > + /* This code assumes all the planes are located in the same buffer. */ > > > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > > > + plane.fd = fd; > > > > > > + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); > > > > > > + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); > > > > > > + } > > > > > > + > > > > > > + return std::make_unique<FrameBuffer>( > > > > > > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > > > > > > > > DO you need to move scopedHandle ? You have a single constructor that > > > > > will bind to && and & if I'm not mistaken. > > > > > > > > > > > > > Yes, I have to. cros::ScopedBufferHandle is move-only constructor. > > > > Google c++ coding style restricts using && in move constructor only. > > > > I think since CrosFrameBufferData is different from > > > > cros::ScopedBufferHandle this is not move constructor. > > > > > > Ok but why constructing a new ScopedBufferHandle in the > > > CrosFrameBufferData constructor ? > > > > > > You currently have > > > > > > CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) > > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > { > > > } > > > > > > return std::make_unique<FrameBuffer>( > > > std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), > > > planes); > > > > > > While you could > > > > > > CrosFrameBufferData(cros::ScopedBufferHandle &scopedHandle) > > > : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) > > > { > > > } > > > > > > return std::make_unique<FrameBuffer>( > > > std::make_unique<CrosFrameBufferData>(scopedHandle), planes); > > > > > > > Hm, I think it is invalid or at least not recommended way to execute > > std::move() against reference value. > > Uh, I didn't know! > But yeah, it hides from the caller that the parameter is stolen > > Is this reported in the C++ style guide ? > > > If you would like to do that, I would do > > CrosFrameBufferData(cros::ScopedBufferHandle &&scopedHandle). > > Yes, it doesn't follow Google C++ style guide, but I am fine in this > > tiny piece of code. > > a && in the CrosFrameBufferData constructor would be better > Then you can keep the move in the caller > > > > > > With your ack I can apply these changes. > > > > > > Also, I wanted to have the allocator being used to replace > > > FrameBufferAllocator in CameraStream before merging so it can be CTS > > > tested. I'll do so and send a new version if you're not working on > > > doing so. > > > > I confirmed replacing it with this allocator works in a few CTS cases. > > However, there is one small issue. I will send you my local patch. > > > > Oh thanks, I didn't want to steal work from you :) > If you're working on it please go ahead, I'll wait for it to be done > before merging! > I just sent my local patch to you. Please feel free to steal if you like. :-) -Hiro > Thanks > j > > > -Hiro > > > > > > Thanks > > > j > > > > > > > > > > > -Hiro > > > > > > > > > With minors and the file descriptor duplication clarified > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > > + planes); > > > > > > +} > > > > > > + > > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > > > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > new file mode 100644 > > > > > > index 00000000..f00fd704 > > > > > > --- /dev/null > > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > @@ -0,0 +1,142 @@ > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > +/* > > > > > > + * Copyright (C) 2021, Google Inc. > > > > > > + * > > > > > > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API > > > > > > + */ > > > > > > + > > > > > > +#include <memory> > > > > > > +#include <vector> > > > > > > + > > > > > > +#include <libcamera/base/log.h> > > > > > > + > > > > > > +#include "libcamera/internal/formats.h" > > > > > > +#include "libcamera/internal/framebuffer.h" > > > > > > + > > > > > > +#include <hardware/camera3.h> > > > > > > +#include <hardware/gralloc.h> > > > > > > +#include <hardware/hardware.h> > > > > > > + > > > > > > +#include "../camera_device.h" > > > > > > +#include "../frame_buffer_allocator.h" > > > > > > + > > > > > > +using namespace libcamera; > > > > > > + > > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > > > + > > > > > > +namespace { > > > > > > +class GenericFrameBufferData : public FrameBuffer::Private > > > > > > +{ > > > > > > + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > > + > > > > > > +public: > > > > > > + GenericFrameBufferData(struct alloc_device_t *allocDevice, > > > > > > + buffer_handle_t handle) > > > > > > + : allocDevice_(allocDevice), handle_(handle) > > > > > > + { > > > > > > + ASSERT(allocDevice_); > > > > > > + ASSERT(handle_); > > > > > > + } > > > > > > + > > > > > > + ~GenericFrameBufferData() override > > > > > > + { > > > > > > + /* > > > > > > + * allocDevice_ is used to destroy handle_. allocDevice_ is > > > > > > + * owned by PlatformFrameBufferAllocator::Private. > > > > > > + * GenericFrameBufferData must be destroyed before it is > > > > > > + * destroyed. > > > > > > + * > > > > > > + * \todo Consider managing alloc_device_t with std::shared_ptr > > > > > > + * if this is difficult to maintain. > > > > > > + * > > > > > > + * \todo Thread safety against alloc_device_t is not documented. > > > > > > + * Is it no problem to call alloc/free in parallel? > > > > > > + */ > > > > > > + allocDevice_->free(allocDevice_, handle_); > > > > > > + } > > > > > > + > > > > > > +private: > > > > > > + struct alloc_device_t *allocDevice_; > > > > > > + const buffer_handle_t handle_; > > > > > > +}; > > > > > > +} /* namespace */ > > > > > > + > > > > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private > > > > > > +{ > > > > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) > > > > > > + > > > > > > +public: > > > > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, > > > > > > + CameraDevice *const cameraDevice) > > > > > > + : cameraDevice_(cameraDevice), > > > > > > + hardwareModule_(cameraDevice->camera3Device()->common.module), > > > > > > + allocDevice_(nullptr) > > > > > > + { > > > > > > + ASSERT(hardwareModule_); > > > > > > + } > > > > > > + > > > > > > + ~Private() override; > > > > > > + > > > > > > + std::unique_ptr<libcamera::FrameBuffer> allocate( > > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage); > > > > > > + > > > > > > +private: > > > > > > + const CameraDevice *const cameraDevice_; > > > > > > + struct hw_module_t *const hardwareModule_; > > > > > > + struct alloc_device_t *allocDevice_; > > > > > > +}; > > > > > > + > > > > > > +PlatformFrameBufferAllocator::Private::~Private() > > > > > > +{ > > > > > > + if (allocDevice_) > > > > > > + gralloc_close(allocDevice_); > > > > > > +} > > > > > > + > > > > > > +std::unique_ptr<libcamera::FrameBuffer> > > > > > > +PlatformFrameBufferAllocator::Private::allocate( > > > > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage) > > > > > > +{ > > > > > > + if (!allocDevice_) { > > > > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_); > > > > > > + if (ret) { > > > > > > + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; > > > > > > + return nullptr; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + int stride = 0; > > > > > > + buffer_handle_t handle = nullptr; > > > > > > + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, > > > > > > + halPixelFormat, usage, &handle, &stride); > > > > > > + if (ret) { > > > > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret; > > > > > > + return nullptr; > > > > > > + } > > > > > > + if (!handle) { > > > > > > + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; > > > > > > + return nullptr; > > > > > > + } > > > > > > + > > > > > > + const libcamera::PixelFormat pixelFormat = > > > > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); > > > > > > + const auto &info = PixelFormatInfo::info(pixelFormat); > > > > > > + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); > > > > > > + > > > > > > + /* This code assumes the planes are mapped consecutively. */ > > > > > > + FileDescriptor fd{ handle->data[0] }; > > > > > > + size_t offset = 0; > > > > > > + for (auto [i, plane] : utils::enumerate(planes)) { > > > > > > + const size_t planeSize = info.planeSize(size.height, i, stride); > > > > > > + > > > > > > + planes[i].fd = fd; > > > > > > + planes[i].offset = offset; > > > > > > + planes[i].length = planeSize; > > > > > > + offset += planeSize; > > > > > > + } > > > > > > + > > > > > > + return std::make_unique<FrameBuffer>( > > > > > > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), > > > > > > + planes); > > > > > > +} > > > > > > + > > > > > > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION > > > > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build > > > > > > index eeb5cc2e..d40a3b0b 100644 > > > > > > --- a/src/android/mm/meson.build > > > > > > +++ b/src/android/mm/meson.build > > > > > > @@ -2,8 +2,10 @@ > > > > > > > > > > > > platform = get_option('android_platform') > > > > > > if platform == 'generic' > > > > > > - android_hal_sources += files(['generic_camera_buffer.cpp']) > > > > > > + android_hal_sources += files(['generic_camera_buffer.cpp', > > > > > > + 'generic_frame_buffer_allocator.cpp']) > > > > > > elif platform == 'cros' > > > > > > - android_hal_sources += files(['cros_camera_buffer.cpp']) > > > > > > + android_hal_sources += files(['cros_camera_buffer.cpp', > > > > > > + 'cros_frame_buffer_allocator.cpp']) > > > > > > android_deps += [dependency('libcros_camera')] > > > > > > endif > > > > > > -- > > > > > > 2.34.0.rc2.393.gf8c9666880-goog > > > > > >
diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h new file mode 100644 index 00000000..41ed4ae1 --- /dev/null +++ b/src/android/frame_buffer_allocator.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in + * platform dependent way. + */ +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ + +#include <memory> + +#include <libcamera/base/class.h> +#include <libcamera/camera.h> +#include <libcamera/framebuffer.h> +#include <libcamera/geometry.h> + +class CameraDevice; + +class PlatformFrameBufferAllocator : libcamera::Extensible +{ + LIBCAMERA_DECLARE_PRIVATE() + +public: + explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice); + ~PlatformFrameBufferAllocator(); + + /* + * FrameBuffer owns the underlying buffer. Returns nullptr on failure. + * Note: The returned FrameBuffer needs to be destroyed before + * PlatformFrameBufferAllocator is destroyed. + */ + std::unique_ptr<libcamera::FrameBuffer> allocate( + int halPixelFormat, const libcamera::Size &size, uint32_t usage); +}; + +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION \ +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \ + CameraDevice *const cameraDevice) \ + : Extensible(std::make_unique<Private>(this, cameraDevice)) \ +{ \ +} \ +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \ +{ \ +} \ +std::unique_ptr<libcamera::FrameBuffer> \ +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \ + const libcamera::Size& size, \ + uint32_t usage) \ +{ \ + return _d()->allocate(halPixelFormat, size, usage); \ +} + +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */ diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp new file mode 100644 index 00000000..d6343e53 --- /dev/null +++ b/src/android/mm/cros_frame_buffer_allocator.cpp @@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using + * CameraBufferManager + */ + +#include <memory> +#include <vector> + +#include <libcamera/base/log.h> + +#include "libcamera/internal/framebuffer.h" + +#include "../camera_device.h" +#include "../frame_buffer_allocator.h" +#include "cros-camera/camera_buffer_manager.h" + +using namespace libcamera; + +LOG_DECLARE_CATEGORY(HAL) + +namespace { +class CrosFrameBufferData : public FrameBuffer::Private +{ + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) +public: + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle) + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle)) + { + } + + ~CrosFrameBufferData() override = default; + +private: + cros::ScopedBufferHandle scopedHandle_; +}; +} /* namespace */ + +class PlatformFrameBufferAllocator::Private : public Extensible::Private +{ + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) + +public: + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, + [[maybe_unused]] CameraDevice *const cameraDevice) + { + } + + std::unique_ptr<libcamera::FrameBuffer> allocate( + int halPixelFormat, const libcamera::Size &size, uint32_t usage); +}; + +std::unique_ptr<libcamera::FrameBuffer> +PlatformFrameBufferAllocator::Private::allocate( + int halPixelFormat, const libcamera::Size &size, uint32_t usage) +{ + cros::ScopedBufferHandle scopedHandle = + cros::CameraBufferManager::AllocateScopedBuffer( + size.width, size.height, halPixelFormat, usage); + if (!scopedHandle) { + LOG(HAL, Error) << "Failed to allocate buffer handle"; + return nullptr; + } + + buffer_handle_t handle = *scopedHandle; + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle); + std::vector<FrameBuffer::Plane> planes(numPlanes); + FileDescriptor fd{ handle->data[0] }; + if (!fd.isValid()) { + LOG(HAL, Fatal) << "Invalid fd"; + return nullptr; + } + + /* This code assumes all the planes are located in the same buffer. */ + for (auto [i, plane] : utils::enumerate(planes)) { + plane.fd = fd; + plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i); + plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i); + } + + return std::make_unique<FrameBuffer>( + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)), + planes); +} + +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp new file mode 100644 index 00000000..f00fd704 --- /dev/null +++ b/src/android/mm/generic_frame_buffer_allocator.cpp @@ -0,0 +1,142 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API + */ + +#include <memory> +#include <vector> + +#include <libcamera/base/log.h> + +#include "libcamera/internal/formats.h" +#include "libcamera/internal/framebuffer.h" + +#include <hardware/camera3.h> +#include <hardware/gralloc.h> +#include <hardware/hardware.h> + +#include "../camera_device.h" +#include "../frame_buffer_allocator.h" + +using namespace libcamera; + +LOG_DECLARE_CATEGORY(HAL) + +namespace { +class GenericFrameBufferData : public FrameBuffer::Private +{ + LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) + +public: + GenericFrameBufferData(struct alloc_device_t *allocDevice, + buffer_handle_t handle) + : allocDevice_(allocDevice), handle_(handle) + { + ASSERT(allocDevice_); + ASSERT(handle_); + } + + ~GenericFrameBufferData() override + { + /* + * allocDevice_ is used to destroy handle_. allocDevice_ is + * owned by PlatformFrameBufferAllocator::Private. + * GenericFrameBufferData must be destroyed before it is + * destroyed. + * + * \todo Consider managing alloc_device_t with std::shared_ptr + * if this is difficult to maintain. + * + * \todo Thread safety against alloc_device_t is not documented. + * Is it no problem to call alloc/free in parallel? + */ + allocDevice_->free(allocDevice_, handle_); + } + +private: + struct alloc_device_t *allocDevice_; + const buffer_handle_t handle_; +}; +} /* namespace */ + +class PlatformFrameBufferAllocator::Private : public Extensible::Private +{ + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator) + +public: + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator, + CameraDevice *const cameraDevice) + : cameraDevice_(cameraDevice), + hardwareModule_(cameraDevice->camera3Device()->common.module), + allocDevice_(nullptr) + { + ASSERT(hardwareModule_); + } + + ~Private() override; + + std::unique_ptr<libcamera::FrameBuffer> allocate( + int halPixelFormat, const libcamera::Size &size, uint32_t usage); + +private: + const CameraDevice *const cameraDevice_; + struct hw_module_t *const hardwareModule_; + struct alloc_device_t *allocDevice_; +}; + +PlatformFrameBufferAllocator::Private::~Private() +{ + if (allocDevice_) + gralloc_close(allocDevice_); +} + +std::unique_ptr<libcamera::FrameBuffer> +PlatformFrameBufferAllocator::Private::allocate( + int halPixelFormat, const libcamera::Size &size, uint32_t usage) +{ + if (!allocDevice_) { + int ret = gralloc_open(hardwareModule_, &allocDevice_); + if (ret) { + LOG(HAL, Fatal) << "gralloc_open() failed: " << ret; + return nullptr; + } + } + + int stride = 0; + buffer_handle_t handle = nullptr; + int ret = allocDevice_->alloc(allocDevice_, size.width, size.height, + halPixelFormat, usage, &handle, &stride); + if (ret) { + LOG(HAL, Error) << "failed buffer allocating: " << ret; + return nullptr; + } + if (!handle) { + LOG(HAL, Fatal) << "buffer_handle_t is empty on success"; + return nullptr; + } + + const libcamera::PixelFormat pixelFormat = + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat); + const auto &info = PixelFormatInfo::info(pixelFormat); + std::vector<FrameBuffer::Plane> planes(info.numPlanes()); + + /* This code assumes the planes are mapped consecutively. */ + FileDescriptor fd{ handle->data[0] }; + size_t offset = 0; + for (auto [i, plane] : utils::enumerate(planes)) { + const size_t planeSize = info.planeSize(size.height, i, stride); + + planes[i].fd = fd; + planes[i].offset = offset; + planes[i].length = planeSize; + offset += planeSize; + } + + return std::make_unique<FrameBuffer>( + std::make_unique<GenericFrameBufferData>(allocDevice_, handle), + planes); +} + +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build index eeb5cc2e..d40a3b0b 100644 --- a/src/android/mm/meson.build +++ b/src/android/mm/meson.build @@ -2,8 +2,10 @@ platform = get_option('android_platform') if platform == 'generic' - android_hal_sources += files(['generic_camera_buffer.cpp']) + android_hal_sources += files(['generic_camera_buffer.cpp', + 'generic_frame_buffer_allocator.cpp']) elif platform == 'cros' - android_hal_sources += files(['cros_camera_buffer.cpp']) + android_hal_sources += files(['cros_camera_buffer.cpp', + 'cros_frame_buffer_allocator.cpp']) android_deps += [dependency('libcros_camera')] endif