[libcamera-devel] [PATCH v3 3/4] android: Stub GraphicBufferAllocator for build tests
Mattijs Korpershoek
mkorpershoek at baylibre.com
Sat Sep 30 10:11:30 CEST 2023
On dim., sept. 24, 2023 at 19:20, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
> On Sun, Sep 24, 2023 at 07:06:40PM +0300, Laurent Pinchart via libcamera-devel 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 think we're reaching a point where we may want to require a VNDK to
>> compile the Android HAL.
>>
>> > };
>> >
>> > // ---------------------------------------------------------------------------
>> > 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.
>
> After reviewing the whole series, I'm a bit in two minds about this :-S
> It's nice to keep the warning active for the rest of the code, to catch
> issues in the libcamera code base. We shouldn't compile the whole HAL
> without this, but maybe it's fine to compile android/mm only ?
That would mean that we have to move android/mm to a static library
because meson cannot apply per directory/per file build flags (as far as
I know)
See: https://github.com/mesonbuild/meson/issues/1367
Do we want to move this to a static library just not have the pragmas?
I'm OK with both options, let me know.
>
>> > +#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.
>>
>> > +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
More information about the libcamera-devel
mailing list