[libcamera-devel] [PATCH v3 4/4] android: mm: generic: Use GraphicBufferAllocator instead of gralloc.h

[Laurent Pinchart]

Hi Mattijs,

Thank you for the patch.

On Sat, Sep 23, 2023 at 06:23:34PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> gralloc.h is a very old API that has been deprecated at least since
> Android P (9).

It's interesting that the latest versions of camera.h and
camera_common.h still include gralloc.h. I suppose that's because those
headers are deprecated too, we should implement the HIDL version of the
camera HAL API. This will be interesting development, especially when
compiling against the VNDK and not as part of AOSP.

> Switch over to a higher level abstraction of gralloc from libui, which
> is compatible with Android 11 and up.
> Libui:
> * is provided in the VNDK (so it's available to vendors).
> * is also used in the camera vts test named VtsAidlHalCameraProvider_TargetTest.
> 
> Drop the libhardware stub since we no longer need it.
> 
> Notes:
> * GraphicsBufferAllocator being a Singleton, buffer lifecycle
>   management is easier.
> * The imported headers from Android generate the -Wextra-semi warning.
>   To avoid patching the files, a pragma has been added before inclusion.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  src/android/mm/generic_frame_buffer_allocator.cpp | 68 ++++++++---------------
>  src/android/mm/libhardware_stub.c                 | 17 ------
>  src/android/mm/meson.build                        |  8 ---
>  3 files changed, 22 insertions(+), 71 deletions(-)
> 
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> index 7ecef2c669df..1f2fbe334f2c 100644
> --- a/src/android/mm/generic_frame_buffer_allocator.cpp
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> @@ -16,8 +16,11 @@
>  #include "libcamera/internal/framebuffer.h"
>  
>  #include <hardware/camera3.h>
> -#include <hardware/gralloc.h>
> -#include <hardware/hardware.h>
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wextra-semi"
> +#include <ui/GraphicBufferAllocator.h>
> +#pragma GCC diagnostic pop
> +#include <utils/Errors.h>
>  
>  #include "../camera_device.h"
>  #include "../frame_buffer_allocator.h"
> @@ -33,35 +36,26 @@ class GenericFrameBufferData : public FrameBuffer::Private
>  	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>  
>  public:
> -	GenericFrameBufferData(struct alloc_device_t *allocDevice,
> -			       buffer_handle_t handle,
> +	GenericFrameBufferData(buffer_handle_t handle,
>  			       const std::vector<FrameBuffer::Plane> &planes)
> -		: FrameBuffer::Private(planes), allocDevice_(allocDevice),
> -		  handle_(handle)
> +		: FrameBuffer::Private(planes), handle_(handle)
>  	{
> -		ASSERT(allocDevice_);
>  		ASSERT(handle_);
>  	}
>  
>  	~GenericFrameBufferData() override
>  	{
>  		/*
> -		 * allocDevice_ is used to destroy handle_. allocDevice_ is
> -		 * owned by PlatformFrameBufferAllocator::Private.
> -		 * GenericFrameBufferData must be destroyed before it is
> -		 * destroyed.
> -		 *
> -		 * \todo Consider managing alloc_device_t with std::shared_ptr
> -		 * if this is difficult to maintain.
> -		 *
>  		 * \todo Thread safety against alloc_device_t is not documented.

This comment needs to be updated, or removed if GraphicBufferAllocator
is thread-safe.

>  		 * Is it no problem to call alloc/free in parallel?
>  		 */
> -		allocDevice_->free(allocDevice_, handle_);
> +		auto &allocator = android::GraphicBufferAllocator::get();

Please indicate the type explicitly, auto hinders readability. Same
below.

> +		android::status_t status = allocator.free(handle_);
> +		if (status != android::NO_ERROR)
> +			LOG(HAL, Error) << "Error freeing framebuffer: " << status;
>  	}
>  
>  private:
> -	struct alloc_device_t *allocDevice_;
>  	const buffer_handle_t handle_;
>  };
>  } /* namespace */
> @@ -72,51 +66,33 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private
>  
>  public:
>  	Private(CameraDevice *const cameraDevice)
> -		: cameraDevice_(cameraDevice),
> -		  hardwareModule_(nullptr),
> -		  allocDevice_(nullptr)
> +		: cameraDevice_(cameraDevice)
>  	{
> -		hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);
> -		ASSERT(hardwareModule_);
>  	}
>  
> -	~Private() override;
> +	~Private() = default;

I think you can drop the destructor completely.

>  
>  	std::unique_ptr<HALFrameBuffer>
>  	allocate(int halPixelFormat, const libcamera::Size &size, uint32_t usage);
>  
>  private:
>  	const CameraDevice *const cameraDevice_;
> -	const struct hw_module_t *hardwareModule_;
> -	struct alloc_device_t *allocDevice_;
>  };
>  
> -PlatformFrameBufferAllocator::Private::~Private()
> -{
> -	if (allocDevice_)
> -		gralloc_close(allocDevice_);
> -	dlclose(hardwareModule_->dso);
> -}
> -
>  std::unique_ptr<HALFrameBuffer>
>  PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
>  						const libcamera::Size &size,
>  						uint32_t usage)
>  {
> -	if (!allocDevice_) {
> -		int ret = gralloc_open(hardwareModule_, &allocDevice_);
> -		if (ret) {
> -			LOG(HAL, Fatal) << "gralloc_open() failed: " << ret;
> -			return nullptr;
> -		}
> -	}
> -
> -	int stride = 0;
> +	uint32_t 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 allocation: " << ret;
> +
> +	auto &allocator = android::GraphicBufferAllocator::get();
> +	android::status_t status = allocator.allocate(size.width, size.height, halPixelFormat,
> +						      1 /*layerCount*/, usage, &handle, &stride,
> +						      "libcameraHAL");
> +	if (status != android::NO_ERROR) {
> +		LOG(HAL, Error) << "failed buffer allocation: " << status;
>  		return nullptr;
>  	}
>  	if (!handle) {
> @@ -143,7 +119,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
>  
>  	return std::make_unique<HALFrameBuffer>(
>  		std::make_unique<GenericFrameBufferData>(
> -			allocDevice_, handle, planes),
> +			handle, planes),
>  		handle);
>  }
>  
> diff --git a/src/android/mm/libhardware_stub.c b/src/android/mm/libhardware_stub.c
> deleted file mode 100644
> index 00f15cd90cac..000000000000
> --- a/src/android/mm/libhardware_stub.c
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -/* SPDX-License-Identifier: Apache-2.0 */
> -/*
> - * Copyright (C) 2023, Ideas on Board
> - *
> - * libhardware_stub.c - Android libhardware stub for test compilation
> - */
> -
> -#include <errno.h>
> -
> -#include <hardware/hardware.h>
> -
> -int hw_get_module(const char *id __attribute__((__unused__)),
> -		  const struct hw_module_t **module)
> -{
> -	*module = NULL;
> -	return -ENOTSUP;
> -}
> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> index 4d1fb718e94e..301a2622008b 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -4,14 +4,6 @@ platform = get_option('android_platform')
>  if platform == 'generic'
>      android_hal_sources += files(['generic_camera_buffer.cpp',
>                                    'generic_frame_buffer_allocator.cpp'])
> -    android_deps += [libdl]
> -
> -    libhardware = dependency('libhardware', required : false)
> -    if libhardware.found()
> -        android_deps += [libhardware]
> -    else
> -        android_hal_sources += files(['libhardware_stub.c'])
> -    endif
>  
>      libui = dependency('libui', required : false)
>      if libui.found()
> 

-- 
Regards,

Laurent Pinchart