[libcamera-devel] [PATCH] subprojects: Add libyuv and built if -Dandroid=enabled

[Hirokazu Honda]

Hi Laurent,

Thanks for reviewing.

On Tue, Jan 26, 2021 at 5:03 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Jan 22, 2021 at 12:06:44PM +0900, Hirokazu Honda wrote:
> > On Wed, Jan 20, 2021 at 4:12 PM Laurent Pinchart wrote:
> > > On Wed, Jan 20, 2021 at 04:34:25AM +0000, Hirokazu Honda wrote:
> > > > Android HAL adaptation layer may need image processing, for
> > > > example, scaling and format conversion. Libyuv is a general image
> > > > processing. This adds libyuv to subprojects, so that it is folked
> > > > locally and can be used with Android HAL implementation code.
> > >
> > > Nice !
> > >
> > > Missing SoB line though.
> > >
> > > > ---
> > > >  meson.build             |  2 +-
> > > >  src/android/meson.build | 14 ++++++++++++++
> > > >  subprojects/.gitignore  |  1 +
> > > >  subprojects/libyuv.wrap |  7 +++++++
> > > >  4 files changed, 23 insertions(+), 1 deletion(-)
> > > >  create mode 100644 subprojects/.gitignore
> > > >  create mode 100644 subprojects/libyuv.wrap
> > > >
> > > > diff --git a/meson.build b/meson.build
> > > > index c47eb420..d9b63278 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -1,7 +1,7 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >
> > > >  project('libcamera', 'c', 'cpp',
> > > > -    meson_version : '>= 0.51',
> > > > +    meson_version : '>= 0.55',
> > >
> > > Debian has now a v0.56 meson backport, and so does Ubuntu 20.10. Ubuntu
> > > 20.04 LTS is still stuck with meson v0.53 :-S If there's no other option
> > > (and looking at the meson.build below we really need
> > > cmake.subproject_options), so be it.
> >
> > Updated README.rst.
> >
> > > >      version : '0.0.0',
> > > >      default_options : [
> > > >          'werror=true',
> > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > index 3d4d3be4..0b55ff63 100644
> > > > --- a/src/android/meson.build
> > > > +++ b/src/android/meson.build
> > > > @@ -5,6 +5,20 @@ android_deps = [
> > > >      dependency('libjpeg', required : get_option('android')),
> > > >  ]
> > > >
> > > > +if get_option('android').enabled()
> > >
> > > Should we move this down after the foreach, and test 'if
> > > android_enabled' instead ? As libyuv is provided as a subproject,
> > > there's no need to take it into account in the android_enabled logic, we
> > > know it will be there, and if the Android HAL isn't enabled due to a
> > > missing libjpeg or libexif, then we would avoid creating the subproject.
> > >
> > > > +   cmake = import('cmake')
> > >
> > > Indentation should use 4 spaces. I'd also add a blank line here.
> > >
> > > > +   libyuv_vars = cmake.subproject_options()
> > > > +   libyuv_vars.set_override_option('cpp_std', 'c++17')
> > > > +   libyuv_vars.append_compile_args('cpp',
> > > > +      '-Wno-sign-compare',
> > > > +      '-Wno-unused-variable',
> > > > +      '-Wno-unused-parameter')
> > > > +   libyuv_vars.append_link_args('-ljpeg')
> > >
> > > And here.
> > >
> > > > +   libyuv = cmake.subproject('libyuv', options : libyuv_vars)
> > > > +   libyuv_dep = libyuv.dependency('yuv_shared')
> > >
> > > And here.
> > >
> > > Wouldn't it be better to link against the status version of libyuv ? It
> > > would avoid the need to install a separate libyuv shared object, which
> >
> > Sorry, I don't understand what you mean.
>
> Sorry, I should have proof-read my mail, I meant static, not status.
>

Aha, thanks.

> > Do you mean we should generate a shared object with a custom string
> > like libyuv-1.0.0.so, so that we can never conflict with other libyuv
> > shared objects if there are?
> >
> > > could conflict with the rest of the system. This seems to require
> > > compiling libyuv with -fPIC, which may require a patch to the libyuv
> > > CMakeList.txt to add
> > >
> > > set_property(TARGET ${ly_lib_static} PROPERTY POSITION_INDEPENDENT_CODE ON)
> > >
> > > unless there's a way to do the same through cmake.subproject_options().
> > > We could host a fork of libyuv to do so, but it would be nice to
> > > upstream PIC support as an option in libyuv.
> > >
> >
> > Wouldn't it be better to link against the status version of libyuv ? It
> > > would avoid the need to install a separate libyuv shared object,
> >
> > Got it. I added
> > libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE':
> > 'ON'}).
>
> Nice !
>
> > > > +   android_deps += [ libyuv_dep, ]
> > >
> > > With this, it seems ninja will link to libyuv correctly. It also passes
> > > corresponding -I options to the compiler:
> > >
> > > [2/73] clang++-9 -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -I../../subprojects/libyuv/include -I../../subprojects/libyuv -I../../../../subprojects/libyuv -Isubprojects/libyuv -Iinclude/libcamera -Iinclude/libcamera/internal -I/usr/include/libexif -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -stdlib=libc++ -Wextra-semi -Wshadow -include config.h -fPIC -pthread -MD -MQ src/libcamera/libcamera.so.p/meson-generated_.._version.cpp.o -MF src/libcamera/libcamera.so.p/meson-generated_.._version.cpp.o.d -o src/libcamera/libcamera.so.p/meson-generated_.._version.cpp.o -c src/libcamera/version.cpp
> > >
> > > But their values is a bit weird:
> > >
> > > -I../../subprojects/libyuv/include: this seems right
> > > -I../../subprojects/libyuv: this isn't needed but shouldn't hurt
> > > -I../../../../subprojects/libyuv: this points to a directory outside of
> > > the sources and build tree, it seems to be a meson bug
> > > -Isubprojects/libyuv: this isn't needed, but should also not hurt
> >
> > After I searched for a few tens of minutes, I couldn't find a way of
> > controlling include directories in this cmake module.
> > Do you have any idea?
>
> I would guess it's a bug in meson. You can ignore it for now.
>

Ack.

-Hiro

> > > > +endif
> > > > +
> > > >  android_enabled = true
> > > >
> > > >  foreach dep : android_deps
> > > > diff --git a/subprojects/.gitignore b/subprojects/.gitignore
> > > > new file mode 100644
> > > > index 00000000..410b8bd6
> > > > --- /dev/null
> > > > +++ b/subprojects/.gitignore
> > > > @@ -0,0 +1 @@
> > > > +/libyuv
> > > > \ No newline at end of file
> > > > diff --git a/subprojects/libyuv.wrap b/subprojects/libyuv.wrap
> > > > new file mode 100644
> > > > index 00000000..4262bf05
> > > > --- /dev/null
> > > > +++ b/subprojects/libyuv.wrap
> > > > @@ -0,0 +1,7 @@
> > > > +[wrap-git]
> > > > +directory = libyuv
> > > > +url = https://chromium.googlesource.com/libyuv/libyuv.git
> > > > +revision = 1d3f901aa016d42b5bc0148be2ef6c0fd56f3b81
> > > > +
> > > > +[provide]
> > > > +libyuv = libyuv_dep
> > >
> > > Do we need the [provide] section ? It seems that the dependency is
> > > handled by the cmake module and doesn't need this, but I could be wrong.
> > >
> > > Overall, this is very nice. I'm OK merging this before the first user of
> > > libyuv in the Android HAL, but could you try to quickly hack a call to a
> > > libyuv function in the code, to make sure the includes are found and the
> > > linking works as expected ?
>
> --
> Regards,
>
> Laurent Pinchart