[libcamera-devel] [PATCH v3 3/4] android: Stub GraphicBufferAllocator for build tests

[Mattijs Korpershoek]

Hi Laurent,

Thank you for your review.

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

> Hi Mattijs,
>
> Thank you for the patch.
>
> On Sat, Sep 23, 2023 at 06:23:33PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> If we want to keep building libcamera on traditional Linux systems with:
>>   -Dandroid=enabled -Dandroid_platform=generic
>> 
>> We should stub GraphicBufferAllocator, which is not available.
>> It's only available when building with the VNDK or when building within the
>> AOSP tree.
>> 
>> Also remove some deprecated methods and inclusions which are not needed for
>> the stub class.
>> 
>> Note: 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>
>> ---
>>  .../libs/ui/include/ui/GraphicBufferAllocator.h    | 30 ------------
>>  src/android/mm/graphic_buffer_allocator_stub.cpp   | 54 ++++++++++++++++++++++
>>  src/android/mm/meson.build                         |  7 +++
>>  3 files changed, 61 insertions(+), 30 deletions(-)
>> 
>> diff --git a/include/android/frameworks/native/libs/ui/include/ui/GraphicBufferAllocator.h b/include/android/frameworks/native/libs/ui/include/ui/GraphicBufferAllocator.h
>> index e4674d746e37..9eac5bbe8324 100644
>> --- a/include/android/frameworks/native/libs/ui/include/ui/GraphicBufferAllocator.h
>> +++ b/include/android/frameworks/native/libs/ui/include/ui/GraphicBufferAllocator.h
>> @@ -29,15 +29,10 @@
>>  #include <ui/PixelFormat.h>
>>  
>>  #include <utils/Errors.h>
>> -#include <utils/KeyedVector.h>
>> -#include <utils/Mutex.h>
>>  #include <utils/Singleton.h>
>>  
>>  namespace android {
>>  
>> -class GrallocAllocator;
>> -class GraphicBufferMapper;
>> -
>>  class GraphicBufferAllocator : public Singleton<GraphicBufferAllocator>
>>  {
>>  public:
>> @@ -52,25 +47,6 @@ public:
>>                        uint64_t usage, buffer_handle_t* handle, uint32_t* stride,
>>                        std::string requestorName);
>>  
>> -    /**
>> -     * Allocates and does NOT import a gralloc buffer. Buffers cannot be used until they have
>> -     * been imported. This function is for advanced use cases only.
>> -     *
>> -     * The raw native handle must be freed by calling native_handle_close() followed by
>> -     * native_handle_delete().
>> -     */
>> -    status_t allocateRawHandle(uint32_t w, uint32_t h, PixelFormat format, uint32_t layerCount,
>> -                               uint64_t usage, buffer_handle_t* handle, uint32_t* stride,
>> -                               std::string requestorName);
>> -
>> -    /**
>> -     * DEPRECATED: GraphicBufferAllocator does not use the graphicBufferId.
>> -     */
>> -    status_t allocate(uint32_t w, uint32_t h, PixelFormat format,
>> -            uint32_t layerCount, uint64_t usage,
>> -            buffer_handle_t* handle, uint32_t* stride, uint64_t graphicBufferId,
>> -            std::string requestorName);
>> -
>>      status_t free(buffer_handle_t handle);
>>  
>>      uint64_t getTotalSize() const;
>> @@ -94,15 +70,9 @@ protected:
>>                              uint64_t usage, buffer_handle_t* handle, uint32_t* stride,
>>                              std::string requestorName, bool importBuffer);
>>  
>> -    static Mutex sLock;
>> -    static KeyedVector<buffer_handle_t, alloc_rec_t> sAllocList;
>> -
>>      friend class Singleton<GraphicBufferAllocator>;
>>      GraphicBufferAllocator();
>>      ~GraphicBufferAllocator();
>> -
>> -    GraphicBufferMapper& mMapper;
>> -    std::unique_ptr<const GrallocAllocator> mAllocator;
>
> This makes me very uncomfortable. You're changing the class size. It is
> probably harmless here as the fields are at the end, but it's still
> fairly bad practice, and could cause hard to debug issues later.

I understand the concern. I felt a little "ugly" doing this but I could
not come up with a better solution to "compile test" with meson only.

>
> I think we're reaching a point where we may want to require a VNDK to
> compile the Android HAL.

I will try it again to see if it's doable. I found some interesting
links from Igalia who use the cerbero tool from gstreamer to do this:

https://github.com/Igalia/wpe-android

>
>>  };
>>  
>>  // ---------------------------------------------------------------------------
>> diff --git a/src/android/mm/graphic_buffer_allocator_stub.cpp b/src/android/mm/graphic_buffer_allocator_stub.cpp
>> new file mode 100644
>> index 000000000000..7e412c956887
>> --- /dev/null
>> +++ b/src/android/mm/graphic_buffer_allocator_stub.cpp
>> @@ -0,0 +1,54 @@
>> +/* SPDX-License-Identifier: Apache-2.0 */
>> +/*
>> + * Copyright (C) 2023, Ideas on Board
>> + * Copyright (C) 2023, BayLibre
>> + *
>> + * graphic_buffer_allocator_stub.cpp - Android GraphicBufferAllocator
>> + * stub for compile-testing
>> + */
>> +
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wextra-semi"
>
> Could you handle this in meson.build ? Adding pragmas around header
> inclusion isn't nice, especially given that you need to do the same in
> patch 4/4.

I could, but that would force a whole module to have this flag. As far
as I know, it's not possible (by design) to add compiler flags on a
per-file basis.

We discussed this on IRC with Kieran and Jacopo on 12th
september and they seemed fine with it. Should I try to do this on the
whole android/mm directory?

>
>> +#include <ui/GraphicBufferAllocator.h>
>> +#pragma GCC diagnostic pop
>> +
>> +namespace android {
>> +
>> +ANDROID_SINGLETON_STATIC_INSTANCE(GraphicBufferAllocator)
>> +
>> +GraphicBufferAllocator::GraphicBufferAllocator()
>> +{
>> +}
>> +
>> +GraphicBufferAllocator::~GraphicBufferAllocator()
>> +{
>> +}
>> +
>> +uint64_t GraphicBufferAllocator::getTotalSize() const
>> +{
>> +	return 0;
>> +}
>> +
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wunused-parameter"
>
> You can use [[maybe_unused]] instead.

Will do.

>
>> +status_t GraphicBufferAllocator::allocate(uint32_t width,
>> +					  uint32_t height,
>> +					  PixelFormat format,
>> +					  uint32_t layerCount,
>> +					  uint64_t usage,
>> +					  buffer_handle_t *handle,
>> +					  uint32_t *stride,
>> +					  std::string requestorName)
>> +{
>> +	*handle = nullptr;
>> +	*stride = 0;
>> +	return INVALID_OPERATION;
>> +}
>> +
>> +status_t GraphicBufferAllocator::free(buffer_handle_t handle)
>> +{
>> +	return INVALID_OPERATION;
>> +}
>> +#pragma GCC diagnostic pop
>> +
>> +} // namespace android
>> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
>> index e3e0484c3720..4d1fb718e94e 100644
>> --- a/src/android/mm/meson.build
>> +++ b/src/android/mm/meson.build
>> @@ -12,6 +12,13 @@ if platform == 'generic'
>>      else
>>          android_hal_sources += files(['libhardware_stub.c'])
>>      endif
>> +
>> +    libui = dependency('libui', required : false)
>> +    if libui.found()
>> +        android_deps += [libui]
>> +     else
>> +        android_hal_sources += files(['graphic_buffer_allocator_stub.cpp'])
>> +    endif
>>  elif platform == 'cros'
>>      android_hal_sources += files(['cros_camera_buffer.cpp',
>>                                    'cros_frame_buffer_allocator.cpp'])
>> 
>
> -- 
> Regards,
>
> Laurent Pinchart