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

13944 diff mbox series

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
PlatformFrameBufferAllocator is destroyed.

Signed-off-by: Hirokazu Honda <hiroh@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

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@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

Hi Laurent, thank you for comments.

On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@chromium.org>
> > ---
> >  src/android/frame_buffer.h              |  53 +++++++++++
> >  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
> >  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> >  src/android/mm/meson.build              |   6 +-
> >  4 files changed, 258 insertions(+), 2 deletions(-)
> >  create mode 100644 src/android/frame_buffer.h
> >  create mode 100644 src/android/mm/cros_frame_buffer.cpp
> >  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> >
> > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> > new file mode 100644
> > index 00000000..6aafeaf3
> > --- /dev/null
> > +++ b/src/android/frame_buffer.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * frame_buffer.h - Frame buffer allocating interface definition
>
> s/allocating/allocator/
>
> > + */
> > +#ifndef __ANDROID_FRAME_BUFFER_H__
> > +#define __ANDROID_FRAME_BUFFER_H__
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/camera.h>
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/geometry.h>
> > +
> > +class CameraDevice;
> > +
> > +class PlatformFrameBufferAllocator : libcamera::Extensible
> > +{
> > +     LIBCAMERA_DECLARE_PRIVATE()
> > +
> > +public:
> > +     explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> > +     ~PlatformFrameBufferAllocator();
> > +
> > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > +     allocate(int halPixelFormat,
> > +              const libcamera::Size &size,
> > +              uint32_t usage,
> > +              size_t numBuffers);
> > +};
> > +
> > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION                           \
>
> I'm not a big fan of this design pattern, but it already exists in
> camera_buffer.h, so we can keep it for now and address both later.
>
> > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(          \
> > +     CameraDevice *const cameraDevice)                               \
> > +     : Extensible(std::make_unique<Private>(this, cameraDevice))     \
> > +{                                                                    \
> > +}                                                                    \
> > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()                \
> > +{                                                                    \
> > +}                                                                    \
> > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>&          \
> > +PlatformFrameBufferAllocator::allocate(int halPixelFormat,           \
> > +                                    const libcamera::Size& size,     \
> > +                                    uint32_t usage,                  \
> > +                                    size_t numBuffers)               \
> > +{                                                                    \
> > +     return _d()->allocate(halPixelFormat, size, usage, numBuffers); \
> > +}
> > +
> > +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> > new file mode 100644
> > index 00000000..114c739b
> > --- /dev/null
> > +++ b/src/android/mm/cros_frame_buffer.cpp
> > @@ -0,0 +1,85 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend
>
> s/allocate/Allocate/
>
> > + * using CameraBufferManager
> > + */
> > +
> > +#include "../frame_buffer.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "cros-camera/camera_buffer_manager.h"
> > +
> > +#include "../camera_device.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL)
> > +
> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > +
> > +public:
> > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > +             [[maybe_unused]] CameraDevice *const cameraDevice)
> > +     {
> > +     }
> > +
> > +     ~Private() = default;
> > +
> > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > +     allocate(int halPixelFormat,
> > +              const libcamera::Size &size,
> > +              uint32_t usage,
> > +              size_t numBuffers);

I think to return std::unique_ptr<libcamera::FrameBuffer> so that we
don't have to consider proper numBuffers.
It is necessary to attach something libcamera::FrameBuffer, for
example, cros::ScopedBufferHandle in cros_frame_buffer.
However, libcamera::FrameBuffer is marked final. How should I solve
this problem?
Is it fine to remove the final mark?

-Hiro
> > +
> > +private:
> > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > +     std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> > +};
> > +
> > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > +PlatformFrameBufferAllocator::Private::allocate(
> > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > +     size_t numBuffers)
> > +{
> > +     ASSERT(allocatedBuffers_.empty());
> > +
> > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> > +     for (size_t i = 0; i < numBuffers; ++i) {
> > +             cros::ScopedBufferHandle handle =
> > +                     cros::CameraBufferManager::AllocateScopedBuffer(
> > +                             size.width, size.height, halPixelFormat, usage);
> > +             if (!handle) {
> > +                     LOG(HAL, Error) << "Failed allocate buffer_handle";
>
> s/Failed/Failed to/
>
> > +                     return allocatedBuffers_;
>
> I think
>
>                         return {};
>
> would be more explicit. For a moment I thought the function would return
> a partial vector of some of the allocations succeeded. Same below.
>
> > +             }
> > +
> > +             const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > +             for (size_t j = 0; j < numPlanes; ++j) {
>
> This could also be written
>
>                 for (auto [j, plane] : utils::enumerate(planes)) {
>
> Up to you.
>
> > +                     FileDescriptor fd{ (*handle)->data[0] };
> > +                     if (!fd.isValid()) {
> > +                             LOG(HAL, Fatal) << "Invalid fd";
> > +                             return allocatedBuffers_;
> > +                     }
>
> You can move this outside of the loop. FileDescriptor uses implicit
> sharing of the actual fd, so all instances of planes[j].fd will use the
> same numerical fd, avoiding the dup() call and saving file descriptor
> space.
>
> > +
> > +                     planes[j].fd = fd;
> > +                     planes[j].offset =
> > +                             cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> > +                     planes[j].length =
> > +                             cros::CameraBufferManager::GetPlaneSize(*handle, j);
> > +             }
> > +
> > +             buffers.push_back(std::make_unique<FrameBuffer>(planes));
> > +             allocatedHandles_.push_back(std::move(handle));
> > +     }
> > +
> > +     allocatedBuffers_ = std::move(buffers);
> > +     return allocatedBuffers_;
> > +}
> > +
> > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> > new file mode 100644
> > index 00000000..b387d5a2
> > --- /dev/null
> > +++ b/src/android/mm/generic_frame_buffer.cpp
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> > + * buffer backend
> > + */
> > +
> > +#include "../frame_buffer.h"
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/internal/formats.h>
> > +
> > +#include <hardware/camera3.h>
> > +#include <hardware/gralloc.h>
> > +#include <hardware/hardware.h>
> > +
> > +#include "../camera_device.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL)
> > +
> > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > +
> > +public:
> > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > +             CameraDevice *const cameraDevice)
> > +             : cameraDevice_(cameraDevice),
> > +               hardwareModule_(cameraDevice->camera3Device()->common.module)
>
> allocDevice_ should be initialized to nullptr or the destructor could
> end up calling gralloc_close() on a random pointer.
>
> > +     {
> > +             ASSERT(!!hardwareModule_);
>
> Why not
>
>                 ASSERT(hardwareModule_);
>
> ?
>
> > +     }
> > +
> > +     ~Private() override;
> > +
> > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > +     allocate(int halPixelFormat,
> > +              const libcamera::Size &size,
> > +              uint32_t usage,
> > +              size_t numBuffers);
> > +
> > +private:
> > +     const CameraDevice *const cameraDevice_;
> > +     struct hw_module_t *const hardwareModule_;
> > +     struct alloc_device_t *allocDevice_;
> > +
> > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > +     std::vector<buffer_handle_t> bufferHandles_;
> > +};
> > +
> > +PlatformFrameBufferAllocator::Private::~Private()
> > +{
> > +     for (buffer_handle_t &handle : bufferHandles_) {
> > +             ASSERT(allocDevice_);
>
> I would drop the assert, this really can't happen.
>
> > +             allocDevice_->free(allocDevice_, handle);
> > +     }
>
> A blank line would be nice.
>
> > +     if (allocDevice_)
> > +             gralloc_close(allocDevice_);
> > +}
> > +
> > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > +PlatformFrameBufferAllocator::Private::allocate(
> > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > +     size_t numBuffers)
> > +{
> > +     ASSERT(allocatedBuffers_.empty());
> > +     ASSERT(bufferHandles_.empty());
> > +     ASSERT(!allocDevice_);
> > +
> > +     int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > +     if (ret) {
> > +             LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> > +             return allocatedBuffers_;
>
> Same here,
>
>                 return {};
>
> > +     }
> > +
> > +     int stride = 0;
> > +     for (size_t i = 0; i < numBuffers; ++i) {
> > +             buffer_handle_t handle{};
> > +             ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > +                                       halPixelFormat, usage, &handle, &stride);
>
> I suppose the stride is guaranteed to be the same for all allocations ?
>
> > +             if (ret) {
> > +                     LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > +                     return allocatedBuffers_;
> > +             }
> > +
> > +             bufferHandles_.push_back(handle);
> > +     }
> > +
> > +     const libcamera::PixelFormat pixelFormat =
> > +             cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > +     const auto &info = PixelFormatInfo::info(pixelFormat);
> > +     const unsigned int numPlanes = info.numPlanes();
> > +
> > +     allocatedBuffers_.reserve(numBuffers);
> > +     for (const buffer_handle_t &handle : bufferHandles_) {
> > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > +             size_t offset = 0;
> > +             for (size_t i = 0; i < numPlanes; ++i) {
>
> Here too you could use utils::enumerate().
>
> > +                     planes[i].fd = FileDescriptor(handle->data[i]);
> > +                     size_t planeSize = info.planeSize(size.height, i, stride);
> > +
> > +                     planes[i].offset = offset;
>
> Does Android use a different dmabuf fd for each plane ? If not, I think
> the offset should be 0, and if it does, we could move the FileDescriptor
> construction outside of the loop.
>
> > +                     planes[i].length = planeSize;
> > +                     offset += planeSize;
> > +             }
> > +
> > +             allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> > +     }
> > +
> > +     return allocatedBuffers_;
> > +}
> > +
> > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > index eeb5cc2e..d1ad64d9 100644
> > --- a/src/android/mm/meson.build
> > +++ b/src/android/mm/meson.build
> > @@ -2,8 +2,10 @@
> >
> >  platform = get_option('android_platform')
> >  if platform == 'generic'
> > -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> > +    android_hal_sources += files(['generic_camera_buffer.cpp',
> > +                                  'generic_frame_buffer.cpp'])
> >  elif platform == 'cros'
> > -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> > +    android_hal_sources += files(['cros_camera_buffer.cpp',
> > +                                  'cros_frame_buffer.cpp'])
> >      android_deps += [dependency('libcros_camera')]
> >  endif
>
> --
> Regards,
>
> Laurent Pinchart

Laurent, gentle ping.

On Wed, Oct 20, 2021 at 1:01 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Hi Laurent, thank you for comments.
>
> On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > 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@chromium.org>
> > > ---
> > >  src/android/frame_buffer.h              |  53 +++++++++++
> > >  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
> > >  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> > >  src/android/mm/meson.build              |   6 +-
> > >  4 files changed, 258 insertions(+), 2 deletions(-)
> > >  create mode 100644 src/android/frame_buffer.h
> > >  create mode 100644 src/android/mm/cros_frame_buffer.cpp
> > >  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> > >
> > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> > > new file mode 100644
> > > index 00000000..6aafeaf3
> > > --- /dev/null
> > > +++ b/src/android/frame_buffer.h
> > > @@ -0,0 +1,53 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * frame_buffer.h - Frame buffer allocating interface definition
> >
> > s/allocating/allocator/
> >
> > > + */
> > > +#ifndef __ANDROID_FRAME_BUFFER_H__
> > > +#define __ANDROID_FRAME_BUFFER_H__
> > > +
> > > +#include <memory>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/framebuffer.h>
> > > +#include <libcamera/geometry.h>
> > > +
> > > +class CameraDevice;
> > > +
> > > +class PlatformFrameBufferAllocator : libcamera::Extensible
> > > +{
> > > +     LIBCAMERA_DECLARE_PRIVATE()
> > > +
> > > +public:
> > > +     explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> > > +     ~PlatformFrameBufferAllocator();
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
> > > +};
> > > +
> > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION                           \
> >
> > I'm not a big fan of this design pattern, but it already exists in
> > camera_buffer.h, so we can keep it for now and address both later.
> >
> > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(          \
> > > +     CameraDevice *const cameraDevice)                               \
> > > +     : Extensible(std::make_unique<Private>(this, cameraDevice))     \
> > > +{                                                                    \
> > > +}                                                                    \
> > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()                \
> > > +{                                                                    \
> > > +}                                                                    \
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>&          \
> > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat,           \
> > > +                                    const libcamera::Size& size,     \
> > > +                                    uint32_t usage,                  \
> > > +                                    size_t numBuffers)               \
> > > +{                                                                    \
> > > +     return _d()->allocate(halPixelFormat, size, usage, numBuffers); \
> > > +}
> > > +
> > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> > > new file mode 100644
> > > index 00000000..114c739b
> > > --- /dev/null
> > > +++ b/src/android/mm/cros_frame_buffer.cpp
> > > @@ -0,0 +1,85 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend
> >
> > s/allocate/Allocate/
> >
> > > + * using CameraBufferManager
> > > + */
> > > +
> > > +#include "../frame_buffer.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include "cros-camera/camera_buffer_manager.h"
> > > +
> > > +#include "../camera_device.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL)
> > > +
> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > +{
> > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > +
> > > +public:
> > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > +             [[maybe_unused]] CameraDevice *const cameraDevice)
> > > +     {
> > > +     }
> > > +
> > > +     ~Private() = default;
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
>
> I think to return std::unique_ptr<libcamera::FrameBuffer> so that we
> don't have to consider proper numBuffers.
> It is necessary to attach something libcamera::FrameBuffer, for
> example, cros::ScopedBufferHandle in cros_frame_buffer.
> However, libcamera::FrameBuffer is marked final. How should I solve
> this problem?
> Is it fine to remove the final mark?
>
> -Hiro
> > > +
> > > +private:
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > +     std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> > > +};
> > > +
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +PlatformFrameBufferAllocator::Private::allocate(
> > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > +     size_t numBuffers)
> > > +{
> > > +     ASSERT(allocatedBuffers_.empty());
> > > +
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > +             cros::ScopedBufferHandle handle =
> > > +                     cros::CameraBufferManager::AllocateScopedBuffer(
> > > +                             size.width, size.height, halPixelFormat, usage);
> > > +             if (!handle) {
> > > +                     LOG(HAL, Error) << "Failed allocate buffer_handle";
> >
> > s/Failed/Failed to/
> >
> > > +                     return allocatedBuffers_;
> >
> > I think
> >
> >                         return {};
> >
> > would be more explicit. For a moment I thought the function would return
> > a partial vector of some of the allocations succeeded. Same below.
> >
> > > +             }
> > > +
> > > +             const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > +             for (size_t j = 0; j < numPlanes; ++j) {
> >
> > This could also be written
> >
> >                 for (auto [j, plane] : utils::enumerate(planes)) {
> >
> > Up to you.
> >
> > > +                     FileDescriptor fd{ (*handle)->data[0] };
> > > +                     if (!fd.isValid()) {
> > > +                             LOG(HAL, Fatal) << "Invalid fd";
> > > +                             return allocatedBuffers_;
> > > +                     }
> >
> > You can move this outside of the loop. FileDescriptor uses implicit
> > sharing of the actual fd, so all instances of planes[j].fd will use the
> > same numerical fd, avoiding the dup() call and saving file descriptor
> > space.
> >
> > > +
> > > +                     planes[j].fd = fd;
> > > +                     planes[j].offset =
> > > +                             cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> > > +                     planes[j].length =
> > > +                             cros::CameraBufferManager::GetPlaneSize(*handle, j);
> > > +             }
> > > +
> > > +             buffers.push_back(std::make_unique<FrameBuffer>(planes));
> > > +             allocatedHandles_.push_back(std::move(handle));
> > > +     }
> > > +
> > > +     allocatedBuffers_ = std::move(buffers);
> > > +     return allocatedBuffers_;
> > > +}
> > > +
> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> > > new file mode 100644
> > > index 00000000..b387d5a2
> > > --- /dev/null
> > > +++ b/src/android/mm/generic_frame_buffer.cpp
> > > @@ -0,0 +1,116 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> > > + * buffer backend
> > > + */
> > > +
> > > +#include "../frame_buffer.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +#include <libcamera/internal/formats.h>
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <hardware/gralloc.h>
> > > +#include <hardware/hardware.h>
> > > +
> > > +#include "../camera_device.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL)
> > > +
> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > +{
> > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > +
> > > +public:
> > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > +             CameraDevice *const cameraDevice)
> > > +             : cameraDevice_(cameraDevice),
> > > +               hardwareModule_(cameraDevice->camera3Device()->common.module)
> >
> > allocDevice_ should be initialized to nullptr or the destructor could
> > end up calling gralloc_close() on a random pointer.
> >
> > > +     {
> > > +             ASSERT(!!hardwareModule_);
> >
> > Why not
> >
> >                 ASSERT(hardwareModule_);
> >
> > ?
> >
> > > +     }
> > > +
> > > +     ~Private() override;
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
> > > +
> > > +private:
> > > +     const CameraDevice *const cameraDevice_;
> > > +     struct hw_module_t *const hardwareModule_;
> > > +     struct alloc_device_t *allocDevice_;
> > > +
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > +     std::vector<buffer_handle_t> bufferHandles_;
> > > +};
> > > +
> > > +PlatformFrameBufferAllocator::Private::~Private()
> > > +{
> > > +     for (buffer_handle_t &handle : bufferHandles_) {
> > > +             ASSERT(allocDevice_);
> >
> > I would drop the assert, this really can't happen.
> >
> > > +             allocDevice_->free(allocDevice_, handle);
> > > +     }
> >
> > A blank line would be nice.
> >
> > > +     if (allocDevice_)
> > > +             gralloc_close(allocDevice_);
> > > +}
> > > +
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +PlatformFrameBufferAllocator::Private::allocate(
> > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > +     size_t numBuffers)
> > > +{
> > > +     ASSERT(allocatedBuffers_.empty());
> > > +     ASSERT(bufferHandles_.empty());
> > > +     ASSERT(!allocDevice_);
> > > +
> > > +     int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > > +     if (ret) {
> > > +             LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> > > +             return allocatedBuffers_;
> >
> > Same here,
> >
> >                 return {};
> >
> > > +     }
> > > +
> > > +     int stride = 0;
> > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > +             buffer_handle_t handle{};
> > > +             ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > > +                                       halPixelFormat, usage, &handle, &stride);
> >
> > I suppose the stride is guaranteed to be the same for all allocations ?
> >
> > > +             if (ret) {
> > > +                     LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > > +                     return allocatedBuffers_;
> > > +             }
> > > +
> > > +             bufferHandles_.push_back(handle);
> > > +     }
> > > +
> > > +     const libcamera::PixelFormat pixelFormat =
> > > +             cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > > +     const auto &info = PixelFormatInfo::info(pixelFormat);
> > > +     const unsigned int numPlanes = info.numPlanes();
> > > +
> > > +     allocatedBuffers_.reserve(numBuffers);
> > > +     for (const buffer_handle_t &handle : bufferHandles_) {
> > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > +             size_t offset = 0;
> > > +             for (size_t i = 0; i < numPlanes; ++i) {
> >
> > Here too you could use utils::enumerate().
> >
> > > +                     planes[i].fd = FileDescriptor(handle->data[i]);
> > > +                     size_t planeSize = info.planeSize(size.height, i, stride);
> > > +
> > > +                     planes[i].offset = offset;
> >
> > Does Android use a different dmabuf fd for each plane ? If not, I think
> > the offset should be 0, and if it does, we could move the FileDescriptor
> > construction outside of the loop.
> >
> > > +                     planes[i].length = planeSize;
> > > +                     offset += planeSize;
> > > +             }
> > > +
> > > +             allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> > > +     }
> > > +
> > > +     return allocatedBuffers_;
> > > +}
> > > +
> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > > index eeb5cc2e..d1ad64d9 100644
> > > --- a/src/android/mm/meson.build
> > > +++ b/src/android/mm/meson.build
> > > @@ -2,8 +2,10 @@
> > >
> > >  platform = get_option('android_platform')
> > >  if platform == 'generic'
> > > -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> > > +    android_hal_sources += files(['generic_camera_buffer.cpp',
> > > +                                  'generic_frame_buffer.cpp'])
> > >  elif platform == 'cros'
> > > -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> > > +    android_hal_sources += files(['cros_camera_buffer.cpp',
> > > +                                  'cros_frame_buffer.cpp'])
> > >      android_deps += [dependency('libcros_camera')]
> > >  endif
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Hi Hiro,

On Wed, Oct 20, 2021 at 01:01:46PM +0900, Hirokazu Honda wrote:
> On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart wrote:
> > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:
> > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using
> > > cros::CameraBufferManager on ChromeOS and gralloc on non
> > > ChromeOS platform. The allocated FrameBuffer(s) are owned by
> > > PlatformFrameBufferAllocator an destroyed when
> >
> > s/an/and/
> >
> > > PlatformFrameBufferAllocator is destroyed.
> >
> > It would also be useful to explain in the commit message why this is
> > needed.
> >
> > Overall the patch looks pretty good. I think we're reaching a point
> > where we'll have to document Android classes such as this one though,
> > but it doesn't have to be part of this series.
> >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/android/frame_buffer.h              |  53 +++++++++++
> > >  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
> > >  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> > >  src/android/mm/meson.build              |   6 +-
> > >  4 files changed, 258 insertions(+), 2 deletions(-)
> > >  create mode 100644 src/android/frame_buffer.h
> > >  create mode 100644 src/android/mm/cros_frame_buffer.cpp
> > >  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> > >
> > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> > > new file mode 100644
> > > index 00000000..6aafeaf3
> > > --- /dev/null
> > > +++ b/src/android/frame_buffer.h
> > > @@ -0,0 +1,53 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * frame_buffer.h - Frame buffer allocating interface definition
> >
> > s/allocating/allocator/
> >
> > > + */
> > > +#ifndef __ANDROID_FRAME_BUFFER_H__
> > > +#define __ANDROID_FRAME_BUFFER_H__
> > > +
> > > +#include <memory>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/framebuffer.h>
> > > +#include <libcamera/geometry.h>
> > > +
> > > +class CameraDevice;
> > > +
> > > +class PlatformFrameBufferAllocator : libcamera::Extensible
> > > +{
> > > +     LIBCAMERA_DECLARE_PRIVATE()
> > > +
> > > +public:
> > > +     explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> > > +     ~PlatformFrameBufferAllocator();
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
> > > +};
> > > +
> > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION                           \
> >
> > I'm not a big fan of this design pattern, but it already exists in
> > camera_buffer.h, so we can keep it for now and address both later.
> >
> > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(          \
> > > +     CameraDevice *const cameraDevice)                               \
> > > +     : Extensible(std::make_unique<Private>(this, cameraDevice))     \
> > > +{                                                                    \
> > > +}                                                                    \
> > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()                \
> > > +{                                                                    \
> > > +}                                                                    \
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>&          \
> > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat,           \
> > > +                                    const libcamera::Size& size,     \
> > > +                                    uint32_t usage,                  \
> > > +                                    size_t numBuffers)               \
> > > +{                                                                    \
> > > +     return _d()->allocate(halPixelFormat, size, usage, numBuffers); \
> > > +}
> > > +
> > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> > > new file mode 100644
> > > index 00000000..114c739b
> > > --- /dev/null
> > > +++ b/src/android/mm/cros_frame_buffer.cpp
> > > @@ -0,0 +1,85 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend
> >
> > s/allocate/Allocate/
> >
> > > + * using CameraBufferManager
> > > + */
> > > +
> > > +#include "../frame_buffer.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include "cros-camera/camera_buffer_manager.h"
> > > +
> > > +#include "../camera_device.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL)
> > > +
> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > +{
> > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > +
> > > +public:
> > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > +             [[maybe_unused]] CameraDevice *const cameraDevice)
> > > +     {
> > > +     }
> > > +
> > > +     ~Private() = default;
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
> 
> I think to return std::unique_ptr<libcamera::FrameBuffer> so that we
> don't have to consider proper numBuffers.

Allocating buffers individually sounds good, I agree with you. Ideally
we should have the same API for the FrameBufferAllocator, but we can
consolidate the APIs later (FrameBufferAllocator will always have the
limitation that buffers can only be allocated between configure() and
start(), but hopefully in the future we will be able to drop this V4L2
allocation backend and use dmabuf heaps).

> It is necessary to attach something libcamera::FrameBuffer, for
> example, cros::ScopedBufferHandle in cros_frame_buffer.
> However, libcamera::FrameBuffer is marked final. How should I solve
> this problem?
> Is it fine to remove the final mark?

FrameBuffer is an Extensible class, so you can subclass
FrameBuffer::Private and add your custom data there without subclassing
FrameBuffer. We do that with the Camera class already, it's declared as
final, but pipeline handlers subclass Camera::Private.

> > > +
> > > +private:
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > +     std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> > > +};
> > > +
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +PlatformFrameBufferAllocator::Private::allocate(
> > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > +     size_t numBuffers)
> > > +{
> > > +     ASSERT(allocatedBuffers_.empty());
> > > +
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > +             cros::ScopedBufferHandle handle =
> > > +                     cros::CameraBufferManager::AllocateScopedBuffer(
> > > +                             size.width, size.height, halPixelFormat, usage);
> > > +             if (!handle) {
> > > +                     LOG(HAL, Error) << "Failed allocate buffer_handle";
> >
> > s/Failed/Failed to/
> >
> > > +                     return allocatedBuffers_;
> >
> > I think
> >
> >                         return {};
> >
> > would be more explicit. For a moment I thought the function would return
> > a partial vector of some of the allocations succeeded. Same below.
> >
> > > +             }
> > > +
> > > +             const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > +             for (size_t j = 0; j < numPlanes; ++j) {
> >
> > This could also be written
> >
> >                 for (auto [j, plane] : utils::enumerate(planes)) {
> >
> > Up to you.
> >
> > > +                     FileDescriptor fd{ (*handle)->data[0] };
> > > +                     if (!fd.isValid()) {
> > > +                             LOG(HAL, Fatal) << "Invalid fd";
> > > +                             return allocatedBuffers_;
> > > +                     }
> >
> > You can move this outside of the loop. FileDescriptor uses implicit
> > sharing of the actual fd, so all instances of planes[j].fd will use the
> > same numerical fd, avoiding the dup() call and saving file descriptor
> > space.
> >
> > > +
> > > +                     planes[j].fd = fd;
> > > +                     planes[j].offset =
> > > +                             cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> > > +                     planes[j].length =
> > > +                             cros::CameraBufferManager::GetPlaneSize(*handle, j);
> > > +             }
> > > +
> > > +             buffers.push_back(std::make_unique<FrameBuffer>(planes));
> > > +             allocatedHandles_.push_back(std::move(handle));
> > > +     }
> > > +
> > > +     allocatedBuffers_ = std::move(buffers);
> > > +     return allocatedBuffers_;
> > > +}
> > > +
> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> > > new file mode 100644
> > > index 00000000..b387d5a2
> > > --- /dev/null
> > > +++ b/src/android/mm/generic_frame_buffer.cpp
> > > @@ -0,0 +1,116 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> > > + * buffer backend
> > > + */
> > > +
> > > +#include "../frame_buffer.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +#include <libcamera/internal/formats.h>
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <hardware/gralloc.h>
> > > +#include <hardware/hardware.h>
> > > +
> > > +#include "../camera_device.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL)
> > > +
> > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > +{
> > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > +
> > > +public:
> > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > +             CameraDevice *const cameraDevice)
> > > +             : cameraDevice_(cameraDevice),
> > > +               hardwareModule_(cameraDevice->camera3Device()->common.module)
> >
> > allocDevice_ should be initialized to nullptr or the destructor could
> > end up calling gralloc_close() on a random pointer.
> >
> > > +     {
> > > +             ASSERT(!!hardwareModule_);
> >
> > Why not
> >
> >                 ASSERT(hardwareModule_);
> >
> > ?
> >
> > > +     }
> > > +
> > > +     ~Private() override;
> > > +
> > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +     allocate(int halPixelFormat,
> > > +              const libcamera::Size &size,
> > > +              uint32_t usage,
> > > +              size_t numBuffers);
> > > +
> > > +private:
> > > +     const CameraDevice *const cameraDevice_;
> > > +     struct hw_module_t *const hardwareModule_;
> > > +     struct alloc_device_t *allocDevice_;
> > > +
> > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > +     std::vector<buffer_handle_t> bufferHandles_;
> > > +};
> > > +
> > > +PlatformFrameBufferAllocator::Private::~Private()
> > > +{
> > > +     for (buffer_handle_t &handle : bufferHandles_) {
> > > +             ASSERT(allocDevice_);
> >
> > I would drop the assert, this really can't happen.
> >
> > > +             allocDevice_->free(allocDevice_, handle);
> > > +     }
> >
> > A blank line would be nice.
> >
> > > +     if (allocDevice_)
> > > +             gralloc_close(allocDevice_);
> > > +}
> > > +
> > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > +PlatformFrameBufferAllocator::Private::allocate(
> > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > +     size_t numBuffers)
> > > +{
> > > +     ASSERT(allocatedBuffers_.empty());
> > > +     ASSERT(bufferHandles_.empty());
> > > +     ASSERT(!allocDevice_);
> > > +
> > > +     int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > > +     if (ret) {
> > > +             LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> > > +             return allocatedBuffers_;
> >
> > Same here,
> >
> >                 return {};
> >
> > > +     }
> > > +
> > > +     int stride = 0;
> > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > +             buffer_handle_t handle{};
> > > +             ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > > +                                       halPixelFormat, usage, &handle, &stride);
> >
> > I suppose the stride is guaranteed to be the same for all allocations ?
> >
> > > +             if (ret) {
> > > +                     LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > > +                     return allocatedBuffers_;
> > > +             }
> > > +
> > > +             bufferHandles_.push_back(handle);
> > > +     }
> > > +
> > > +     const libcamera::PixelFormat pixelFormat =
> > > +             cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > > +     const auto &info = PixelFormatInfo::info(pixelFormat);
> > > +     const unsigned int numPlanes = info.numPlanes();
> > > +
> > > +     allocatedBuffers_.reserve(numBuffers);
> > > +     for (const buffer_handle_t &handle : bufferHandles_) {
> > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > +             size_t offset = 0;
> > > +             for (size_t i = 0; i < numPlanes; ++i) {
> >
> > Here too you could use utils::enumerate().
> >
> > > +                     planes[i].fd = FileDescriptor(handle->data[i]);
> > > +                     size_t planeSize = info.planeSize(size.height, i, stride);
> > > +
> > > +                     planes[i].offset = offset;
> >
> > Does Android use a different dmabuf fd for each plane ? If not, I think
> > the offset should be 0, and if it does, we could move the FileDescriptor
> > construction outside of the loop.
> >
> > > +                     planes[i].length = planeSize;
> > > +                     offset += planeSize;
> > > +             }
> > > +
> > > +             allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> > > +     }
> > > +
> > > +     return allocatedBuffers_;
> > > +}
> > > +
> > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > > index eeb5cc2e..d1ad64d9 100644
> > > --- a/src/android/mm/meson.build
> > > +++ b/src/android/mm/meson.build
> > > @@ -2,8 +2,10 @@
> > >
> > >  platform = get_option('android_platform')
> > >  if platform == 'generic'
> > > -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> > > +    android_hal_sources += files(['generic_camera_buffer.cpp',
> > > +                                  'generic_frame_buffer.cpp'])
> > >  elif platform == 'cros'
> > > -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> > > +    android_hal_sources += files(['cros_camera_buffer.cpp',
> > > +                                  'cros_frame_buffer.cpp'])
> > >      android_deps += [dependency('libcros_camera')]
> > >  endif

Hi Laurent,

On Thu, Oct 28, 2021 at 9:03 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Wed, Oct 20, 2021 at 01:01:46PM +0900, Hirokazu Honda wrote:
> > On Wed, Oct 13, 2021 at 12:18 PM Laurent Pinchart wrote:
> > > On Mon, Sep 27, 2021 at 07:48:20PM +0900, Hirokazu Honda wrote:
> > > > PlatformFrameBufferAllocator allocates FrameBuffer(s) using
> > > > cros::CameraBufferManager on ChromeOS and gralloc on non
> > > > ChromeOS platform. The allocated FrameBuffer(s) are owned by
> > > > PlatformFrameBufferAllocator an destroyed when
> > >
> > > s/an/and/
> > >
> > > > PlatformFrameBufferAllocator is destroyed.
> > >
> > > It would also be useful to explain in the commit message why this is
> > > needed.
> > >
> > > Overall the patch looks pretty good. I think we're reaching a point
> > > where we'll have to document Android classes such as this one though,
> > > but it doesn't have to be part of this series.
> > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >  src/android/frame_buffer.h              |  53 +++++++++++
> > > >  src/android/mm/cros_frame_buffer.cpp    |  85 +++++++++++++++++
> > > >  src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++
> > > >  src/android/mm/meson.build              |   6 +-
> > > >  4 files changed, 258 insertions(+), 2 deletions(-)
> > > >  create mode 100644 src/android/frame_buffer.h
> > > >  create mode 100644 src/android/mm/cros_frame_buffer.cpp
> > > >  create mode 100644 src/android/mm/generic_frame_buffer.cpp
> > > >
> > > > diff --git a/src/android/frame_buffer.h b/src/android/frame_buffer.h
> > > > new file mode 100644
> > > > index 00000000..6aafeaf3
> > > > --- /dev/null
> > > > +++ b/src/android/frame_buffer.h
> > > > @@ -0,0 +1,53 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * frame_buffer.h - Frame buffer allocating interface definition
> > >
> > > s/allocating/allocator/
> > >
> > > > + */
> > > > +#ifndef __ANDROID_FRAME_BUFFER_H__
> > > > +#define __ANDROID_FRAME_BUFFER_H__
> > > > +
> > > > +#include <memory>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/base/class.h>
> > > > +#include <libcamera/camera.h>
> > > > +#include <libcamera/framebuffer.h>
> > > > +#include <libcamera/geometry.h>
> > > > +
> > > > +class CameraDevice;
> > > > +
> > > > +class PlatformFrameBufferAllocator : libcamera::Extensible
> > > > +{
> > > > +     LIBCAMERA_DECLARE_PRIVATE()
> > > > +
> > > > +public:
> > > > +     explicit PlatformFrameBufferAllocator(CameraDevice *const cameraDevice);
> > > > +     ~PlatformFrameBufferAllocator();
> > > > +
> > > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +     allocate(int halPixelFormat,
> > > > +              const libcamera::Size &size,
> > > > +              uint32_t usage,
> > > > +              size_t numBuffers);
> > > > +};
> > > > +
> > > > +#define PUBLIC_FRAME_BUFFER_IMPLEMENTATION                           \
> > >
> > > I'm not a big fan of this design pattern, but it already exists in
> > > camera_buffer.h, so we can keep it for now and address both later.
> > >
> > > > +PlatformFrameBufferAllocator::PlatformFrameBufferAllocator(          \
> > > > +     CameraDevice *const cameraDevice)                               \
> > > > +     : Extensible(std::make_unique<Private>(this, cameraDevice))     \
> > > > +{                                                                    \
> > > > +}                                                                    \
> > > > +PlatformFrameBufferAllocator::~PlatformFrameBufferAllocator()                \
> > > > +{                                                                    \
> > > > +}                                                                    \
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>>&          \
> > > > +PlatformFrameBufferAllocator::allocate(int halPixelFormat,           \
> > > > +                                    const libcamera::Size& size,     \
> > > > +                                    uint32_t usage,                  \
> > > > +                                    size_t numBuffers)               \
> > > > +{                                                                    \
> > > > +     return _d()->allocate(halPixelFormat, size, usage, numBuffers); \
> > > > +}
> > > > +
> > > > +#endif /* __ANDROID_FRAME_BUFFER_H__ */
> > > > diff --git a/src/android/mm/cros_frame_buffer.cpp b/src/android/mm/cros_frame_buffer.cpp
> > > > new file mode 100644
> > > > index 00000000..114c739b
> > > > --- /dev/null
> > > > +++ b/src/android/mm/cros_frame_buffer.cpp
> > > > @@ -0,0 +1,85 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * cros_frame_buffer.cpp - allocate FrameBuffer for Chromium OS buffer backend
> > >
> > > s/allocate/Allocate/
> > >
> > > > + * using CameraBufferManager
> > > > + */
> > > > +
> > > > +#include "../frame_buffer.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include "cros-camera/camera_buffer_manager.h"
> > > > +
> > > > +#include "../camera_device.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DECLARE_CATEGORY(HAL)
> > > > +
> > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > > +{
> > > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > > +
> > > > +public:
> > > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > > +             [[maybe_unused]] CameraDevice *const cameraDevice)
> > > > +     {
> > > > +     }
> > > > +
> > > > +     ~Private() = default;
> > > > +
> > > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +     allocate(int halPixelFormat,
> > > > +              const libcamera::Size &size,
> > > > +              uint32_t usage,
> > > > +              size_t numBuffers);
> >
> > I think to return std::unique_ptr<libcamera::FrameBuffer> so that we
> > don't have to consider proper numBuffers.
>
> Allocating buffers individually sounds good, I agree with you. Ideally
> we should have the same API for the FrameBufferAllocator, but we can
> consolidate the APIs later (FrameBufferAllocator will always have the
> limitation that buffers can only be allocated between configure() and
> start(), but hopefully in the future we will be able to drop this V4L2
> allocation backend and use dmabuf heaps).
>
> > It is necessary to attach something libcamera::FrameBuffer, for
> > example, cros::ScopedBufferHandle in cros_frame_buffer.
> > However, libcamera::FrameBuffer is marked final. How should I solve
> > this problem?
> > Is it fine to remove the final mark?
>
> FrameBuffer is an Extensible class, so you can subclass
> FrameBuffer::Private and add your custom data there without subclassing
> FrameBuffer. We do that with the Camera class already, it's declared as
> final, but pipeline handlers subclass Camera::Private.
>

Thanks. I didn't notice that. Seems like I am still not get used to
the D-pointer design.

-Hiro

> > > > +
> > > > +private:
> > > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > > +     std::vector<cros::ScopedBufferHandle> allocatedHandles_;
> > > > +};
> > > > +
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +PlatformFrameBufferAllocator::Private::allocate(
> > > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > > +     size_t numBuffers)
> > > > +{
> > > > +     ASSERT(allocatedBuffers_.empty());
> > > > +
> > > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers;
> > > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > > +             cros::ScopedBufferHandle handle =
> > > > +                     cros::CameraBufferManager::AllocateScopedBuffer(
> > > > +                             size.width, size.height, halPixelFormat, usage);
> > > > +             if (!handle) {
> > > > +                     LOG(HAL, Error) << "Failed allocate buffer_handle";
> > >
> > > s/Failed/Failed to/
> > >
> > > > +                     return allocatedBuffers_;
> > >
> > > I think
> > >
> > >                         return {};
> > >
> > > would be more explicit. For a moment I thought the function would return
> > > a partial vector of some of the allocations succeeded. Same below.
> > >
> > > > +             }
> > > > +
> > > > +             const size_t numPlanes = cros::CameraBufferManager::GetNumPlanes(*handle);
> > > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > > +             for (size_t j = 0; j < numPlanes; ++j) {
> > >
> > > This could also be written
> > >
> > >                 for (auto [j, plane] : utils::enumerate(planes)) {
> > >
> > > Up to you.
> > >
> > > > +                     FileDescriptor fd{ (*handle)->data[0] };
> > > > +                     if (!fd.isValid()) {
> > > > +                             LOG(HAL, Fatal) << "Invalid fd";
> > > > +                             return allocatedBuffers_;
> > > > +                     }
> > >
> > > You can move this outside of the loop. FileDescriptor uses implicit
> > > sharing of the actual fd, so all instances of planes[j].fd will use the
> > > same numerical fd, avoiding the dup() call and saving file descriptor
> > > space.
> > >
> > > > +
> > > > +                     planes[j].fd = fd;
> > > > +                     planes[j].offset =
> > > > +                             cros::CameraBufferManager::GetPlaneOffset(*handle, j);
> > > > +                     planes[j].length =
> > > > +                             cros::CameraBufferManager::GetPlaneSize(*handle, j);
> > > > +             }
> > > > +
> > > > +             buffers.push_back(std::make_unique<FrameBuffer>(planes));
> > > > +             allocatedHandles_.push_back(std::move(handle));
> > > > +     }
> > > > +
> > > > +     allocatedBuffers_ = std::move(buffers);
> > > > +     return allocatedBuffers_;
> > > > +}
> > > > +
> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/generic_frame_buffer.cpp b/src/android/mm/generic_frame_buffer.cpp
> > > > new file mode 100644
> > > > index 00000000..b387d5a2
> > > > --- /dev/null
> > > > +++ b/src/android/mm/generic_frame_buffer.cpp
> > > > @@ -0,0 +1,116 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * generic_camera_buffer.cpp - allocate FrameBuffer for Generic Android frame
> > > > + * buffer backend
> > > > + */
> > > > +
> > > > +#include "../frame_buffer.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +#include <libcamera/internal/formats.h>
> > > > +
> > > > +#include <hardware/camera3.h>
> > > > +#include <hardware/gralloc.h>
> > > > +#include <hardware/hardware.h>
> > > > +
> > > > +#include "../camera_device.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DECLARE_CATEGORY(HAL)
> > > > +
> > > > +class PlatformFrameBufferAllocator::Private : public Extensible::Private
> > > > +{
> > > > +     LIBCAMERA_DECLARE_PUBLIC(PlatformFrameBufferAllocator)
> > > > +
> > > > +public:
> > > > +     Private([[maybe_unused]] PlatformFrameBufferAllocator *allocator,
> > > > +             CameraDevice *const cameraDevice)
> > > > +             : cameraDevice_(cameraDevice),
> > > > +               hardwareModule_(cameraDevice->camera3Device()->common.module)
> > >
> > > allocDevice_ should be initialized to nullptr or the destructor could
> > > end up calling gralloc_close() on a random pointer.
> > >
> > > > +     {
> > > > +             ASSERT(!!hardwareModule_);
> > >
> > > Why not
> > >
> > >                 ASSERT(hardwareModule_);
> > >
> > > ?
> > >
> > > > +     }
> > > > +
> > > > +     ~Private() override;
> > > > +
> > > > +     const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +     allocate(int halPixelFormat,
> > > > +              const libcamera::Size &size,
> > > > +              uint32_t usage,
> > > > +              size_t numBuffers);
> > > > +
> > > > +private:
> > > > +     const CameraDevice *const cameraDevice_;
> > > > +     struct hw_module_t *const hardwareModule_;
> > > > +     struct alloc_device_t *allocDevice_;
> > > > +
> > > > +     std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;
> > > > +     std::vector<buffer_handle_t> bufferHandles_;
> > > > +};
> > > > +
> > > > +PlatformFrameBufferAllocator::Private::~Private()
> > > > +{
> > > > +     for (buffer_handle_t &handle : bufferHandles_) {
> > > > +             ASSERT(allocDevice_);
> > >
> > > I would drop the assert, this really can't happen.
> > >
> > > > +             allocDevice_->free(allocDevice_, handle);
> > > > +     }
> > >
> > > A blank line would be nice.
> > >
> > > > +     if (allocDevice_)
> > > > +             gralloc_close(allocDevice_);
> > > > +}
> > > > +
> > > > +const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &
> > > > +PlatformFrameBufferAllocator::Private::allocate(
> > > > +     int halPixelFormat, const libcamera::Size &size, uint32_t usage,
> > > > +     size_t numBuffers)
> > > > +{
> > > > +     ASSERT(allocatedBuffers_.empty());
> > > > +     ASSERT(bufferHandles_.empty());
> > > > +     ASSERT(!allocDevice_);
> > > > +
> > > > +     int ret = gralloc_open(hardwareModule_, &allocDevice_);
> > > > +     if (ret) {
> > > > +             LOG(HAL, Error) << "gralloc_open() failed: " << ret;
> > > > +             return allocatedBuffers_;
> > >
> > > Same here,
> > >
> > >                 return {};
> > >
> > > > +     }
> > > > +
> > > > +     int stride = 0;
> > > > +     for (size_t i = 0; i < numBuffers; ++i) {
> > > > +             buffer_handle_t handle{};
> > > > +             ret = allocDevice_->alloc(allocDevice_, size.width, size.height,
> > > > +                                       halPixelFormat, usage, &handle, &stride);
> > >
> > > I suppose the stride is guaranteed to be the same for all allocations ?
> > >
> > > > +             if (ret) {
> > > > +                     LOG(HAL, Error) << "failed buffer allocating: " << ret;
> > > > +                     return allocatedBuffers_;
> > > > +             }
> > > > +
> > > > +             bufferHandles_.push_back(handle);
> > > > +     }
> > > > +
> > > > +     const libcamera::PixelFormat pixelFormat =
> > > > +             cameraDevice_->capabilities()->toPixelFormat(halPixelFormat);
> > > > +     const auto &info = PixelFormatInfo::info(pixelFormat);
> > > > +     const unsigned int numPlanes = info.numPlanes();
> > > > +
> > > > +     allocatedBuffers_.reserve(numBuffers);
> > > > +     for (const buffer_handle_t &handle : bufferHandles_) {
> > > > +             std::vector<FrameBuffer::Plane> planes(numPlanes);
> > > > +             size_t offset = 0;
> > > > +             for (size_t i = 0; i < numPlanes; ++i) {
> > >
> > > Here too you could use utils::enumerate().
> > >
> > > > +                     planes[i].fd = FileDescriptor(handle->data[i]);
> > > > +                     size_t planeSize = info.planeSize(size.height, i, stride);
> > > > +
> > > > +                     planes[i].offset = offset;
> > >
> > > Does Android use a different dmabuf fd for each plane ? If not, I think
> > > the offset should be 0, and if it does, we could move the FileDescriptor
> > > construction outside of the loop.
> > >
> > > > +                     planes[i].length = planeSize;
> > > > +                     offset += planeSize;
> > > > +             }
> > > > +
> > > > +             allocatedBuffers_.push_back(std::make_unique<FrameBuffer>(planes));
> > > > +     }
> > > > +
> > > > +     return allocatedBuffers_;
> > > > +}
> > > > +
> > > > +PUBLIC_FRAME_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > > > index eeb5cc2e..d1ad64d9 100644
> > > > --- a/src/android/mm/meson.build
> > > > +++ b/src/android/mm/meson.build
> > > > @@ -2,8 +2,10 @@
> > > >
> > > >  platform = get_option('android_platform')
> > > >  if platform == 'generic'
> > > > -    android_hal_sources += files(['generic_camera_buffer.cpp'])
> > > > +    android_hal_sources += files(['generic_camera_buffer.cpp',
> > > > +                                  'generic_frame_buffer.cpp'])
> > > >  elif platform == 'cros'
> > > > -    android_hal_sources += files(['cros_camera_buffer.cpp'])
> > > > +    android_hal_sources += files(['cros_camera_buffer.cpp',
> > > > +                                  'cros_frame_buffer.cpp'])
> > > >      android_deps += [dependency('libcros_camera')]
> > > >  endif
>
> --
> Regards,
>
> Laurent Pinchart