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

11378 diff mbox series

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@ideasonboard.com>
---
 src/cros/camera3_hal.cpp  | 21 +++++++++++++++++++++
 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

Hi Paul,

a few drive by comments:

On 25/02/2021 10:42, 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@ideasonboard.com>
> ---
>  src/cros/camera3_hal.cpp  | 21 +++++++++++++++++++++
>  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

ChromeOS specific ?

> + */
> +
> +#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 = {
> +	.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'
> +
> +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
> +
>  # 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

Can this be the same style as the other subdirs, where it's just
subdir('cros') here, but the subdir itself returns early if
!cros_enabled or such?


> +
>  subdir('libcamera')
>  subdir('ipa')
>  
>

Hi Paul,

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@ideasonboard.com>
> ---
>  src/cros/camera3_hal.cpp  | 21 +++++++++++++++++++++
>  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
> + */
> +
> +#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 ?

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

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')
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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')
> >