[libcamera-devel,2/2] android: Introduce PlatformFrameBufferAllocator
diff mbox series

Message ID 20211028073038.653786-3-hiroh@chromium.org
State Superseded
Headers show
Series
  • Introduce PlatformFrameBufferAllocator
Related show

Commit Message

Hirokazu Honda Oct. 28, 2021, 7:30 a.m. UTC 
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 CL 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          |  55 +++++++
 .../mm/cros_frame_buffer_allocator.cpp        |  88 +++++++++++
 .../mm/generic_frame_buffer_allocator.cpp     | 140 ++++++++++++++++++
 src/android/mm/meson.build                    |   6 +-
 4 files changed, 287 insertions(+), 2 deletions(-)
 create mode 100644 src/android/frame_buffer_allocator.h
 create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp
 create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp

Comments

Jacopo Mondi Oct. 29, 2021, 3:09 p.m. UTC | #1 
On Thu, Oct 28, 2021 at 04:30:38PM +0900, Hirokazu Honda wrote:
> After we unify identical streams to one stream configuration, the
> capture requested by a HAL client can have been resolved as
> CameraStream::Mapped. That is, a buffer to be written by a camera
> is not provided by a client. We would handle this case by
> dynamically allocating FrameBuffer.
>
> The existing FrameBufferAllocator cannot used because it is not
> allowed to allocate a new buffer while Camera is running.

That's a nice context but seems slightly unrelated to this patch :)
>
> This CL introduces PlatformFrameBufferAllocator. It allocates

After years, I still don't know what CL means

> 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          |  55 +++++++
>  .../mm/cros_frame_buffer_allocator.cpp        |  88 +++++++++++
>  .../mm/generic_frame_buffer_allocator.cpp     | 140 ++++++++++++++++++
>  src/android/mm/meson.build                    |   6 +-
>  4 files changed, 287 insertions(+), 2 deletions(-)
>  create mode 100644 src/android/frame_buffer_allocator.h
>  create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp
>  create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp
>
> diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h
> new file mode 100644
> index 00000000..e46a43b5
> --- /dev/null
> +++ b/src/android/frame_buffer_allocator.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in
> + * platform dependent way.
> + */
> +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
> +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
> +
> +#include <memory>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/camera.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/geometry.h>
> +
> +class CameraDevice;
> +
> +class PlatformFrameBufferAllocator : libcamera::Extensible
> +{
> +	LIBCAMERA_DECLARE_PRIVATE()
> +
> +public:
> +	explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> +	~PlatformFrameBufferAllocator();
> +
> +	/*
> +	 * FrameBuffer owns the underlying buffer. Returns nullptr on failure.
> +	 * Note: The returned FrameBuffer needs to be destroyed before
> +	 * PlatformFrameBufferAllocator is destroyed.
> +	 */

This applies to both backends I assume

> +	std::unique_ptr<libcamera::FrameBuffer> allocate(
> +		int halPixelFormat, const libcamera::Size &size, uint32_t usage);
> +};
> +
> +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION			\
> +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(		\
> +	CameraDevice *const cameraDevice)				\
> +	: Extensible(std::make_unique<Private>(this, cameraDevice))	\
> +{									\
> +}									\
> +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()		\
> +{									\
> +}									\
> +									\

No empty line if you have none between the other declarations

> +std::unique_ptr<libcamera::FrameBuffer>					\
> +PlatformFrameBufferAllocator::allocate(int halPixelFormat,		\
> +				       const libcamera::Size& size,	\
> +				       uint32_t usage)			\
> +{									\
> +	return _d()->allocate(halPixelFormat, size, usage);		\
> +}
> +
> +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */
> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp
> new file mode 100644
> index 00000000..9d7e3c88
> --- /dev/null
> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using
> + * CameraBufferManager
> + */
> +
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/framebuffer.h"
> +
> +#include "../camera_device.h"
> +#include "../frame_buffer_allocator.h"
> +#include "cros-camera/camera_buffer_manager.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +namespace {
> +class CrosFrameBufferData : public FrameBuffer::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)

empty line

> +public:
> +	CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)
> +		: FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))
> +	{
> +	}
> +
> +	~CrosFrameBufferData() override = default;

empty line

> +private:
> +	cros::ScopedBufferHandle scopedHandle_;
> +};
> +} /* namespace */
> +
> +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> +
> +public:
> +	Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> +		[[maybe_unused]] CameraDevice *const cameraDevice)
> +	{
> +	}
> +
> +	~Private() = default;
> +
> +	std::unique_ptr<libcamera::FrameBuffer> allocate(
> +		int halPixelFormat, const libcamera::Size &size, uint32_t usage);
> +};
> +
> +std::unique_ptr<libcamera::FrameBuffer>
> +PlatformFrameBufferAllocator::Private::allocate(
> +	int halPixelFormat, const libcamera::Size &size, uint32_t usage)
> +{
> +	cros::ScopedBufferHandle scopedHandle =
> +		cros::CameraBufferManager::AllocateScopedBuffer(
> +			size.width, size.height, halPixelFormat, usage);
> +	if (!scopedHandle) {
> +		LOG(HAL, Error) << "Failed to allocate buffer handle";
> +		return nullptr;
> +	}
> +
> +	buffer_handle_t handle = *scopedHandle;
> +	const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);
> +	std::vector<FrameBuffer::Plane> planes(numPlanes);
> +	FileDescriptor fd{ handle->data[0] };
> +	if (!fd.isValid()) {
> +		LOG(HAL, Fatal) << "Invalid fd";
> +		return nullptr;
> +	}
> +

        Also this assumes all planes are contigous, right ?

> +	for (auto [i, plane] : utils::enumerate(planes)) {
> +		plane.fd = fd;
> +		plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);
> +		plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);
> +	}
> +
> +	return std::make_unique<FrameBuffer>(
> +		std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> +		planes);
> +}
> +
> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> new file mode 100644
> index 00000000..1bc3c405
> --- /dev/null
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API
> + */
> +
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/formats.h"
> +#include "libcamera/internal/framebuffer.h"
> +
> +#include <hardware/camera3.h>
> +#include <hardware/gralloc.h>
> +#include <hardware/hardware.h>
> +
> +#include "../camera_device.h"
> +#include "../frame_buffer_allocator.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +namespace {
> +class GenericFrameBufferData : public FrameBuffer::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)

empty line

> +public:
> +	GenericFrameBufferData(struct alloc_device_t *allocDevice,
> +			       buffer_handle_t handle)
> +		: allocDevice_(allocDevice), handle_(handle)
> +	{
> +		ASSERT(allocDevice_);
> +		ASSERT(handle_);
> +	}

In the other implementation you had an empty line here

> +	~GenericFrameBufferData()
> +	{
> +		/*
> +		 *  allocDevice_ is used to destroy handle_. allocDevice_ is
> +		 * owned by PlatformFrameBufferAllocator::Private.
> +		 * GenericFrameBufferData must be destroyed before it is
> +		 * destroyed.
> +		 *
> +		 * \todo: Consider managing alloc_device_t with std::shared_ptr
> +		 * if this is difficult to maintain.

In the meantime I would be tempted to ASSERT(allocDevice_) to catch
potential issues early.

> +		 *
> +		 * Q: Thread safety against alloc_device_t is not documented.
> +		 * Is it no problem to call alloc/free in parallel?
> +		 */
> +		allocDevice_->free(allocDevice_, handle_);
> +	}

empty line

> +private:
> +	struct alloc_device_t *allocDevice_;
> +	const buffer_handle_t handle_;
> +};
> +} /* namespace */
> +
> +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> +
> +public:
> +	Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> +		CameraDevice *const cameraDevice)
> +		: cameraDevice_(cameraDevice),
> +		  hardwareModule_(cameraDevice->camera3Device()->common.module),
> +		  allocDevice_(nullptr)
> +	{
> +		ASSERT(hardwareModule_);
> +	}
> +
> +	~Private() override;
> +
> +	std::unique_ptr<libcamera::FrameBuffer> allocate(
> +		int halPixelFormat, const libcamera::Size &size, uint32_t usage);

override ?

> +
> +private:
> +	const CameraDevice *const cameraDevice_;
> +	struct hw_module_t *const hardwareModule_;
> +	struct alloc_device_t *allocDevice_;
> +};
> +
> +PlatformFrameBufferAllocator::Private::~Private()
> +{
> +	if (allocDevice_)
> +		gralloc_close(allocDevice_);
> +}
> +
> +std::unique_ptr<libcamera::FrameBuffer>
> +PlatformFrameBufferAllocator::Private::allocate(
> +	int halPixelFormat, const libcamera::Size &size, uint32_t usage)
> +{
> +	if (!allocDevice_) {
> +		int ret = gralloc_open(hardwareModule_, &allocDevice_);
> +		if (ret) {
> +			LOG(HAL, Error) << "gralloc_open() failed: " << ret;

Should this be Fatal ?

> +			return nullptr;
> +		}
> +	}
> +
> +	int stride = 0;
> +	buffer_handle_t handle = nullptr;
> +	int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> +				      halPixelFormat, usage, &handle, &stride);
> +	if (ret) {
> +		LOG(HAL, Error) << "failed buffer allocating: " << ret;
> +		return nullptr;
> +	}
> +	if (!handle) {
> +		LOG(HAL, Fatal) << "buffer_handle_t is empty on success";
> +		return nullptr;
> +	}

I don't know the gralloc API but can (!ret && !handle)

> +
> +	const libcamera::PixelFormat pixelFormat =
> +		cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> +	const auto &info = PixelFormatInfo::info(pixelFormat);
> +	const unsigned int numPlanes = info.numPlanes();
> +	std::vector<FrameBuffer::Plane> planes(numPlanes);
> +
> +	/* This code assumes the planes are mapped consecutively. */
> +	FileDescriptor fd{ handle->data[0] };
> +	size_t offset = 0;
> +	for (auto [i, plane] : utils::enumerate(planes)) {
> +		const size_t planeSize = info.planeSize(size.height, i, stride);
> +
> +		planes[i].fd = fd;
> +		planes[i].offset = offset;
> +		planes[i].length = planeSize;
> +		offset += planeSize;
> +	}
> +
> +	return std::make_unique<FrameBuffer>(
> +		std::make_unique<GenericFrameBufferData>(allocDevice_, handle),
> +		planes);
> +}
> +
> +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> index eeb5cc2e..d40a3b0b 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -2,8 +2,10 @@
>
>  platform = get_option('android_platform')
>  if platform == 'generic'
> -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> +    android_hal_sources += files(['generic_camera_buffer.cpp',
> +                                  'generic_frame_buffer_allocator.cpp'])
>  elif platform == 'cros'
> -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> +    android_hal_sources += files(['cros_camera_buffer.cpp',
> +                                  'cros_frame_buffer_allocator.cpp'])

Very nice overall!

Before merging, do you think it would be possible to replace the usage
of FrameBufferAllocator in CameraStream with this new API and run CTS
on it ?

Thanks
   j

>      android_deps += [dependency('libcros_camera')]
>  endif
> --
> 2.33.1.1089.g2158813163f-goog
>
Hirokazu Honda Nov. 1, 2021, 5:09 a.m. UTC | #2 
HI Jacopo, thank you for reviewing.

On Sat, Oct 30, 2021 at 12:08 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> On Thu, Oct 28, 2021 at 04:30:38PM +0900, Hirokazu Honda wrote:
> > After we unify identical streams to one stream configuration, the
> > capture requested by a HAL client can have been resolved as
> > CameraStream::Mapped. That is, a buffer to be written by a camera
> > is not provided by a client. We would handle this case by
> > dynamically allocating FrameBuffer.
> >
> > The existing FrameBufferAllocator cannot used because it is not
> > allowed to allocate a new buffer while Camera is running.
>
> That's a nice context but seems slightly unrelated to this patch :)
> >
> > This CL introduces PlatformFrameBufferAllocator. It allocates
>
> After years, I still don't know what CL means
>

Oops.. CL stands for change list. It is Google idom. Removed.
https://stackoverflow.com/questions/25716920/what-does-cl-mean-in-a-commit-message-what-does-it-stand-for

> > FrameBuffer using cros::CameraBufferManager on ChromeOS and
> > gralloc on non ChromeOS platform. The allocated FrameBuffer owns
> > the underlying buffer but must be destroyed before
> > PlatformFrameBufferAllocator is destroyed.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/frame_buffer_allocator.h          |  55 +++++++
> >  .../mm/cros_frame_buffer_allocator.cpp        |  88 +++++++++++
> >  .../mm/generic_frame_buffer_allocator.cpp     | 140 ++++++++++++++++++
> >  src/android/mm/meson.build                    |   6 +-
> >  4 files changed, 287 insertions(+), 2 deletions(-)
> >  create mode 100644 src/android/frame_buffer_allocator.h
> >  create mode 100644 src/android/mm/cros_frame_buffer_allocator.cpp
> >  create mode 100644 src/android/mm/generic_frame_buffer_allocator.cpp
> >
> > diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h
> > new file mode 100644
> > index 00000000..e46a43b5
> > --- /dev/null
> > +++ b/src/android/frame_buffer_allocator.h
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in
> > + * platform dependent way.
> > + */
> > +#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
> > +#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
> > +
> > +#include <memory>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/camera.h>
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/geometry.h>
> > +
> > +class CameraDevice;
> > +
> > +class PlatformFrameBufferAllocator : libcamera::Extensible
> > +{
> > +     LIBCAMERA_DECLARE_PRIVATE()
> > +
> > +public:
> > +     explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> > +     ~PlatformFrameBufferAllocator();
> > +
> > +     /*
> > +      * FrameBuffer owns the underlying buffer. Returns nullptr on failure.
> > +      * Note: The returned FrameBuffer needs to be destroyed before
> > +      * PlatformFrameBufferAllocator is destroyed.
> > +      */
>
> This applies to both backends I assume
>

Actually this restriction doesn't apply cros_frame_buffer_allocator.
cros::ScopedBufferHandle will be destroyed by using the leaky
cros::CameraBufferManager instance.
See https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=593;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e


> > +     std::unique_ptr<libcamera::FrameBuffer> allocate(
> > +             int halPixelFormat, const libcamera::Size &size, uint32_t usage);
> > +};
> > +
> > +#define PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION                 \
> > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(          \
> > +     CameraDevice *const cameraDevice)                               \
> > +     : Extensible(std::make_unique<Private>(this, cameraDevice))     \
> > +{                                                                    \
> > +}                                                                    \
> > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()                \
> > +{                                                                    \
> > +}                                                                    \
> > +                                                                     \
>
> No empty line if you have none between the other declarations
>
> > +std::unique_ptr<libcamera::FrameBuffer>                                      \
> > +PlatformFrameBufferAllocator::allocate(int halPixelFormat,           \
> > +                                    const libcamera::Size& size,     \
> > +                                    uint32_t usage)                  \
> > +{                                                                    \
> > +     return _d()->allocate(halPixelFormat, size, usage);             \
> > +}
> > +
> > +#endif /* __ANDROID_FRAME_BUFFER_ALLOCATOR_H__ */
> > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp
> > new file mode 100644
> > index 00000000..9d7e3c88
> > --- /dev/null
> > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using
> > + * CameraBufferManager
> > + */
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/framebuffer.h"
> > +
> > +#include "../camera_device.h"
> > +#include "../frame_buffer_allocator.h"
> > +#include "cros-camera/camera_buffer_manager.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL)
> > +
> > +namespace {
> > +class CrosFrameBufferData : public FrameBuffer::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>
> empty line
>
> > +public:
> > +     CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)
> > +             : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))
> > +     {
> > +     }
> > +
> > +     ~CrosFrameBufferData() override = default;
>
> empty line
>
> > +private:
> > +     cros::ScopedBufferHandle scopedHandle_;
> > +};
> > +} /* namespace */
> > +
> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > +
> > +public:
> > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > +             [[maybe_unused]] CameraDevice *const cameraDevice)
> > +     {
> > +     }
> > +
> > +     ~Private() = default;
> > +
> > +     std::unique_ptr<libcamera::FrameBuffer> allocate(
> > +             int halPixelFormat, const libcamera::Size &size, uint32_t usage);
> > +};
> > +
> > +std::unique_ptr<libcamera::FrameBuffer>
> > +PlatformFrameBufferAllocator::Private::allocate(
> > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage)
> > +{
> > +     cros::ScopedBufferHandle scopedHandle =
> > +             cros::CameraBufferManager::AllocateScopedBuffer(
> > +                     size.width, size.height, halPixelFormat, usage);
> > +     if (!scopedHandle) {
> > +             LOG(HAL, Error) << "Failed to allocate buffer handle";
> > +             return nullptr;
> > +     }
> > +
> > +     buffer_handle_t handle = *scopedHandle;
> > +     const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(handle);
> > +     std::vector<FrameBuffer::Plane> planes(numPlanes);
> > +     FileDescriptor fd{ handle->data[0] };
> > +     if (!fd.isValid()) {
> > +             LOG(HAL, Fatal) << "Invalid fd";
> > +             return nullptr;
> > +     }
> > +
>
>         Also this assumes all planes are contigous, right ?
>
> > +     for (auto [i, plane] : utils::enumerate(planes)) {
> > +             plane.fd = fd;
> > +             plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);
> > +             plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);
> > +     }
> > +
> > +     return std::make_unique<FrameBuffer>(
> > +             std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> > +             planes);
> > +}
> > +
> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> > new file mode 100644
> > index 00000000..1bc3c405
> > --- /dev/null
> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API
> > + */
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/framebuffer.h"
> > +
> > +#include <hardware/camera3.h>
> > +#include <hardware/gralloc.h>
> > +#include <hardware/hardware.h>
> > +
> > +#include "../camera_device.h"
> > +#include "../frame_buffer_allocator.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL)
> > +
> > +namespace {
> > +class GenericFrameBufferData : public FrameBuffer::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>
> empty line
>
> > +public:
> > +     GenericFrameBufferData(struct alloc_device_t *allocDevice,
> > +                            buffer_handle_t handle)
> > +             : allocDevice_(allocDevice), handle_(handle)
> > +     {
> > +             ASSERT(allocDevice_);
> > +             ASSERT(handle_);
> > +     }
>
> In the other implementation you had an empty line here
>
> > +     ~GenericFrameBufferData()
> > +     {
> > +             /*
> > +              *  allocDevice_ is used to destroy handle_. allocDevice_ is
> > +              * owned by PlatformFrameBufferAllocator::Private.
> > +              * GenericFrameBufferData must be destroyed before it is
> > +              * destroyed.
> > +              *
> > +              * \todo: Consider managing alloc_device_t with std::shared_ptr
> > +              * if this is difficult to maintain.
>
> In the meantime I would be tempted to ASSERT(allocDevice_) to catch
> potential issues early.
>
> > +              *
> > +              * Q: Thread safety against alloc_device_t is not documented.
> > +              * Is it no problem to call alloc/free in parallel?
> > +              */
> > +             allocDevice_->free(allocDevice_, handle_);
> > +     }
>
> empty line
>
> > +private:
> > +     struct alloc_device_t *allocDevice_;
> > +     const buffer_handle_t handle_;
> > +};
> > +} /* namespace */
> > +
> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > +
> > +public:
> > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > +             CameraDevice *const cameraDevice)
> > +             : cameraDevice_(cameraDevice),
> > +               hardwareModule_(cameraDevice->camera3Device()->common.module),
> > +               allocDevice_(nullptr)
> > +     {
> > +             ASSERT(hardwareModule_);
> > +     }
> > +
> > +     ~Private() override;
> > +
> > +     std::unique_ptr<libcamera::FrameBuffer> allocate(
> > +             int halPixelFormat, const libcamera::Size &size, uint32_t usage);
>
> override ?

We don't need override because allocate() is not Extensible::Private function.

>
> > +
> > +private:
> > +     const CameraDevice *const cameraDevice_;
> > +     struct hw_module_t *const hardwareModule_;
> > +     struct alloc_device_t *allocDevice_;
> > +};
> > +
> > +PlatformFrameBufferAllocator::Private::~Private()
> > +{
> > +     if (allocDevice_)
> > +             gralloc_close(allocDevice_);
> > +}
> > +
> > +std::unique_ptr<libcamera::FrameBuffer>
> > +PlatformFrameBufferAllocator::Private::allocate(
> > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage)
> > +{
> > +     if (!allocDevice_) {
> > +             int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > +             if (ret) {
> > +                     LOG(HAL, Error) << "gralloc_open() failed: " << ret;
>
> Should this be Fatal ?
>
> > +                     return nullptr;
> > +             }
> > +     }
> > +
> > +     int stride = 0;
> > +     buffer_handle_t handle = nullptr;
> > +     int ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > +                                   halPixelFormat, usage, &handle, &stride);
> > +     if (ret) {
> > +             LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > +             return nullptr;
> > +     }
> > +     if (!handle) {
> > +             LOG(HAL, Fatal) << "buffer_handle_t is empty on success";
> > +             return nullptr;
> > +     }
>
> I don't know the gralloc API but can (!ret && !handle)
>

I would like to check independently. Or this can be ASSERT(!handle).

> > +
> > +     const libcamera::PixelFormat pixelFormat =
> > +             cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > +     const auto &info = PixelFormatInfo::info(pixelFormat);
> > +     const unsigned int numPlanes = info.numPlanes();
> > +     std::vector<FrameBuffer::Plane> planes(numPlanes);
> > +
> > +     /* This code assumes the planes are mapped consecutively. */
> > +     FileDescriptor fd{ handle->data[0] };
> > +     size_t offset = 0;
> > +     for (auto [i, plane] : utils::enumerate(planes)) {
> > +             const size_t planeSize = info.planeSize(size.height, i, stride);
> > +
> > +             planes[i].fd = fd;
> > +             planes[i].offset = offset;
> > +             planes[i].length = planeSize;
> > +             offset += planeSize;
> > +     }
> > +
> > +     return std::make_unique<FrameBuffer>(
> > +             std::make_unique<GenericFrameBufferData>(allocDevice_, handle),
> > +             planes);
> > +}
> > +
> > +PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > index eeb5cc2e..d40a3b0b 100644
> > --- a/src/android/mm/meson.build
> > +++ b/src/android/mm/meson.build
> > @@ -2,8 +2,10 @@
> >
> >  platform = get_option('android_platform')
> >  if platform == 'generic'
> > -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> > +    android_hal_sources += files(['generic_camera_buffer.cpp',
> > +                                  'generic_frame_buffer_allocator.cpp'])
> >  elif platform == 'cros'
> > -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> > +    android_hal_sources += files(['cros_camera_buffer.cpp',
> > +                                  'cros_frame_buffer_allocator.cpp'])
>
> Very nice overall!
>
> Before merging, do you think it would be possible to replace the usage
> of FrameBufferAllocator in CameraStream with this new API and run CTS
> on it ?
>

Sure. I will do.

-Hiro
> Thanks
>    j
>
> >      android_deps += [dependency('libcros_camera')]
> >  endif
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

Patch
diff mbox series 

diff --git a/src/android/frame_buffer_allocator.h b/src/android/frame_buffer_allocator.h
new file mode 100644
index 00000000..e46a43b5
--- /dev/null
+++ b/src/android/frame_buffer_allocator.h
@@ -0,0 +1,55 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * frame_buffer_allocator.h - Interface definition to allocate Frame buffer in
+ * platform dependent way.
+ */
+#ifndef __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
+#define __ANDROID_FRAME_BUFFER_ALLOCATOR_H__
+
+#include <memory>
+
+#include <libcamera/base/class.h>
+#include <libcamera/camera.h>
+#include <libcamera/framebuffer.h>
+#include <libcamera/geometry.h>
+
+class CameraDevice;
+
+class PlatformFrameBufferAllocator : libcamera::Extensible
+{
+	LIBCAMERA_DECLARE_PRIVATE()
+
+public:
+	explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
+	~PlatformFrameBufferAllocator();
+
+	/*
+	 * FrameBuffer owns the underlying buffer. Returns nullptr on failure.
+	 * Note: The returned FrameBuffer needs to be destroyed before
+	 * PlatformFrameBufferAllocator is destroyed.
+	 */
+	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..9d7e3c88
--- /dev/null
+++ b/src/android/mm/cros_frame_buffer_allocator.cpp
@@ -0,0 +1,88 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * cros_frame_buffer.cpp - Allocate FrameBuffer for Chromium OS using
+ * CameraBufferManager
+ */
+
+#include <memory>
+#include <vector>
+
+#include <libcamera/base/log.h>
+
+#include "libcamera/internal/framebuffer.h"
+
+#include "../camera_device.h"
+#include "../frame_buffer_allocator.h"
+#include "cros-camera/camera_buffer_manager.h"
+
+using namespace libcamera;
+
+LOG_DECLARE_CATEGORY(HAL)
+
+namespace {
+class CrosFrameBufferData : public FrameBuffer::Private
+{
+	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
+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() = 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;
+	}
+
+	for (auto [i, plane] : utils::enumerate(planes)) {
+		plane.fd = fd;
+		plane.offset = cros::CameraBufferManager::GetPlaneOffset(handle, i);
+		plane.length = cros::CameraBufferManager::GetPlaneSize(handle, i);
+	}
+
+	return std::make_unique<FrameBuffer>(
+		std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
+		planes);
+}
+
+PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
new file mode 100644
index 00000000..1bc3c405
--- /dev/null
+++ b/src/android/mm/generic_frame_buffer_allocator.cpp
@@ -0,0 +1,140 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API
+ */
+
+#include <memory>
+#include <vector>
+
+#include <libcamera/base/log.h>
+
+#include "libcamera/internal/formats.h"
+#include "libcamera/internal/framebuffer.h"
+
+#include <hardware/camera3.h>
+#include <hardware/gralloc.h>
+#include <hardware/hardware.h>
+
+#include "../camera_device.h"
+#include "../frame_buffer_allocator.h"
+
+using namespace libcamera;
+
+LOG_DECLARE_CATEGORY(HAL)
+
+namespace {
+class GenericFrameBufferData : public FrameBuffer::Private
+{
+	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
+public:
+	GenericFrameBufferData(struct alloc_device_t *allocDevice,
+			       buffer_handle_t handle)
+		: allocDevice_(allocDevice), handle_(handle)
+	{
+		ASSERT(allocDevice_);
+		ASSERT(handle_);
+	}
+	~GenericFrameBufferData()
+	{
+		/*
+		 *  allocDevice_ is used to destroy handle_. allocDevice_ is
+		 * owned by PlatformFrameBufferAllocator::Private.
+		 * GenericFrameBufferData must be destroyed before it is
+		 * destroyed.
+		 *
+		 * \todo: Consider managing alloc_device_t with std::shared_ptr
+		 * if this is difficult to maintain.
+		 *
+		 * Q: Thread safety against alloc_device_t is not documented.
+		 * Is it no problem to call alloc/free in parallel?
+		 */
+		allocDevice_->free(allocDevice_, handle_);
+	}
+private:
+	struct alloc_device_t *allocDevice_;
+	const buffer_handle_t handle_;
+};
+} /* namespace */
+
+class PlatformFrameBufferAllocator::Private : public Extensible::Private
+{
+	LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
+
+public:
+	Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
+		CameraDevice *const cameraDevice)
+		: cameraDevice_(cameraDevice),
+		  hardwareModule_(cameraDevice->camera3Device()->common.module),
+		  allocDevice_(nullptr)
+	{
+		ASSERT(hardwareModule_);
+	}
+
+	~Private() override;
+
+	std::unique_ptr<libcamera::FrameBuffer> allocate(
+		int halPixelFormat, const libcamera::Size &size, uint32_t usage);
+
+private:
+	const CameraDevice *const cameraDevice_;
+	struct hw_module_t *const hardwareModule_;
+	struct alloc_device_t *allocDevice_;
+};
+
+PlatformFrameBufferAllocator::Private::~Private()
+{
+	if (allocDevice_)
+		gralloc_close(allocDevice_);
+}
+
+std::unique_ptr<libcamera::FrameBuffer>
+PlatformFrameBufferAllocator::Private::allocate(
+	int halPixelFormat, const libcamera::Size &size, uint32_t usage)
+{
+	if (!allocDevice_) {
+		int ret = gralloc_open(hardwareModule_, &allocDevice_);
+		if (ret) {
+			LOG(HAL, Error) << "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