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

[Mattijs Korpershoek]

Hi Laurent,

Thank you for your review.

On dim., sept. 24, 2023 at 18:42, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> Hi Mattijs,
>
> Thank you for the series.
>
> 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?

>
>> 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