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

[Hirokazu Honda]

Hi Laurent,

On Wed, Jan 20, 2021 at 4:12 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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.
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'}).

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

Best Regards,
-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