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

[Laurent Pinchart]

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.

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

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

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