[libcamera-devel] [RFC PATCH 1/2] android: Introduce PlatformFrameBufferAllocator
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 28 02:03:09 CEST 2021
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.
> > > +
> > > +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