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

[Mattijs Korpershoek]

Hi Jacopo,

Thank you for your review.

On ven., sept. 22, 2023 at 11:07, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:

> Hi Mattijs
>
> On Tue, Sep 12, 2023 at 04:15:22PM +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>
>> ---
>>  .../libs/ui/include/ui/GraphicBufferAllocator.h    | 30 ------------
>>  src/android/mm/graphic_buffer_allocator_stub.cpp   | 53 ++++++++++++++++++++++
>>  src/android/mm/meson.build                         |  1 +
>>  3 files changed, 54 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>
>
> ack, I tried moving Mutex.h as imported by 1/4 but then Singleton.h
> fails, so it seems all headers imported in 1/4 are needed! good

Perfect, thank you for double-checking.

>
>
>>  #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;
>
> I might be confused here. This is fine when building on Linux with the
> stub you have provided, but what happens when building on Android ? I
> presume the system-wide header takes precendece ?

So I have not tried it with meson+NDK build so in the case of an in-AOSP
tree (with a Android.bp), the system header takes precedence indeed.

>
>>  };
>>
>>  // ---------------------------------------------------------------------------
>> 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..814b3d0e38bd
>> --- /dev/null
>> +++ b/src/android/mm/graphic_buffer_allocator_stub.cpp
>> @@ -0,0 +1,53 @@
>> +/* 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"
>> +#include <ui/GraphicBufferAllocator.h>
>> +#pragma GCC diagnostic pop
>> +
>> +namespace android {
>> +
>> +ANDROID_SINGLETON_STATIC_INSTANCE(GraphicBufferAllocator)
>> +
>> +GraphicBufferAllocator::GraphicBufferAllocator()
>> +{
>> +}
>
> nit: empty line maybe ?

Will do in v2.

>
>> +GraphicBufferAllocator::~GraphicBufferAllocator()
>> +{
>> +}
>> +
>> +uint64_t GraphicBufferAllocator::getTotalSize() const
>> +{
>> +	return 0;
>> +}
>> +
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wunused-parameter"
>> +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..e9ceb3afba67 100644
>> --- a/src/android/mm/meson.build
>> +++ b/src/android/mm/meson.build
>> @@ -11,6 +11,7 @@ if platform == 'generic'
>>          android_deps += [libhardware]
>>      else
>>          android_hal_sources += files(['libhardware_stub.c'])
>> +        android_hal_sources += files(['graphic_buffer_allocator_stub.cpp'])
>
> Shouldn't this be done later ?

Yeah you're right it feels odd to add the stub as part of the sources
when looking for libhardware.

Actually I think I should include the meson build logic looking for
libui from patch 4 into this patch.

I will do that in v2 and keep you're reviewed-by if that's ok.

>
> All minors:
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
>   j
>
>>      endif
>>  elif platform == 'cros'
>>      android_hal_sources += files(['cros_camera_buffer.cpp',
>>
>> --
>> 2.41.0
>>