[libcamera-devel] [PATCH 3/3] cros: Support the new cros camera API with set_up and tear_down

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Feb 25, 2021 at 06:19:17PM +0100, Jacopo Mondi wrote:
> On Thu, Feb 25, 2021 at 07:42:17PM +0900, Paul Elder wrote:
> > Implement and expose the symbol and functions that the new cros camera
> > API requires. Since we don't actually need them, leave them empty.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  src/cros/camera3_hal.cpp  | 21 +++++++++++++++++++++

I'd store this in src/android/ and call it cros_camera3_hal.cpp.

> >  src/cros/meson.build      | 16 ++++++++++++++++
> >  src/libcamera/meson.build |  6 ++++++
> >  src/meson.build           |  4 ++++
> >  4 files changed, 47 insertions(+)
> >  create mode 100644 src/cros/camera3_hal.cpp
> >  create mode 100644 src/cros/meson.build
> >
> > diff --git a/src/cros/camera3_hal.cpp b/src/cros/camera3_hal.cpp
> > new file mode 100644
> > index 00000000..31ad36ac
> > --- /dev/null
> > +++ b/src/cros/camera3_hal.cpp
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera3_hal.cpp - cros-specific components of Android Camera HALv3 module

s/cros/Chrome OS/ (or, if you have to abbreviate it, CrOS.

> > + */
> > +
> > +#include <cros-camera/cros_camera_hal.h>
> > +
> > +static void set_up(cros::CameraMojoChannelManagerToken *token)
> > +{
> > +}
> > +
> > +static void tear_down()
> > +{
> > +}
> > +
> > +cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {
> 
> I'm debated... AFAICT this is just a temporary workaround as the
> dependency on the CROS_CAMERA_HAL_INFO_SYM symbol should not be made
> mandatory in future.
> 
> If that's the case, should we really import all these headers instead
> of defining the two symbols we need and drop this file once the
> requirement is dropped ?

I don't really like importing the headers much. Could we instead include
them from their location in the system ? CrOS uses BUILD.gn files for
its build system, and and it integrates with pkg-config. See the
BUILD.gn in src/platform2/camera/common/. If you define a meson
dependency using the right package, its include directories should be
useable to access headers without importing anything.

> > +	.set_up = set_up,
> > +	.tear_down = tear_down
> > +};
> > diff --git a/src/cros/meson.build b/src/cros/meson.build
> > new file mode 100644
> > index 00000000..31aa58c9
> > --- /dev/null
> > +++ b/src/cros/meson.build
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +
> > +cros_hal_include_dir = '/mnt/host/source/src/aosp/external/libchrome'

See above, we should use pkg-config and meson dependencies.

> > +
> > +cros_hal_info_sources = files([
> > +    'camera3_hal.cpp',
> > +])
> > +
> > +cros_hal_info = static_library('cros_hal_info',
> > +                               cros_hal_info_sources,
> > +                               c_args : '-Wno-shadow',
> > +                               include_directories : [cros_includes,
> > +                                                      cros_hal_include_dir,
> > +                                                      android_includes])
> > +
> > +cros_hal_info_obj = cros_hal_info.extract_objects('camera3_hal.cpp')
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 4b5e33ce..c40eb41f 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -139,6 +139,11 @@ if android_enabled
> >      libcamera_deps += android_deps
> >  endif
> >
> > +libcamera_objects = []
> > +if cros_enabled
> > +    libcamera_objects += cros_hal_info_obj
> > +endif
> > +
> 
> Can't we create a static library object and link against it as we do
> for android_camera_metadata ?

I'm not entirely sure, but in that case I think the linker will only
pull the symbols that are referenced.

> Basically you can define the static library in src/android/meson.build
> and instruct libcamera to link against it conditionally to 'if cros'
> in src/libcamera/meson.build.
> 
> But maybe I'm overlooking something
> 
> >  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> >  # The build_rpath is stripped at install time by meson, so we determine at
> >  # runtime if the library is running from an installed location by checking
> > @@ -149,6 +154,7 @@ libcamera = shared_library('camera',
> >                             install : true,
> >                             link_with : libcamera_link_with,
> >                             include_directories : includes,
> > +                           objects : libcamera_objects,
> >                             build_rpath : '/',
> >                             dependencies : libcamera_deps)
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index 0b26ca70..ec85cc47 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -15,6 +15,10 @@ endif
> >  # are included directly into the libcamera library when this is enabled.
> >  subdir('android')
> >
> > +if cros_enabled
> > +    subdir('cros')
> > +endif
> > +
> >  subdir('libcamera')
> >  subdir('ipa')
> >

-- 
Regards,

Laurent Pinchart