| Message ID | 20211101071652.107912-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Hirokazu Honda (2021-11-01 07:16:52) > 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. > > 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> I'm finding this one harder to grasp. It's late so I think I might have to continue tomorrow. I wish we had better infrastructure for unit tests on the Android layer, as I'm sure some of this would benefit from more tests, but even then we'd have to have an environment to run the unit tests against the allocators... > --- > src/android/frame_buffer_allocator.h | 54 +++++++ > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > src/android/mm/meson.build | 6 +- > 4 files changed, 292 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> > +#include <libcamera/camera.h> > +#include <libcamera/framebuffer.h> > +#include <libcamera/geometry.h> > + > +class CameraDevice; > + > +class PlatformFrameBufferAllocator : libcamera::Extensible Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? Is it required somehow? Can't it just be a normal class that the CrOS and Android variants inherit from? > +{ > + 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..9c7e4ea4 > --- /dev/null > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > @@ -0,0 +1,90 @@ > +/* 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) > + { > + } > + > + ~Private() override = 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; > + } > + > + /* 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..167cc1a5 > --- /dev/null > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > @@ -0,0 +1,144 @@ > +/* 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. > + * > + * Q: Thread safety against alloc_device_t is not documented. > + * Is it no problem to call alloc/free in parallel? > + */ > + ASSERT(allocDevice_); > + 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); > + 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']) > android_deps += [dependency('libcros_camera')] > endif > -- > 2.33.1.1089.g2158813163f-goog >
Hi Kieran, On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Hirokazu Honda (2021-11-01 07:16:52) > > 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. > > > > 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> > > I'm finding this one harder to grasp. It's late so I think I might have > to continue tomorrow. > > I wish we had better infrastructure for unit tests on the Android layer, > as I'm sure some of this would benefit from more tests, but even then > we'd have to have an environment to run the unit tests against the > allocators... > > > > --- > > src/android/frame_buffer_allocator.h | 54 +++++++ > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > > src/android/mm/meson.build | 6 +- > > 4 files changed, 292 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> > > +#include <libcamera/camera.h> > > +#include <libcamera/framebuffer.h> > > +#include <libcamera/geometry.h> > > + > > +class CameraDevice; > > + > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? > Is it required somehow? Can't it just be a normal class that the CrOS > and Android variants inherit from? > I followed CameraBuffer design pattern here. I am fine to make them normal classes. How shall I decide which to be used in run time? -Hiro > > > +{ > > + 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..9c7e4ea4 > > --- /dev/null > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > @@ -0,0 +1,90 @@ > > +/* 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) > > + { > > + } > > + > > + ~Private() override = 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; > > + } > > + > > + /* 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..167cc1a5 > > --- /dev/null > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > @@ -0,0 +1,144 @@ > > +/* 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. > > + * > > + * Q: Thread safety against alloc_device_t is not documented. > > + * Is it no problem to call alloc/free in parallel? > > + */ > > + ASSERT(allocDevice_); > > + 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); > > + 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']) > > android_deps += [dependency('libcros_camera')] > > endif > > -- > > 2.33.1.1089.g2158813163f-goog > >
Hello On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote: > Hi Kieran, > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Hirokazu Honda (2021-11-01 07:16:52) > > > 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. > > > > > > 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> > > > > I'm finding this one harder to grasp. It's late so I think I might have > > to continue tomorrow. > > > > I wish we had better infrastructure for unit tests on the Android layer, > > as I'm sure some of this would benefit from more tests, but even then > > we'd have to have an environment to run the unit tests against the > > allocators... > > > > > > > --- > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > > > src/android/mm/meson.build | 6 +- > > > 4 files changed, 292 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> > > > +#include <libcamera/camera.h> > > > +#include <libcamera/framebuffer.h> > > > +#include <libcamera/geometry.h> > > > + > > > +class CameraDevice; > > > + > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? > > Is it required somehow? Can't it just be a normal class that the CrOS > > and Android variants inherit from? > > > > I followed CameraBuffer design pattern here. > I am fine to make them normal classes. > How shall I decide which to be used in run time? > I think compile time selection like it's done for CameraBuffer is better and I don't see what benefit would bring removing the Extensible pattern. Thanks j > -Hiro > > > > > +{ > > > + 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..9c7e4ea4 > > > --- /dev/null > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > @@ -0,0 +1,90 @@ > > > +/* 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) > > > + { > > > + } > > > + > > > + ~Private() override = 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; > > > + } > > > + > > > + /* 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..167cc1a5 > > > --- /dev/null > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > @@ -0,0 +1,144 @@ > > > +/* 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. > > > + * > > > + * Q: Thread safety against alloc_device_t is not documented. > > > + * Is it no problem to call alloc/free in parallel? > > > + */ > > > + ASSERT(allocDevice_); > > > + 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); > > > + 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']) > > > android_deps += [dependency('libcros_camera')] > > > endif > > > -- > > > 2.33.1.1089.g2158813163f-goog > > >
Quoting Jacopo Mondi (2021-11-09 08:57:26) > Hello > > On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote: > > Hi Kieran, > > > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Hirokazu Honda (2021-11-01 07:16:52) > > > > 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. > > > > > > > > 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> > > > > > > I'm finding this one harder to grasp. It's late so I think I might have > > > to continue tomorrow. > > > > > > I wish we had better infrastructure for unit tests on the Android layer, > > > as I'm sure some of this would benefit from more tests, but even then > > > we'd have to have an environment to run the unit tests against the > > > allocators... > > > > > > > > > > --- > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > > > > src/android/mm/meson.build | 6 +- > > > > 4 files changed, 292 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> > > > > +#include <libcamera/camera.h> > > > > +#include <libcamera/framebuffer.h> > > > > +#include <libcamera/geometry.h> > > > > + > > > > +class CameraDevice; > > > > + > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? > > > Is it required somehow? Can't it just be a normal class that the CrOS > > > and Android variants inherit from? > > > > > > > I followed CameraBuffer design pattern here. > > I am fine to make them normal classes. > > How shall I decide which to be used in run time? > > > > I think compile time selection like it's done for CameraBuffer is > better and I don't see what benefit would bring removing the Extensible > pattern. Aha, that's the part I'd missed last night. Yes this is fine for Extensible I think. > Thanks > j > > > -Hiro > > > > > > > +{ > > > > + 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..9c7e4ea4 > > > > --- /dev/null > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > @@ -0,0 +1,90 @@ > > > > +/* 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) > > > > + { > > > > + } > > > > + > > > > + ~Private() override = 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; > > > > + } > > > > + > > > > + /* 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..167cc1a5 > > > > --- /dev/null > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > @@ -0,0 +1,144 @@ > > > > +/* 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. > > > > + * > > > > + * Q: Thread safety against alloc_device_t is not documented. > > > > + * Is it no problem to call alloc/free in parallel? > > > > + */ > > > > + ASSERT(allocDevice_); > > > > + 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); > > > > + 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']) > > > > android_deps += [dependency('libcros_camera')] > > > > endif > > > > -- > > > > 2.33.1.1089.g2158813163f-goog > > > >
Quoting Kieran Bingham (2021-11-09 10:14:19) > Quoting Jacopo Mondi (2021-11-09 08:57:26) > > Hello > > > > On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote: > > > Hi Kieran, > > > > > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > Quoting Hirokazu Honda (2021-11-01 07:16:52) > > > > > 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. > > > > > > > > > > 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> > > > > > > > > I'm finding this one harder to grasp. It's late so I think I might have > > > > to continue tomorrow. > > > > > > > > I wish we had better infrastructure for unit tests on the Android layer, > > > > as I'm sure some of this would benefit from more tests, but even then > > > > we'd have to have an environment to run the unit tests against the > > > > allocators... > > > > > > > > > > > > > --- > > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > > > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > > > > > src/android/mm/meson.build | 6 +- > > > > > 4 files changed, 292 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> > > > > > +#include <libcamera/camera.h> > > > > > +#include <libcamera/framebuffer.h> > > > > > +#include <libcamera/geometry.h> > > > > > + > > > > > +class CameraDevice; > > > > > + > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > > > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? > > > > Is it required somehow? Can't it just be a normal class that the CrOS > > > > and Android variants inherit from? > > > > > > > > > > I followed CameraBuffer design pattern here. > > > I am fine to make them normal classes. > > > How shall I decide which to be used in run time? > > > > > > > I think compile time selection like it's done for CameraBuffer is > > better and I don't see what benefit would bring removing the Extensible > > pattern. > > Aha, that's the part I'd missed last night. Yes this is fine for > Extensible I think. In fact, it doesn't matter too much - but I still don't see why this needs to be Extensible. The implementation for PlatformFrameBufferAllocator can have an identically named class in both src/android/mm/cros_frame_buffer_allocator.cpp src/android/mm/generic_frame_buffer_allocator.cpp And the one that gets used will be the one that is compiled in...? > > Thanks > > j > > > > > -Hiro > > > > > > > > > +{ > > > > > + 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..9c7e4ea4 > > > > > --- /dev/null > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > @@ -0,0 +1,90 @@ > > > > > +/* 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) > > > > > + { > > > > > + } > > > > > + > > > > > + ~Private() override = 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; > > > > > + } > > > > > + > > > > > + /* 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..167cc1a5 > > > > > --- /dev/null > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > @@ -0,0 +1,144 @@ > > > > > +/* 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. > > > > > + * > > > > > + * Q: Thread safety against alloc_device_t is not documented. > > > > > + * Is it no problem to call alloc/free in parallel? > > > > > + */ > > > > > + ASSERT(allocDevice_); > > > > > + 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); > > > > > + 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']) > > > > > android_deps += [dependency('libcros_camera')] > > > > > endif > > > > > -- > > > > > 2.33.1.1089.g2158813163f-goog > > > > >
Hi Kieran, On Wed, Nov 10, 2021 at 5:31 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Kieran Bingham (2021-11-09 10:14:19) > > Quoting Jacopo Mondi (2021-11-09 08:57:26) > > > Hello > > > > > > On Tue, Nov 09, 2021 at 02:46:14PM +0900, Hirokazu Honda wrote: > > > > Hi Kieran, > > > > > > > > On Tue, Nov 9, 2021 at 8:26 AM Kieran Bingham > > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > > Quoting Hirokazu Honda (2021-11-01 07:16:52) > > > > > > 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. > > > > > > > > > > > > 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> > > > > > > > > > > I'm finding this one harder to grasp. It's late so I think I might have > > > > > to continue tomorrow. > > > > > > > > > > I wish we had better infrastructure for unit tests on the Android layer, > > > > > as I'm sure some of this would benefit from more tests, but even then > > > > > we'd have to have an environment to run the unit tests against the > > > > > allocators... > > > > > > > > > > > > > > > > --- > > > > > > src/android/frame_buffer_allocator.h | 54 +++++++ > > > > > > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > > > > > > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > > > > > > src/android/mm/meson.build | 6 +- > > > > > > 4 files changed, 292 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> > > > > > > +#include <libcamera/camera.h> > > > > > > +#include <libcamera/framebuffer.h> > > > > > > +#include <libcamera/geometry.h> > > > > > > + > > > > > > +class CameraDevice; > > > > > > + > > > > > > +class PlatformFrameBufferAllocator : libcamera::Extensible > > > > > > > > > > Does the 'PlatformFrameBufferAllocater' benefit from using Extensible? > > > > > Is it required somehow? Can't it just be a normal class that the CrOS > > > > > and Android variants inherit from? > > > > > > > > > > > > > I followed CameraBuffer design pattern here. > > > > I am fine to make them normal classes. > > > > How shall I decide which to be used in run time? > > > > > > > > > > I think compile time selection like it's done for CameraBuffer is > > > better and I don't see what benefit would bring removing the Extensible > > > pattern. > > > > Aha, that's the part I'd missed last night. Yes this is fine for > > Extensible I think. > > In fact, it doesn't matter too much - but I still don't see why this > needs to be Extensible. > > The implementation for PlatformFrameBufferAllocator can have an > identically named class in both > src/android/mm/cros_frame_buffer_allocator.cpp > src/android/mm/generic_frame_buffer_allocator.cpp > > And the one that gets used will be the one that is compiled in...? > That's a good point. I couldn't find a reason for this. If this is applicable for PlatformFrameBufferAllocator, I think we can change CameraBuffer too. -Hiro > > > > Thanks > > > j > > > > > > > -Hiro > > > > > > > > > > > +{ > > > > > > + 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..9c7e4ea4 > > > > > > --- /dev/null > > > > > > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > > > > > > @@ -0,0 +1,90 @@ > > > > > > +/* 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) > > > > > > + { > > > > > > + } > > > > > > + > > > > > > + ~Private() override = 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; > > > > > > + } > > > > > > + > > > > > > + /* 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..167cc1a5 > > > > > > --- /dev/null > > > > > > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > @@ -0,0 +1,144 @@ > > > > > > +/* 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. > > > > > > + * > > > > > > + * Q: Thread safety against alloc_device_t is not documented. > > > > > > + * Is it no problem to call alloc/free in parallel? > > > > > > + */ > > > > > > + ASSERT(allocDevice_); > > > > > > + 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); > > > > > > + 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']) > > > > > > android_deps += [dependency('libcros_camera')] > > > > > > endif > > > > > > -- > > > > > > 2.33.1.1089.g2158813163f-goog > > > > > >
Quoting Hirokazu Honda (2021-11-10 03:09:34) > Hi Kieran, > > > In fact, it doesn't matter too much - but I still don't see why this > > needs to be Extensible. > > > > The implementation for PlatformFrameBufferAllocator can have an > > identically named class in both > > src/android/mm/cros_frame_buffer_allocator.cpp > > src/android/mm/generic_frame_buffer_allocator.cpp > > > > And the one that gets used will be the one that is compiled in...? > > > > That's a good point. I couldn't find a reason for this. > If this is applicable for PlatformFrameBufferAllocator, I think we can > change CameraBuffer too. Sure, if it simplifies things. The class is not a public API, and doesn't need to be 'extensible' with a private implementation as far as I am aware. In this case, it's just that there are two possible implementations which are selected at compile time anyway. -- Kieran
On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Hirokazu Honda (2021-11-10 03:09:34) > > Hi Kieran, > > > > > In fact, it doesn't matter too much - but I still don't see why this > > > needs to be Extensible. > > > > > > The implementation for PlatformFrameBufferAllocator can have an > > > identically named class in both > > > src/android/mm/cros_frame_buffer_allocator.cpp > > > src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > And the one that gets used will be the one that is compiled in...? > > > > > > > That's a good point. I couldn't find a reason for this. > > If this is applicable for PlatformFrameBufferAllocator, I think we can > > change CameraBuffer too. > > Sure, if it simplifies things. The class is not a public API, and > doesn't need to be 'extensible' with a private implementation as far as > I am aware. In this case, it's just that there are two possible > implementations which are selected at compile time anyway. > +Laurent Pinchart , can I implement so? Why is CameraBuffer designed so? -Hiro > -- > Kieran
Hi Hiro, On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote: > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Hirokazu Honda (2021-11-10 03:09:34) > > > Hi Kieran, > > > > > > > In fact, it doesn't matter too much - but I still don't see why this > > > > needs to be Extensible. > > > > > > > > The implementation for PlatformFrameBufferAllocator can have an > > > > identically named class in both > > > > src/android/mm/cros_frame_buffer_allocator.cpp > > > > src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > And the one that gets used will be the one that is compiled in...? > > > > > > > > > > That's a good point. I couldn't find a reason for this. > > > If this is applicable for PlatformFrameBufferAllocator, I think we can > > > change CameraBuffer too. > > > > Sure, if it simplifies things. The class is not a public API, and > > doesn't need to be 'extensible' with a private implementation as far as > > I am aware. In this case, it's just that there are two possible > > implementations which are selected at compile time anyway. > > > > +Laurent Pinchart , can I implement so? > Why is CameraBuffer designed so? > At the time I designed CameraBuffer using Extensible seemed like a good idea. I had probably been lazy as there might be more opportune patterns. What I was looking for was a mechanism that - Allows to define a standard and unique interface for the CameraManager to interface with - Had a compile-time selectable implementation - Allowed to not pollute the shared interface with details of the implementation I evaluated making use of a more canonical class hierarchy, and I considered: a) A single .h file that defines the interface, multiple .cpp that implement it selected at compile time. I considered that Extensible would have left more room for implementation-specificities in the Private class interface, allowing more flexibility and wouldn't have requried implementation-specific details to surface in the interface. This mostly apply to the implementation-specific data, which Extensible allows to easy model in the Private subclass. Different .h is not an option and keeping them in sync with the common user would be painful b) A canonical class hierarchy where the 'right' sub-class would be instantiated at run time, but I ditched that as it would have required #ifdefs in the code which I would prefer not to. There surely are better solution, as your implementation shows details and requirement of a specific implementation are surfacing to the interface as well, so this can be surely improved with some more opportune design pattern. That said, I won't be too much concerned. The pattern is there and is in use, simply replacing it with a) won't bring much value, so I would say do whatever is faster for you and if we find a more elegant solution we'll change both BufferAllocator and CameraBuffer in one go. - > -Hiro > > -- > > Kieran
Hi Jacopo, On Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote: > > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Hirokazu Honda (2021-11-10 03:09:34) > > > > Hi Kieran, > > > > > > > > > In fact, it doesn't matter too much - but I still don't see why this > > > > > needs to be Extensible. > > > > > > > > > > The implementation for PlatformFrameBufferAllocator can have an > > > > > identically named class in both > > > > > src/android/mm/cros_frame_buffer_allocator.cpp > > > > > src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > > > And the one that gets used will be the one that is compiled in...? > > > > > > > > > > > > > That's a good point. I couldn't find a reason for this. > > > > If this is applicable for PlatformFrameBufferAllocator, I think we can > > > > change CameraBuffer too. > > > > > > Sure, if it simplifies things. The class is not a public API, and > > > doesn't need to be 'extensible' with a private implementation as far as > > > I am aware. In this case, it's just that there are two possible > > > implementations which are selected at compile time anyway. > > > > > > > +Laurent Pinchart , can I implement so? > > Why is CameraBuffer designed so? > > > > At the time I designed CameraBuffer using Extensible seemed like a > good idea. I had probably been lazy as there might be more opportune > patterns. > > What I was looking for was a mechanism that > > - Allows to define a standard and unique interface for the > CameraManager to interface with > - Had a compile-time selectable implementation > - Allowed to not pollute the shared interface with details of the > implementation > > I evaluated making use of a more canonical class hierarchy, and I > considered: > > a) A single .h file that defines the interface, multiple .cpp that > implement it selected at compile time. > > I considered that Extensible would have left more room for > implementation-specificities in the Private class interface, > allowing more flexibility and wouldn't have requried > implementation-specific details to surface in the interface. This > mostly apply to the implementation-specific data, which Extensible > allows to easy model in the Private subclass. > > Different .h is not an option and keeping them in sync with the > common user would be painful > > b) A canonical class hierarchy where the 'right' sub-class would be > instantiated at run time, but I ditched that as it would have > required #ifdefs in the code which I would prefer not to. > > There surely are better solution, as your implementation shows > details and requirement of a specific implementation are surfacing to > the interface as well, so this can be surely improved with some more > opportune design pattern. > > That said, I won't be too much concerned. The pattern is there and is > in use, simply replacing it with a) won't bring much value, so I would > say do whatever is faster for you and if we find a more elegant > solution we'll change both BufferAllocator and CameraBuffer in one > go. > Ah, it is a good point that with Extensible we can have the platform dependent class like allocDevice_ in the Extensible class. So I would go along with Extensible. -Hiro > - > > > -Hiro > > > -- > > > Kieran
Hello, On Thu, Nov 18, 2021 at 05:50:15PM +0900, Hirokazu Honda wrote: > On Thu, Nov 18, 2021 at 5:43 PM Jacopo Mondi wrote: > > On Thu, Nov 18, 2021 at 05:13:29PM +0900, Hirokazu Honda wrote: > > > On Thu, Nov 18, 2021 at 3:02 AM Kieran Bingham wrote: > > > > Quoting Hirokazu Honda (2021-11-10 03:09:34) > > > > > Hi Kieran, > > > > > > > > > > > In fact, it doesn't matter too much - but I still don't see why this > > > > > > needs to be Extensible. > > > > > > > > > > > > The implementation for PlatformFrameBufferAllocator can have an > > > > > > identically named class in both > > > > > > src/android/mm/cros_frame_buffer_allocator.cpp > > > > > > src/android/mm/generic_frame_buffer_allocator.cpp > > > > > > > > > > > > And the one that gets used will be the one that is compiled in...? > > > > > > > > > > That's a good point. I couldn't find a reason for this. > > > > > If this is applicable for PlatformFrameBufferAllocator, I think we can > > > > > change CameraBuffer too. > > > > > > > > Sure, if it simplifies things. The class is not a public API, and > > > > doesn't need to be 'extensible' with a private implementation as far as > > > > I am aware. In this case, it's just that there are two possible > > > > implementations which are selected at compile time anyway. > > > > > > +Laurent Pinchart , can I implement so? > > > Why is CameraBuffer designed so? > > > > At the time I designed CameraBuffer using Extensible seemed like a > > good idea. I had probably been lazy as there might be more opportune > > patterns. Usage of the Extensible class for CameraBuffer was a bit of a hack I think. In particular, I'm not fond of the PUBLIC_CAMERA_BUFFER_IMPLEMENTATION macro. I haven't taken the time to try and find a better solution. > > What I was looking for was a mechanism that > > > > - Allows to define a standard and unique interface for the > > CameraManager to interface with > > - Had a compile-time selectable implementation > > - Allowed to not pollute the shared interface with details of the > > implementation > > > > I evaluated making use of a more canonical class hierarchy, and I > > considered: > > > > a) A single .h file that defines the interface, multiple .cpp that > > implement it selected at compile time. > > > > I considered that Extensible would have left more room for > > implementation-specificities in the Private class interface, > > allowing more flexibility and wouldn't have requried > > implementation-specific details to surface in the interface. This > > mostly apply to the implementation-specific data, which Extensible > > allows to easy model in the Private subclass. > > > > Different .h is not an option and keeping them in sync with the > > common user would be painful > > > > b) A canonical class hierarchy where the 'right' sub-class would be > > instantiated at run time, but I ditched that as it would have > > required #ifdefs in the code which I would prefer not to. > > > > There surely are better solution, as your implementation shows > > details and requirement of a specific implementation are surfacing to > > the interface as well, so this can be surely improved with some more > > opportune design pattern. > > > > That said, I won't be too much concerned. The pattern is there and is > > in use, simply replacing it with a) won't bring much value, so I would > > say do whatever is faster for you and if we find a more elegant > > solution we'll change both BufferAllocator and CameraBuffer in one > > go. > > Ah, it is a good point that with Extensible we can have the platform > dependent class like allocDevice_ in the Extensible class. > So I would go along with Extensible.
Hi Hiro, Thank you for the patch. On Mon, Nov 01, 2021 at 04:16:52PM +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. This sounds quite confusing. > The existing FrameBufferAllocator cannot used because it 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> > --- > src/android/frame_buffer_allocator.h | 54 +++++++ > .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ > .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ > src/android/mm/meson.build | 6 +- > 4 files changed, 292 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> > +#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..9c7e4ea4 > --- /dev/null > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp > @@ -0,0 +1,90 @@ > +/* 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; Do you need this ? > + > +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() override = default; And this ? > + > + 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..167cc1a5 > --- /dev/null > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > @@ -0,0 +1,144 @@ > +/* 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 Extra space before allocDevice_. > + * owned by PlatformFrameBufferAllocator::Private. > + * GenericFrameBufferData must be destroyed before it is > + * destroyed. > + * > + * \todo: Consider managing alloc_device_t with std::shared_ptr s/todo:/todo/ > + * if this is difficult to maintain. > + * > + * Q: Thread safety against alloc_device_t is not documented. s/Q:/\todo/ > + * Is it no problem to call alloc/free in parallel? > + */ > + ASSERT(allocDevice_); You can drop this, it's tested in the constructor. > + 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); > + const unsigned int numPlanes = info.numPlanes(); > + std::vector<FrameBuffer::Plane> planes(numPlanes); std::vector<FrameBuffer::Plane> planes(info.numPlanes()); I wonder how all this will interact with a generic way to map FrameBuffer objects, in the libcamera core. We'll see when we get there, it will be interesting. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + /* 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
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..9c7e4ea4 --- /dev/null +++ b/src/android/mm/cros_frame_buffer_allocator.cpp @@ -0,0 +1,90 @@ +/* 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) + { + } + + ~Private() override = 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; + } + + /* 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..167cc1a5 --- /dev/null +++ b/src/android/mm/generic_frame_buffer_allocator.cpp @@ -0,0 +1,144 @@ +/* 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. + * + * Q: Thread safety against alloc_device_t is not documented. + * Is it no problem to call alloc/free in parallel? + */ + ASSERT(allocDevice_); + 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); + 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']) android_deps += [dependency('libcros_camera')] endif
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. 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> --- src/android/frame_buffer_allocator.h | 54 +++++++ .../mm/cros_frame_buffer_allocator.cpp | 90 +++++++++++ .../mm/generic_frame_buffer_allocator.cpp | 144 ++++++++++++++++++ src/android/mm/meson.build | 6 +- 4 files changed, 292 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