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

[Mattijs Korpershoek]

Hi Laurent,

Thank you for your review.

On dim., sept. 24, 2023 at 19:18, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

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

Yes. As of today libcamera is implemented as a shared library and uses
the AOSP-provided HIDL -> libhardware wrapper located in:
//hardware/interfaces/camera/provider/2.4/default/

This means the following:
* We implement only a subset (version 2.4) of the required features
  according to Android
* Upgrading to the latest and greatest would require to be implemented
  as a process. (each HAL is its own process nowadays)

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

I will check if it's thread-safe (I believe it is but I'm not 100% sure)

>
>>  		 * 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.

Will do, sorry about that. I've seen auto used for iterators in the
libcamera codebase so I thought we could use it for a singleton pattern
as well.

>
>> +		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.

Will do

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