[libcamera-devel] [RFC PATCH 1/2] android: Introduce PlatformFrameBufferAllocator

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:
> PlatformFrameBufferAllocator allocates FrameBuffer(s) using
> cros::CameraBufferManager on ChromeOS and gralloc on non
> ChromeOS platform. The allocated FrameBuffer(s) are owned by
> PlatformFrameBufferAllocator an destroyed when

s/an/and/

> PlatformFrameBufferAllocator is destroyed.

It would also be useful to explain in the commit message why this is
needed.

Overall the patch looks pretty good. I think we're reaching a point
where we'll have to document Android classes such as this one though,
but it doesn't have to be part of this series.

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/frame_buffer.h              |  53 +++++++++++
>  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
>  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
>  src/android/mm/meson.build              |   6 +-
>  4 files changed, 258 insertions(+), 2 deletions(-)
>  create mode 100644 src/android/frame_buffer.h
>  create mode 100644 src/android/mm/cros_frame_buffer.cpp
>  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> 
> diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> new file mode 100644
> index 00000000..6aafeaf3
> --- /dev/null
> +++ b/src/android/frame_buffer.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * frame_buffer.h - Frame buffer allocating interface definition

s/allocating/allocator/

> + */
> +#ifndef __ANDROID_FRAME_BUFFER_H__
> +#define __ANDROID_FRAME_BUFFER_H__
> +
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/camera.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/geometry.h>
> +
> +class CameraDevice;
> +
> +class PlatformFrameBufferAllocator : libcamera::Extensible
> +{
> +	LIBCAMERA_DECLARE_PRIVATE()
> +
> +public:
> +	explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> +	~PlatformFrameBufferAllocator();
> +
> +	const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> +	allocate(int halPixelFormat,
> +		 const libcamera::Size &size,
> +		 uint32_t usage,
> +		 size_t numBuffers);
> +};
> +
> +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION				\

I'm not a big fan of this design pattern, but it already exists in
camera_buffer.h, so we can keep it for now and address both later.

> +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(		\
> +	CameraDevice *const cameraDevice)				\
> +	: Extensible(std::make_unique<Private>(this, cameraDevice))	\
> +{									\
> +}									\
> +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()		\
> +{									\
> +}									\
> +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>&		\
> +PlatformFrameBufferAllocator::allocate(int halPixelFormat,		\
> +				       const libcamera::Size& size,	\
> +				       uint32_t usage,			\
> +				       size_t numBuffers)		\
> +{									\
> +	return _d()->allocate(halPixelFormat, size, usage, numBuffers);	\
> +}
> +
> +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> new file mode 100644
> index 00000000..114c739b
> --- /dev/null
> +++ b/src/android/mm/cros_frame_buffer.cpp
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend

s/allocate/Allocate/

> + * using CameraBufferManager
> + */
> +
> +#include "../frame_buffer.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include "cros-camera/camera_buffer_manager.h"
> +
> +#include "../camera_device.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> +
> +public:
> +	Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> +		[[maybe_unused]] CameraDevice *const cameraDevice)
> +	{
> +	}
> +
> +	~Private() = default;
> +
> +	const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> +	allocate(int halPixelFormat,
> +		 const libcamera::Size &size,
> +		 uint32_t usage,
> +		 size_t numBuffers);
> +
> +private:
> +	std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> +	std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> +};
> +
> +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> +PlatformFrameBufferAllocator::Private::allocate(
> +	int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> +	size_t numBuffers)
> +{
> +	ASSERT(allocatedBuffers_.empty());
> +
> +	std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> +	for (size_t i = 0; i < numBuffers; ++i) {
> +		cros::ScopedBufferHandle handle =
> +			cros::CameraBufferManager::AllocateScopedBuffer(
> +				size.width, size.height, halPixelFormat, usage);
> +		if (!handle) {
> +			LOG(HAL, Error) << "Failed allocate buffer_handle";

s/Failed/Failed to/

> +			return allocatedBuffers_;

I think

			return {};

would be more explicit. For a moment I thought the function would return
a partial vector of some of the allocations succeeded. Same below.

> +		}
> +
> +		const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> +		std::vector<FrameBuffer::Plane> planes(numPlanes);
> +		for (size_t j = 0; j < numPlanes; ++j) {

This could also be written

		for (auto [j, plane] : utils::enumerate(planes)) {

Up to you.

> +			FileDescriptor fd{ (*handle)->data[0] };
> +			if (!fd.isValid()) {
> +				LOG(HAL, Fatal) << "Invalid fd";
> +				return allocatedBuffers_;
> +			}

You can move this outside of the loop. FileDescriptor uses implicit
sharing of the actual fd, so all instances of planes[j].fd will use the
same numerical fd, avoiding the dup() call and saving file descriptor
space.

> +
> +			planes[j].fd = fd;
> +			planes[j].offset =
> +				cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> +			planes[j].length =
> +				cros::CameraBufferManager::GetPlaneSize(*handle, j);
> +		}
> +
> +		buffers.push_back(std::make_unique<FrameBuffer>(planes));
> +		allocatedHandles_.push_back(std::move(handle));
> +	}
> +
> +	allocatedBuffers_ = std::move(buffers);
> +	return allocatedBuffers_;
> +}
> +
> +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> new file mode 100644
> index 00000000..b387d5a2
> --- /dev/null
> +++ b/src/android/mm/generic_frame_buffer.cpp
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> + * buffer backend
> + */
> +
> +#include "../frame_buffer.h"
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/internal/formats.h>
> +
> +#include <hardware/camera3.h>
> +#include <hardware/gralloc.h>
> +#include <hardware/hardware.h>
> +
> +#include "../camera_device.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> +
> +public:
> +	Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> +		CameraDevice *const cameraDevice)
> +		: cameraDevice_(cameraDevice),
> +		  hardwareModule_(cameraDevice->camera3Device()->common.module)

allocDevice_ should be initialized to nullptr or the destructor could
end up calling gralloc_close() on a random pointer.

> +	{
> +		ASSERT(!!hardwareModule_);

Why not

		ASSERT(hardwareModule_);

?

> +	}
> +
> +	~Private() override;
> +
> +	const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> +	allocate(int halPixelFormat,
> +		 const libcamera::Size &size,
> +		 uint32_t usage,
> +		 size_t numBuffers);
> +
> +private:
> +	const CameraDevice *const cameraDevice_;
> +	struct hw_module_t *const hardwareModule_;
> +	struct alloc_device_t *allocDevice_;
> +
> +	std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> +	std::vector<buffer_handle_t> bufferHandles_;
> +};
> +
> +PlatformFrameBufferAllocator::Private::~Private()
> +{
> +	for (buffer_handle_t &handle : bufferHandles_) {
> +		ASSERT(allocDevice_);

I would drop the assert, this really can't happen.

> +		allocDevice_->free(allocDevice_, handle);
> +	}

A blank line would be nice.

> +	if (allocDevice_)
> +		gralloc_close(allocDevice_);
> +}
> +
> +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> +PlatformFrameBufferAllocator::Private::allocate(
> +	int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> +	size_t numBuffers)
> +{
> +	ASSERT(allocatedBuffers_.empty());
> +	ASSERT(bufferHandles_.empty());
> +	ASSERT(!allocDevice_);
> +
> +	int ret = gralloc_open(hardwareModule_, &allocDevice_);
> +	if (ret) {
> +		LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> +		return allocatedBuffers_;

Same here,

		return {};

> +	}
> +
> +	int stride = 0;
> +	for (size_t i = 0; i < numBuffers; ++i) {
> +		buffer_handle_t handle{};
> +		ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> +					  halPixelFormat, usage, &handle, &stride);

I suppose the stride is guaranteed to be the same for all allocations ?

> +		if (ret) {
> +			LOG(HAL, Error) << "failed buffer allocating: " << ret;
> +			return allocatedBuffers_;
> +		}
> +
> +		bufferHandles_.push_back(handle);
> +	}
> +
> +	const libcamera::PixelFormat pixelFormat =
> +		cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> +	const auto &info = PixelFormatInfo::info(pixelFormat);
> +	const unsigned int numPlanes = info.numPlanes();
> +
> +	allocatedBuffers_.reserve(numBuffers);
> +	for (const buffer_handle_t &handle : bufferHandles_) {
> +		std::vector<FrameBuffer::Plane> planes(numPlanes);
> +		size_t offset = 0;
> +		for (size_t i = 0; i < numPlanes; ++i) {

Here too you could use utils::enumerate().

> +			planes[i].fd = FileDescriptor(handle->data[i]);
> +			size_t planeSize = info.planeSize(size.height, i, stride);
> +
> +			planes[i].offset = offset;

Does Android use a different dmabuf fd for each plane ? If not, I think
the offset should be 0, and if it does, we could move the FileDescriptor
construction outside of the loop.

> +			planes[i].length = planeSize;
> +			offset += planeSize;
> +		}
> +
> +		allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> +	}
> +
> +	return allocatedBuffers_;
> +}
> +
> +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> index eeb5cc2e..d1ad64d9 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -2,8 +2,10 @@
>  
>  platform = get_option('android_platform')
>  if platform == 'generic'
> -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> +    android_hal_sources += files(['generic_camera_buffer.cpp',
> +                                  'generic_frame_buffer.cpp'])
>  elif platform == 'cros'
> -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> +    android_hal_sources += files(['cros_camera_buffer.cpp',
> +                                  'cros_frame_buffer.cpp'])
>      android_deps += [dependency('libcros_camera')]
>  endif

-- 
Regards,

Laurent Pinchart