| Message ID | 20230923-gralloc-api-v4-v3-4-9a9e039284ba@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@baylibre.com> > Reviewed-by: Jacopo Mondi <jacopo.mondi@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() >
Hi Laurent, Thank you for your review. On dim., sept. 24, 2023 at 19:18, Laurent Pinchart <laurent.pinchart@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@baylibre.com> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@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
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@baylibre.com> > >> Reviewed-by: Jacopo Mondi <jacopo.mondi@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() > >>
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. * Is it no problem to call alloc/free in parallel? */ - allocDevice_->free(allocDevice_, handle_); + auto &allocator = android::GraphicBufferAllocator::get(); + 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; 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()