[libcamera-devel] [PATCH v3 0/4] android: switch over to modern gralloc API via libui

[Mattijs Korpershoek]

On lun., sept. 25, 2023 at 10:56, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Mon, Sep 25, 2023 at 09:09:42AM +0200, Mattijs Korpershoek wrote:
>> On dim., sept. 24, 2023 at 18:42, Laurent Pinchart wrote:
>> > On Sat, Sep 23, 2023 at 06:23:30PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> >> gralloc.h is a very old API that has been deprecated at least since
>> >> Android P (9).
>> >> 
>> >> Devices are encouraged to switch over to HIDL interface named
>> >> android.hardware.graphics.allocator@<VERSION>, where <VERSION> can be
>> >> 2.0 ,3.0 or 4.0.
>> >> 
>> >> This is mandatory since Android Q (10) [1]
>> >> 
>> >> Fortunately, Android provides an abstraction on top of
>> >> android.hardware.graphics.allocator which is compatible with each
>> >> version.
>> >> This abstraction is implemented in libui, which is available in the
>> >> VNDK.
>> >
>> > What are the pros and cons of using libui compared to direct usage of
>> > android.hardware.graphics.allocator ?
>> 
>> libui's GraphicBufferAllocator has the following pros compared to
>> android.hardware.graphics.allocator:
>> * Supports multiple version of android.hardware.graphics.allocator (2.0, 3.0, 4.0)
>>   https://cs.android.com/android/platform/superproject/main/+/main:frameworks/native/libs/ui/GraphicBufferAllocator.cpp;l=50
>> 
>>   I suspect (no proof) that future versions of
>>   android.hardware.graphics.allocator will be added there as well.
>> 
>>   Also, the trend seem to be Google migrating from HIDL to AIDL, making
>>   using an abstraction on top seems a better choice
>> 
>> * Is implemented as singleton so very easy to interact with. No need to
>>   query the hwservice manager to retrieve an instance of android.hardware.graphics.allocator
>> 
>> * GraphicBufferAllocator::allocate() and GraphicBufferAllocator::free()
>>   are higher level of abstraction so easier to use.
>> 
>> The cons I can think of are:
>> * GraphicBufferAllocator has more include/dependencies than android.hardware.graphics.allocator.
>>   I have not counted them.
>> 
>> Hope that answers.
>> Should I add this to the cover letter?
>
> Thanks for the explanation. Including it in the cover letter of v4 would
> be nice indeed.

Will do.

>
>> >> Import all necessary headers from AOSP and switch over the
>> >> generic_frame_buffer_allocator to use GraphicBufferAllocator.
>> >> 
>> >> This series has been build-tested on a linux host and functionally
>> >> tested on an AM62x SK EVM with Android 13. (preview and capture).
>> >> 
>> >> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/main/compatibility_matrices/compatibility_matrix.4.xml#195
>> >> 
>> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> >> ---
>> >> Changes in v3:
>> >> - Fixed patch 3 missing whitespace (Jacopo)
>> >> - Added libui meson import build in patch 3 (more consistent) (Jacopo)
>> >> - Removed member reference to GraphicBufferAllocator in patch 4 (Jacopo)
>> >> - Link to v2: https://lists.libcamera.org/pipermail/libcamera-devel/2023-September/038942.html
>> >> 
>> >> Changes in v2:
>> >> - Dropped additional ; in graphic_buffer_allocator_stub.cpp. (Kieran)
>> >> - Surrounded problematic includes with #pragma to avoid clang compile
>> >>   errors related to -Wextra-semi. (Kieran)
>> >> - Link to v1: https://lists.libcamera.org/pipermail/libcamera-devel/2023-September/038927.html
>> >> 
>> >> Tested on linux with:
>> >> CC=clang CXX=clang++ meson setup build -Dandroid=enabled -Dandroid_platform=generic
>> >> ninja -C build
>> >> And clang version clang++ (clang 16.0.6 "clang version 16.0.6 (Fedora 16.0.6-2.fc38)")
>> >> 
>> >> ---
>> >> Mattijs Korpershoek (4):
>> >>       android: Import libutils/libui headers from vndk v33
>> >>       android: Import GraphicBufferAllocator header from vndk v33
>> >>       android: Stub GraphicBufferAllocator for build tests
>> >>       android: mm: generic: Use GraphicBufferAllocator instead of gralloc.h
>> >> 
>> >>  .../libs/ui/include/ui/GraphicBufferAllocator.h    |  81 ++++++++
>> >>  .../native/libs/ui/include/ui/PixelFormat.h        |  75 +++++++
>> >>  include/android/meson.build                        |   2 +
>> >>  .../system/core/libutils/include/utils/Compat.h    |  94 +++++++++
>> >>  .../system/core/libutils/include/utils/Errors.h    |  78 ++++++++
>> >>  .../system/core/libutils/include/utils/Mutex.h     | 219 +++++++++++++++++++++
>> >>  .../system/core/libutils/include/utils/Singleton.h | 102 ++++++++++
>> >>  .../system/core/libutils/include/utils/Timers.h    | 103 ++++++++++
>> >>  src/android/mm/generic_frame_buffer_allocator.cpp  |  68 +++----
>> >>  src/android/mm/graphic_buffer_allocator_stub.cpp   |  54 +++++
>> >>  src/android/mm/libhardware_stub.c                  |  17 --
>> >>  src/android/mm/meson.build                         |  11 +-
>> >>  12 files changed, 835 insertions(+), 69 deletions(-)
>> >> ---
>> >> base-commit: 1d616141420d1f51e5999d758e3e0cc721a46290
>> >> change-id: 20230824-gralloc-api-v4-e3388fd364c6
>
> -- 
> Regards,
>
> Laurent Pinchart