[libcamera-devel] [PATCH v2 4/4] android: mm: generic: Use GraphicBufferAllocator instead of gralloc.h
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Sep 22 11:27:02 CEST 2023
HI Mattijs
nice work!
On Tue, Sep 12, 2023 at 04:15:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> gralloc.h is a very old API that has been deprecated at least since
> Android P (9).
>
> 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>
> ---
> src/android/mm/generic_frame_buffer_allocator.cpp | 61 ++++++++---------------
> src/android/mm/libhardware_stub.c | 17 -------
> src/android/mm/meson.build | 8 ++-
> 3 files changed, 23 insertions(+), 63 deletions(-)
>
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> index 7ecef2c669df..468579068c32 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>
our version of include/android/system/core/include/system/camera.h
still includes gralloc.h. We should update all headers most probably
> -#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,28 @@ class GenericFrameBufferData : public FrameBuffer::Private
> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>
> public:
> - GenericFrameBufferData(struct alloc_device_t *allocDevice,
> + GenericFrameBufferData(android::GraphicBufferAllocator &allocDevice,
You could
GenericFrameBufferData(buffer_handle_t handle,
const std::vector<FrameBuffer::Plane> &planes)
: FrameBuffer::Private(planes),
allocDevice_(android::GraphicBufferAllocator::get()),
handle_(handle)
Instead of passing it to the constructor of ::Private.
It does't make much different though, up to you!
> buffer_handle_t handle,
> const std::vector<FrameBuffer::Plane> &planes)
> : FrameBuffer::Private(planes), allocDevice_(allocDevice),
> 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.
> - *
nice!
> * \todo Thread safety against alloc_device_t is not documented.
> * Is it no problem to call alloc/free in parallel?
> */
> - allocDevice_->free(allocDevice_, handle_);
> + android::status_t status = allocDevice_.free(handle_);
Or you could even get the singleton instance here!
> + if (status != android::NO_ERROR)
> + LOG(HAL, Error) << "Error freeing framebuffer: " << status;
> }
>
> private:
> - struct alloc_device_t *allocDevice_;
> + android::GraphicBufferAllocator &allocDevice_;
Usually reference class members are worrying to me, but this really
just points to a system-wide component accessed via a singleton if i'm
not mistaken, so this should be fine!
> const buffer_handle_t handle_;
> };
> } /* namespace */
> @@ -73,50 +69,33 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private
> public:
> Private(CameraDevice *const cameraDevice)
> : cameraDevice_(cameraDevice),
> - hardwareModule_(nullptr),
> - allocDevice_(nullptr)
> + allocDevice_(android::GraphicBufferAllocator::get())
> {
> - hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);
> - ASSERT(hardwareModule_);
> }
>
> - ~Private() override;
> + ~Private() = default;
>
> 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_;
> + android::GraphicBufferAllocator &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;
> +
> + android::status_t status = allocDevice_.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) {
> 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 e9ceb3afba67..203b8c3e5804 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -4,13 +4,11 @@ platform = get_option('android_platform')
> if platform == 'generic'
> android_hal_sources += files(['generic_camera_buffer.cpp',
> 'generic_frame_buffer_allocator.cpp'])
> - android_deps += [libdl]
libdl was used for ?
>
> - libhardware = dependency('libhardware', required : false)
> - if libhardware.found()
> - android_deps += [libhardware]
> + libui = dependency('libui', required : false)
> + if libui.found()
> + android_deps += [libui]
> else
> - android_hal_sources += files(['libhardware_stub.c'])
> android_hal_sources += files(['graphic_buffer_allocator_stub.cpp'])
> endif
> elif platform == 'cros'
All nits
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
>
> --
> 2.41.0
>
More information about the libcamera-devel
mailing list