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

18998 diff mbox series

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@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(-)

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


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

>  };
>
>  // ---------------------------------------------------------------------------
> 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 ?

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

All minors:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>      endif
>  elif platform == 'cros'
>      android_hal_sources += files(['cros_camera_buffer.cpp',
>
> --
> 2.41.0
>

Hi Jacopo,

Thank you for your review.

On ven., sept. 22, 2023 at 11:07, Jacopo Mondi <jacopo.mondi@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@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@ideasonboard.com>
>
> Thanks
>   j
>
>>      endif
>>  elif platform == 'cros'
>>      android_hal_sources += files(['cros_camera_buffer.cpp',
>>
>> --
>> 2.41.0
>>