[libcamera-devel] [PATCH 2/2] android: Introduce PlatformFrameBufferAllocator
Hirokazu Honda
hiroh at chromium.org
Mon Nov 1 06:09:04 CET 2021
HI Jacopo, thank you for reviewing.
On Sat, Oct 30, 2021 at 12:08 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> On Thu, Oct 28, 2021 at 04:30:38PM +0900, Hirokazu Honda wrote:
> > After we unify identical streams to one stream configuration, the
> > capture requested by a HAL client can have been resolved as
> > CameraStream::Mapped. That is, a buffer to be written by a camera
> > is not provided by a client. We would handle this case by
> > dynamically allocating FrameBuffer.
> >
> > The existing FrameBufferAllocator cannot used because it is not
> > allowed to allocate a new buffer while Camera is running.
>
> That's a nice context but seems slightly unrelated to this patch :)
> >
> > This CL introduces PlatformFrameBufferAllocator. It allocates
>
> After years, I still don't know what CL means
>
Oops.. CL stands for change list. It is Google idom. Removed.
https://stackoverflow.com/questions/25716920/what-does-cl-mean-in-a-commit-message-what-does-it-stand-for
> > 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>
> > ---
> > src/android/frame_buffer_allocator.h | 55 +++++++
> > .../mm/cros_frame_buffer_allocator.cpp | 88 +++++++++++
> > .../mm/generic_frame_buffer_allocator.cpp | 140 ++++++++++++++++++
> > src/android/mm/meson.build | 6 +-
> > 4 files changed, 287 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..e46a43b5
> > --- /dev/null
> > +++ b/src/android/frame_buffer_allocator.h
> > @@ -0,0 +1,55 @@
> > +/* 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.
> > + */
>
> This applies to both backends I assume
>
Actually this restriction doesn't apply cros_frame_buffer_allocator.
cros::ScopedBufferHandle will be destroyed by using the leaky
cros::CameraBufferManager instance.
See https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=593;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e
> > + 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() \
> > +{ \
> > +} \
> > + \
>
> No empty line if you have none between the other declarations
>
> > +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..9d7e3c88
> > --- /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)
>
> empty line
>
> > +public:
> > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)
> > + : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))
> > + {
> > + }
> > +
> > + ~CrosFrameBufferData() override = default;
>
> empty line
>
> > +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)
> > + {
> > + }
> > +
> > + ~Private() = default;
> > +
> > + 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;
> > + }
> > +
>
> Also this assumes all planes are contigous, right ?
>
> > + 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..1bc3c405
> > --- /dev/null
> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> > @@ -0,0 +1,140 @@
> > +/* 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)
>
> empty line
>
> > +public:
> > + GenericFrameBufferData(struct alloc_device_t *allocDevice,
> > + buffer_handle_t handle)
> > + : allocDevice_(allocDevice), handle_(handle)
> > + {
> > + ASSERT(allocDevice_);
> > + ASSERT(handle_);
> > + }
>
> In the other implementation you had an empty line here
>
> > + ~GenericFrameBufferData()
> > + {
> > + /*
> > + * 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.
>
> In the meantime I would be tempted to ASSERT(allocDevice_) to catch
> potential issues early.
>
> > + *
> > + * Q: Thread safety against alloc_device_t is not documented.
> > + * Is it no problem to call alloc/free in parallel?
> > + */
> > + allocDevice_->free(allocDevice_, handle_);
> > + }
>
> empty line
>
> > +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);
>
> override ?
We don't need override because allocate() is not Extensible::Private function.
>
> > +
> > +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, Error) << "gralloc_open() failed: " << ret;
>
> Should this be Fatal ?
>
> > + 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;
> > + }
>
> I don't know the gralloc API but can (!ret && !handle)
>
I would like to check independently. Or this can be ASSERT(!handle).
> > +
> > + const libcamera::PixelFormat pixelFormat =
> > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > + const auto &info = PixelFormatInfo::info(pixelFormat);
> > + const unsigned int numPlanes = info.numPlanes();
> > + std::vector<FrameBuffer::Plane> planes(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'])
>
> Very nice overall!
>
> Before merging, do you think it would be possible to replace the usage
> of FrameBufferAllocator in CameraStream with this new API and run CTS
> on it ?
>
Sure. I will do.
-Hiro
> Thanks
> j
>
> > android_deps += [dependency('libcros_camera')]
> > endif
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
More information about the libcamera-devel
mailing list