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

[Hirokazu Honda]

Laurent, gentle ping :)

On Fri, Jan 22, 2021 at 12:06 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> 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