[libcamera-devel] [PATCH] android: Make libyaml dependency optional
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 29 15:37:16 CET 2021
Hi Roman,
On Wed, Dec 29, 2021 at 12:24:48PM +0200, Roman Stratiienko wrote:
> ср, 29 дек. 2021 г. в 12:14, Laurent Pinchart:
> > On Wed, Dec 29, 2021 at 11:45:17AM +0200, Roman Stratiienko wrote:
> > > вт, 28 дек. 2021 г. в 20:27, Laurent Pinchart:
> > > > On Tue, Dec 28, 2021 at 01:22:34PM +0200, Roman Stratiienko wrote:
> > > > > вт, 28 дек. 2021 г., 13:04 Jacopo Mondi:
> > > > > > Hi Roman,
> > > > > >
> > > > > > cc Hanlin Chen from ChromiumOS as he's planning to extend
> > > > > > configuration file support for the ChromeOS Android HAL.
> > > > >
> > > > > > Before looking into the patch: is there any hope libyaml might become
> > > > > > part of the standard AOSP distribution (or a process in place to request
> > > > > > that)
> > > > >
> > > > > I am not aware of that, and I haven't initiated such a process. It will be
> > > > > a very slow and bumpy road.
> > > > >
> > > > > Not sure what is planned for these properties in future, but now it is not
> > > > > necessary to have it in libcamera to have fully functional hal. Also having
> > > > > no configuration file available will encourage developers to add
> > > > > corresponding records to dts, which is also a great thing to align with
> > > > > linux world.
> > > >
> > > > Our goals align here. This is exactly why we have decided not to set the
> > > > camera location in a configuration file for the libcamera core, but to
> > > > isolate it in the HAL as a temporary (but in practice likely a long-term
> > > > temporary) measure, as we have to support platforms that will take some
> > > > time before a firmware (ACPI in this case) update can be deployed.
> > > >
> > > > This being said, we will introduce a configuration file for the
> > > > libcamera core in the near future, and we also plan to use libyaml
> > > > there. The tuning files for the IPU3 and RkISP1 IPA modules will also
> > > > use YAML, and I'd like to move the Raspberry Pi tuning files from JSON
> > > > to YAML for consistency.
> > >
> > > It's a good idea to have a single file format. Currently we have to
> > > ship boost to run raspberrypi cameras on Android.
> >
> > I'm happy you agree :-)
> >
> > > > We could merge this patch in the meantime, but compilation on Android
> > > > will likely break again soon. I wonder if we couldn't instead use a
> > > > meson wrap to include libyaml as a subproject. This should solve the
> > > > issue in the longer term.
> > >
> > > At this moment we have libyaml with extra Android.bp, which can
> > > satisfy libcamera needs.
> > > But I really like to see it as an optional dependency only to help
> > > fine-tuning things or for ACPI-only platforms.
> >
> > Making it optional may cause difficulties in the future, with
> > potentially lots of conditional compilation. I'd rather not go that way,
> > unless we can wrap the yaml parser in a helper class with a dummy
> > implementation to hide the missing dependency from everything else.
> >
> > I've quickly tested compiling libyaml as a subproject, and it seems to
> > work:
>
> I have some doubts if such an approach supports cross-compilation.
> Could meson generate a single ninja.build from all sub-projects?
> Implementing the fallback stub sounds not so bad.
With an additional
libyaml_vars.append_compile_args('c', '-Wno-unused-value')
it cross-compiles fine for ARM64.
> > commit f1d86859f522c7728d8f854b82296673291750fb
> > Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Date: Wed Dec 29 12:07:35 2021 +0200
> >
> > android: Compile libyaml as a subproject if not available on the system
> >
> > AOSP doesn't ship libyaml, making it more difficult to compile libcamera
> > for Android. Use a meson wrap to compile libyaml as a subproject if the
> > dependency is not found.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 75b4bf207085..1fa67746d058 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -3,7 +3,6 @@
> > android_deps = [
> > dependency('libexif', required : get_option('android')),
> > dependency('libjpeg', required : get_option('android')),
> > - dependency('yaml-0.1', required : get_option('android')),
> > libcamera_private,
> > ]
> >
> > @@ -16,13 +15,35 @@ foreach dep : android_deps
> > endif
> > endforeach
> >
> > +cmake = import('cmake')
> > +
> > +#
> > +# libyaml
> > +#
> > +# Default to the system version, and fallback to a subproject if not found, as
> > +# libyaml is not packaged in AOSP.
> > +#
> > +libyaml_dep = dependency('yaml-0.1', required : false)
> > +
> > +# Fallback to a subproject if libyaml isn't found, as it's not packed in AOSP.
> > +if not libyaml_dep.found()
> > + libyaml_vars = cmake.subproject_options()
> > + libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'})
> > + libyaml = cmake.subproject('libyaml', options : libyaml_vars)
> > + libyaml_dep = libyaml.dependency('yaml')
> > +endif
> > +
> > +android_deps += [libyaml_dep]
> > +
> > +#
> > +# libyuv
> > +#
> > +# Default to the system version, and fallback to a subproject if not found, as
> > +# libyuv is typically not provided by distributions.
> > +#
> > libyuv_dep = dependency('libyuv', required : false)
> >
> > -# Fallback to a subproject if libyuv isn't found, as it's typically not
> > -# provided by distributions.
> > if not libyuv_dep.found()
> > - cmake = import('cmake')
> > -
> > libyuv_vars = cmake.subproject_options()
> > libyuv_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'})
> > libyuv_vars.set_override_option('cpp_std', 'c++17')
> > diff --git a/subprojects/.gitignore b/subprojects/.gitignore
> > index 391fde2ce1e0..f207720b271a 100644
> > --- a/subprojects/.gitignore
> > +++ b/subprojects/.gitignore
> > @@ -1,3 +1,4 @@
> > /googletest-release*
> > +/libyaml
> > /libyuv
> > -/packagecache
> > \ No newline at end of file
> > +/packagecache
> > diff --git a/subprojects/libyaml.wrap b/subprojects/libyaml.wrap
> > new file mode 100644
> > index 000000000000..e4426e44774f
> > --- /dev/null
> > +++ b/subprojects/libyaml.wrap
> > @@ -0,0 +1,4 @@
> > +[wrap-git]
> > +directory = libyaml
> > +url = https://github.com/yaml/libyaml
> > +revision = 2c891fc7a770e8ba2fec34fc6b545c672beb37e6
> >
> > > > By the way, out of curiosity, how do you compile libcamera for Android ?
> > > > We have a look at Soong (as the Makefile-based build system is being
> > > > phased out), but blueprint files don't support all our needs (and would
> > > > be annoying to maintain manually), and there's no way we found to
> > > > generate Android.bp files from meson in a way that would integrate with
> > > > Soong. This is still an unsolved problem, and I'm increasingly tempted
> > > > to do the same as mesa3d and just document that libcamera should be
> > > > built outside of AOSP, using the NDK.
> > >
> > > We shipped libcamera in our GloDroid project v0.7.0 release for
> > > raspberry pi about a month ago:
> > > 1. https://github.com/GloDroid/glodroid_manifest/releases/tag/v0.7.0
> > > 2. https://github.com/GloDroid/glodroid_forks/commits/libcamera-v0.7.0
> > >
> > > For building using meson within AOSP we are using some tricks to
> > > extract dependencies from AOSP makesystem and inject them into
> > > meson cross compilation file.
> >
> > Does this mean that you compile libcamera manually, separately from AOSP
> > ? That's probably the best option for now, until Android decides to
> > integrate support for other build systems (likely never...).
>
> No. Building within AOSP does mean building within AOSP.
> Not manually.
> It utilizes the Kati (Android.mk) build system.
> And Kati is currently tightly bound to Soong, separating both would require some
> time for Google if they decide to do so.
> And we always have "building using NDK" as a fallback once it happens.
I thought they had mostly finished the conversion to Soong, but I could
be wrong. I remember finding a page that listed the timeline and what
types of packages were still allowed to use Android.mk at different
points in the transition, but I can't locate that anymore.
Could you point me to the sources that show how you built libcamera ?
And if it wasn't "fun" enough, they're moving away from Soong to Bazel
(https://android.googlesource.com/platform/build/bazel/+/refs/heads/master/docs/concepts.md).
> > > First such kind of approach was integrated into mesa3d and replaced
> > > existing Android.mk files (about 6 month ago).
> > > It doesn't require continuous maintenance as previous Android.mk did
> > > and has proven to be reliable.
> > > It also enables FULL support of mesa3d for Android. Previously only
> > > about 60% of mesa3d drivers were covered with Android.mk files.
> > > Reworking the same approach for libcamera was pretty easy.
> > >
> > > And yes, as an official approach for vendors NDK builds would be great
> > > to support.
> > > NDK approach and our meson.build within AOSP approach are 98% aligned.
> > >
> > > > > > On Tue, Dec 28, 2021 at 11:51:53AM +0200, Roman Stratiienko wrote:
> > > > > > > Mainline Android has no libyaml in-tree. Moreover camera_hal_config.cpp
> > > > > > > uses std::filesystem class that is not permitted for AOSP VNDK vendor
> > > > > > > libraries.
> > > > > > >
> > > > > > > External configuration file is no longer mandatory and DTS overlays can
> > > > > > > be used to specify camera location and rotation.
> > > > > > >
> > > > > > > Do not require external configuration file for cases where meson.build
> > > > > > > can't find libyaml. Assume that cameras without location specified
> > > > > > > by V4L2 properties are always external.
> > > > > > >
> > > > > > > Signed-off-by: Roman Stratiienko <roman.o.stratiienko at globallogic.com>
> > > > > > > ---
> > > > > > > src/android/camera_hal_manager.cpp | 13 +++++++++++++
> > > > > > > src/android/camera_hal_manager.h | 2 ++
> > > > > > > src/android/meson.build | 10 ++++++++--
> > > > > > > 3 files changed, 23 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > > > > > > index 5f7bfe265c71b..9e575bfa5204a 100644
> > > > > > > --- a/src/android/camera_hal_manager.cpp
> > > > > > > +++ b/src/android/camera_hal_manager.cpp
> > > > > > > @@ -48,6 +48,7 @@ int CameraHalManager::init()
> > > > > > > {
> > > > > > > cameraManager_ = std::make_unique<CameraManager>();
> > > > > > >
> > > > > > > +#ifdef HAVE_LIBYAML
> > > > > > > /*
> > > > > > > * If the configuration file is not available the HAL only supports
> > > > > > > * external cameras. If it exists but it's not valid then error out.
> > > > > > > @@ -56,6 +57,7 @@ int CameraHalManager::init()
> > > > > > > LOG(HAL, Error) << "HAL configuration file is not valid";
> > > > > > > return -EINVAL;
> > > > > > > }
> > > > > > > +#endif
> > > > > > >
> > > > > > > /* Support camera hotplug. */
> > > > > > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
> > > > > > > @@ -133,6 +135,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > +#ifdef HAVE_LIBYAML
> > > > > > > /*
> > > > > > > * The configuration file must be valid, and contain a corresponding
> > > > > > > * entry for internal cameras. External cameras can be initialized
> > > > > > > @@ -145,6 +148,16 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > > > > > > }
> > > > > > >
> > > > > > > const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());
> > > > > > > +#else
> > > > > > > + /*
> > > > > > > + * Assume that cameras without properties::Location specified are external.
> > > > > > > + */
> > > > > > > + const CameraConfigData *cameraConfigData = nullptr;
> > > > > > > + if (!isCameraExternal && cameraLocation(cam.get()) < 0) {
> > > > > > > + isCameraExternal = true;
> > > > > > > + id = nextExternalCameraId_;
> > > > > > > + }
> > > > > > > +#endif
> > > > > > >
> > > > > > > /*
> > > > > > > * Some cameras whose location is reported by libcamera as external may
> > > > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > > > > > > index a5f8b933a7902..056abbb0d4e54 100644
> > > > > > > --- a/src/android/camera_hal_manager.h
> > > > > > > +++ b/src/android/camera_hal_manager.h
> > > > > > > @@ -56,7 +56,9 @@ private:
> > > > > > > CameraDevice *cameraDeviceFromHalId(unsigned int id) LIBCAMERA_TSA_REQUIRES(mutex_);
> > > > > > >
> > > > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_;
> > > > > > > +#ifdef HAVE_LIBYAML
> > > > > > > CameraHalConfig halConfig_;
> > > > > > > +#endif
> > > > > > >
> > > > > > > const camera_module_callbacks_t *callbacks_;
> > > > > > > std::vector<std::unique_ptr<CameraDevice>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > > > > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > > > > index 75b4bf2070851..1e471c291a1e4 100644
> > > > > > > --- a/src/android/meson.build
> > > > > > > +++ b/src/android/meson.build
> > > > > > > @@ -3,7 +3,6 @@
> > > > > > > android_deps = [
> > > > > > > dependency('libexif', required : get_option('android')),
> > > > > > > dependency('libjpeg', required : get_option('android')),
> > > > > > > - dependency('yaml-0.1', required : get_option('android')),
> > > > > > > libcamera_private,
> > > > > > > ]
> > > > > > >
> > > > > > > @@ -41,7 +40,6 @@ android_hal_sources = files([
> > > > > > > 'camera3_hal.cpp',
> > > > > > > 'camera_capabilities.cpp',
> > > > > > > 'camera_device.cpp',
> > > > > > > - 'camera_hal_config.cpp',
> > > > > > > 'camera_hal_manager.cpp',
> > > > > > > 'camera_metadata.cpp',
> > > > > > > 'camera_ops.cpp',
> > > > > > > @@ -56,6 +54,14 @@ android_hal_sources = files([
> > > > > > >
> > > > > > > android_cpp_args = []
> > > > > > >
> > > > > > > +libyaml_dep = dependency('yaml-0.1', required : false)
> > > > > > > +if libyaml_dep.found()
> > > > > > > + config_h.set('HAVE_LIBYAML', 1)
> > > > > > > + android_hal_sources += files([
> > > > > > > + 'camera_hal_config.cpp',
> > > > > > > + ])
> > > > > > > +endif
> > > > > > > +
> > > > > > > subdir('cros')
> > > > > > > subdir('mm')
> > > > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list