[libcamera-devel] [PATCH v3 4/4] android: mm: generic: Use GraphicBufferAllocator instead of gralloc.h
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Sep 30 15:06:22 CEST 2023
Hi Mattijs,
On Sat, Sep 30, 2023 at 10:18:30AM +0200, Mattijs Korpershoek wrote:
> On dim., sept. 24, 2023 at 19:18, Laurent Pinchart wrote:
> > 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.
We use auto when spelling out the type would be very inconvenient.
Iterators are a very good example.
> >> + 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
More information about the libcamera-devel
mailing list