[libcamera-devel] [PATCH v2 4/4] android: mm: generic: Use GraphicBufferAllocator instead of gralloc.h
Mattijs Korpershoek
mkorpershoek at baylibre.com
Sat Sep 23 12:31:29 CEST 2023
Hi Jacopo,
Thank you for your review.
On ven., sept. 22, 2023 at 11:27, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
> 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
That's also the case for the one in aosp main:
https://cs.android.com/android/platform/superproject/main/+/main:system/core/libsystem/include/system/camera.h?q=camera.h%20&ss=android%2Fplatform%2Fsuperproject%2Fmain
How about, instead, we get rid of the unused headers (which were copied
from AOSP) in the libcamera include/android tree instead?
If that seems fine, I will do some cleaning up and get rid of the
"no-longer needed" headers in a new patch for v2.
>
>> -#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!
I will give it some thought and either change it for v2 or put a note in
the cover to explain why I kept it the same.
>
>> 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!
You mean dropping the member and just getting the singleton instance with
android::GraphicBufferAllocator::get() ?
That might be even simpler and avoid adding an additional member indeed.
>
>> + 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!
Yes, that is correct. I will study to remove the member for v2.
>
>
>> 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 ?
for dlclose() call done in PlatformFrameBufferAllocator::Private::~Private().
For some background of why this was needed, refer to:
* 1450e09a0839 ("android: mm: generic: use GRALLOC_HARDWARE_MODULE_ID")
* https://patchwork.libcamera.org/patch/18309/#26496
>
>>
>> - 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