[libcamera-devel] [RFC PATCH 1/2] android: Introduce PlatformFrameBufferAllocator
Hirokazu Honda
hiroh at chromium.org
Thu Oct 28 06:38:14 CEST 2021
Hi Laurent,
On Thu, Oct 28, 2021 at 9:03 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Wed, Oct 20, 2021 at 01:01:46PM +0900, Hirokazu Honda wrote:
> > On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart wrote:
> > > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:
> > > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using
> > > > cros::CameraBufferManager on ChromeOS and gralloc on non
> > > > ChromeOS platform. The allocated FrameBuffer(s) are owned by
> > > > PlatformFrameBufferAllocator an destroyed when
> > >
> > > s/an/and/
> > >
> > > > PlatformFrameBufferAllocator is destroyed.
> > >
> > > It would also be useful to explain in the commit message why this is
> > > needed.
> > >
> > > Overall the patch looks pretty good. I think we're reaching a point
> > > where we'll have to document Android classes such as this one though,
> > > but it doesn't have to be part of this series.
> > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > > src/android/frame_buffer.h | 53 +++++++++++
> > > > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++
> > > > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> > > > src/android/mm/meson.build | 6 +-
> > > > 4 files changed, 258 insertions(+), 2 deletions(-)
> > > > create mode 100644 src/android/frame_buffer.h
> > > > create mode 100644 src/android/mm/cros_frame_buffer.cpp
> > > > create mode 100644 src/android/mm/generic_frame_buffer.cpp
> > > >
> > > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> > > > new file mode 100644
> > > > index 00000000..6aafeaf3
> > > > --- /dev/null
> > > > +++ b/src/android/frame_buffer.h
> > > > @@ -0,0 +1,53 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * frame_buffer.h - Frame buffer allocating interface definition
> > >
> > > s/allocating/allocator/
> > >
> > > > + */
> > > > +#ifndef __ANDROID_FRAME_BUFFER_H__
> > > > +#define __ANDROID_FRAME_BUFFER_H__
> > > > +
> > > > +#include <memory>
> > > > +#include <vector>
> > > > +
> > > > +#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();
> > > > +
> > > > + const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > + allocate(int halPixelFormat,
> > > > + const libcamera::Size &size,
> > > > + uint32_t usage,
> > > > + size_t numBuffers);
> > > > +};
> > > > +
> > > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION \
> > >
> > > I'm not a big fan of this design pattern, but it already exists in
> > > camera_buffer.h, so we can keep it for now and address both later.
> > >
> > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator( \
> > > > + CameraDevice *const cameraDevice) \
> > > > + : Extensible(std::make_unique<Private>(this, cameraDevice)) \
> > > > +{ \
> > > > +} \
> > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator() \
> > > > +{ \
> > > > +} \
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>& \
> > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat, \
> > > > + const libcamera::Size& size, \
> > > > + uint32_t usage, \
> > > > + size_t numBuffers) \
> > > > +{ \
> > > > + return _d()->allocate(halPixelFormat, size, usage, numBuffers); \
> > > > +}
> > > > +
> > > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> > > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> > > > new file mode 100644
> > > > index 00000000..114c739b
> > > > --- /dev/null
> > > > +++ b/src/android/mm/cros_frame_buffer.cpp
> > > > @@ -0,0 +1,85 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend
> > >
> > > s/allocate/Allocate/
> > >
> > > > + * using CameraBufferManager
> > > > + */
> > > > +
> > > > +#include "../frame_buffer.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include "cros-camera/camera_buffer_manager.h"
> > > > +
> > > > +#include "../camera_device.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DECLARE_CATEGORY(HAL)
> > > > +
> > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > > +{
> > > > + LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > > +
> > > > +public:
> > > > + Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > > + [[maybe_unused]] CameraDevice *const cameraDevice)
> > > > + {
> > > > + }
> > > > +
> > > > + ~Private() = default;
> > > > +
> > > > + const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > + allocate(int halPixelFormat,
> > > > + const libcamera::Size &size,
> > > > + uint32_t usage,
> > > > + size_t numBuffers);
> >
> > I think to return std::unique_ptr<libcamera::FrameBuffer> so that we
> > don't have to consider proper numBuffers.
>
> Allocating buffers individually sounds good, I agree with you. Ideally
> we should have the same API for the FrameBufferAllocator, but we can
> consolidate the APIs later (FrameBufferAllocator will always have the
> limitation that buffers can only be allocated between configure() and
> start(), but hopefully in the future we will be able to drop this V4L2
> allocation backend and use dmabuf heaps).
>
> > It is necessary to attach something libcamera::FrameBuffer, for
> > example, cros::ScopedBufferHandle in cros_frame_buffer.
> > However, libcamera::FrameBuffer is marked final. How should I solve
> > this problem?
> > Is it fine to remove the final mark?
>
> FrameBuffer is an Extensible class, so you can subclass
> FrameBuffer::Private and add your custom data there without subclassing
> FrameBuffer. We do that with the Camera class already, it's declared as
> final, but pipeline handlers subclass Camera::Private.
>
Thanks. I didn't notice that. Seems like I am still not get used to
the D-pointer design.
-Hiro
> > > > +
> > > > +private:
> > > > + std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > > + std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> > > > +};
> > > > +
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +PlatformFrameBufferAllocator::Private::allocate(
> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > > + size_t numBuffers)
> > > > +{
> > > > + ASSERT(allocatedBuffers_.empty());
> > > > +
> > > > + std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> > > > + for (size_t i = 0; i < numBuffers; ++i) {
> > > > + cros::ScopedBufferHandle handle =
> > > > + cros::CameraBufferManager::AllocateScopedBuffer(
> > > > + size.width, size.height, halPixelFormat, usage);
> > > > + if (!handle) {
> > > > + LOG(HAL, Error) << "Failed allocate buffer_handle";
> > >
> > > s/Failed/Failed to/
> > >
> > > > + return allocatedBuffers_;
> > >
> > > I think
> > >
> > > return {};
> > >
> > > would be more explicit. For a moment I thought the function would return
> > > a partial vector of some of the allocations succeeded. Same below.
> > >
> > > > + }
> > > > +
> > > > + const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> > > > + std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > > + for (size_t j = 0; j < numPlanes; ++j) {
> > >
> > > This could also be written
> > >
> > > for (auto [j, plane] : utils::enumerate(planes)) {
> > >
> > > Up to you.
> > >
> > > > + FileDescriptor fd{ (*handle)->data[0] };
> > > > + if (!fd.isValid()) {
> > > > + LOG(HAL, Fatal) << "Invalid fd";
> > > > + return allocatedBuffers_;
> > > > + }
> > >
> > > You can move this outside of the loop. FileDescriptor uses implicit
> > > sharing of the actual fd, so all instances of planes[j].fd will use the
> > > same numerical fd, avoiding the dup() call and saving file descriptor
> > > space.
> > >
> > > > +
> > > > + planes[j].fd = fd;
> > > > + planes[j].offset =
> > > > + cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> > > > + planes[j].length =
> > > > + cros::CameraBufferManager::GetPlaneSize(*handle, j);
> > > > + }
> > > > +
> > > > + buffers.push_back(std::make_unique<FrameBuffer>(planes));
> > > > + allocatedHandles_.push_back(std::move(handle));
> > > > + }
> > > > +
> > > > + allocatedBuffers_ = std::move(buffers);
> > > > + return allocatedBuffers_;
> > > > +}
> > > > +
> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> > > > new file mode 100644
> > > > index 00000000..b387d5a2
> > > > --- /dev/null
> > > > +++ b/src/android/mm/generic_frame_buffer.cpp
> > > > @@ -0,0 +1,116 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> > > > + * buffer backend
> > > > + */
> > > > +
> > > > +#include "../frame_buffer.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +#include <libcamera/internal/formats.h>
> > > > +
> > > > +#include <hardware/camera3.h>
> > > > +#include <hardware/gralloc.h>
> > > > +#include <hardware/hardware.h>
> > > > +
> > > > +#include "../camera_device.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DECLARE_CATEGORY(HAL)
> > > > +
> > > > +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_ should be initialized to nullptr or the destructor could
> > > end up calling gralloc_close() on a random pointer.
> > >
> > > > + {
> > > > + ASSERT(!!hardwareModule_);
> > >
> > > Why not
> > >
> > > ASSERT(hardwareModule_);
> > >
> > > ?
> > >
> > > > + }
> > > > +
> > > > + ~Private() override;
> > > > +
> > > > + const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > + allocate(int halPixelFormat,
> > > > + const libcamera::Size &size,
> > > > + uint32_t usage,
> > > > + size_t numBuffers);
> > > > +
> > > > +private:
> > > > + const CameraDevice *const cameraDevice_;
> > > > + struct hw_module_t *const hardwareModule_;
> > > > + struct alloc_device_t *allocDevice_;
> > > > +
> > > > + std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > > + std::vector<buffer_handle_t> bufferHandles_;
> > > > +};
> > > > +
> > > > +PlatformFrameBufferAllocator::Private::~Private()
> > > > +{
> > > > + for (buffer_handle_t &handle : bufferHandles_) {
> > > > + ASSERT(allocDevice_);
> > >
> > > I would drop the assert, this really can't happen.
> > >
> > > > + allocDevice_->free(allocDevice_, handle);
> > > > + }
> > >
> > > A blank line would be nice.
> > >
> > > > + if (allocDevice_)
> > > > + gralloc_close(allocDevice_);
> > > > +}
> > > > +
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +PlatformFrameBufferAllocator::Private::allocate(
> > > > + int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > > + size_t numBuffers)
> > > > +{
> > > > + ASSERT(allocatedBuffers_.empty());
> > > > + ASSERT(bufferHandles_.empty());
> > > > + ASSERT(!allocDevice_);
> > > > +
> > > > + int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > > > + if (ret) {
> > > > + LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> > > > + return allocatedBuffers_;
> > >
> > > Same here,
> > >
> > > return {};
> > >
> > > > + }
> > > > +
> > > > + int stride = 0;
> > > > + for (size_t i = 0; i < numBuffers; ++i) {
> > > > + buffer_handle_t handle{};
> > > > + ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > > > + halPixelFormat, usage, &handle, &stride);
> > >
> > > I suppose the stride is guaranteed to be the same for all allocations ?
> > >
> > > > + if (ret) {
> > > > + LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > > > + return allocatedBuffers_;
> > > > + }
> > > > +
> > > > + bufferHandles_.push_back(handle);
> > > > + }
> > > > +
> > > > + const libcamera::PixelFormat pixelFormat =
> > > > + cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > > > + const auto &info = PixelFormatInfo::info(pixelFormat);
> > > > + const unsigned int numPlanes = info.numPlanes();
> > > > +
> > > > + allocatedBuffers_.reserve(numBuffers);
> > > > + for (const buffer_handle_t &handle : bufferHandles_) {
> > > > + std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > > + size_t offset = 0;
> > > > + for (size_t i = 0; i < numPlanes; ++i) {
> > >
> > > Here too you could use utils::enumerate().
> > >
> > > > + planes[i].fd = FileDescriptor(handle->data[i]);
> > > > + size_t planeSize = info.planeSize(size.height, i, stride);
> > > > +
> > > > + planes[i].offset = offset;
> > >
> > > Does Android use a different dmabuf fd for each plane ? If not, I think
> > > the offset should be 0, and if it does, we could move the FileDescriptor
> > > construction outside of the loop.
> > >
> > > > + planes[i].length = planeSize;
> > > > + offset += planeSize;
> > > > + }
> > > > +
> > > > + allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> > > > + }
> > > > +
> > > > + return allocatedBuffers_;
> > > > +}
> > > > +
> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > > > index eeb5cc2e..d1ad64d9 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.cpp'])
> > > > elif platform == 'cros'
> > > > - android_hal_sources += files(['cros_camera_buffer.cpp'])
> > > > + android_hal_sources += files(['cros_camera_buffer.cpp',
> > > > + 'cros_frame_buffer.cpp'])
> > > > android_deps += [dependency('libcros_camera')]
> > > > endif
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list