[libcamera-devel] [PATCH v3 2/2] android: Introduce PlatformFrameBufferAllocator
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 25 01:10:13 CET 2021
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 at chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at 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
>
More information about the libcamera-devel
mailing list