[libcamera-devel] [PATCH v3 1/4] android: Define CHROMEOS macro if android_platform=cros

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Tue, Mar 30, 2021 at 02:25:18PM +0900, Hirokazu Honda wrote:
> Android Camera HAL 3 API used in ChromeOS has a ChromeOS own
> extension, for example, crop_rotate_scale_degrees in
> camera3_stream. To avoid breaking bisection for platforms using
> the API without the extensions, this introduce CHROMEOS macro,
> which is defined if and only if android_platform is 'cros'.

The main purpose of the macro isn't so much to avoid breaking bisection,
but to avoid breaking compilation :-) How about

"As those extensions are not available on Android platforms, introduce a
CHROMEOS macro that can be used to compile CrOS-specific code
conditionally. The macro is defined if and only if android_platform is
'cros'."

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/meson.build   | 6 ++++++
>  src/libcamera/meson.build | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 8e7d07d9..00a3d86b 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -35,6 +35,12 @@ endif
>  
>  android_deps += [libyuv_dep]
>  
> +android_cpp_args = []
> +
> +if get_option('android_platform') == 'cros'
> +   android_cpp_args += [ '-DCHROMEOS']

Looking at
https://chromium.googlesource.com/chromium/src/build/+/refs/heads/master/config/linux/BUILD.gn,
I wonder if we should use OS_CHROMEOS instead of CHROMEOS.

> +endif
> +
>  subdir('cros')
>  
>  android_hal_sources = files([
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 815629db..c0443fd2 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -131,10 +131,13 @@ libcamera_deps = [
>  
>  libcamera_link_with = []
>  
> +libcamera_cpp_args = []
> +

You can declare this in src/meson.build, just above libcamera_objects.

>  if android_enabled
>      libcamera_sources += android_hal_sources
>      includes += android_includes
>      libcamera_link_with += android_camera_metadata
> +    libcamera_cpp_args += android_cpp_args

Then this could be dropped, with and above android_cpp_args replaced
with libcamera_cpp_args.

>  
>      libcamera_deps += android_deps
>  endif
> @@ -144,10 +147,13 @@ endif
>  # runtime if the library is running from an installed location by checking
>  # for the presence or abscence of the dynamic tag.
>  
> +message(libcamera_cpp_args)
> +

Is this a test leftover ?

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  libcamera = shared_library('camera',
>                             libcamera_sources,
>                             install : true,
>                             link_with : libcamera_link_with,
> +                           cpp_args : libcamera_cpp_args,
>                             include_directories : includes,
>                             objects : libcamera_objects,
>                             build_rpath : '/',

-- 
Regards,

Laurent Pinchart