[libcamera-devel] [PATCH] android: Make libyaml dependency optional
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 29 15:40:14 CET 2021
Hi Roman,
On Wed, Dec 29, 2021 at 02:17:03PM +0200, Roman Stratiienko wrote:
> +CC: John Stultz
>
> Laurent,
>
> I would like to make mainline libcamera work on mainline android
> without additional patches / extra packages and then give it to John
> Stultz for testing with AOSP devboards in this shape.
> Unlike mesa3d libcamera doesn't have a lot of generated code,
> therefore shipping this code / creating Android.bp is less painful.
How do you envision this being done ? We don't want to commit generated
sources to the libcamera git repository. Please note that one of the
source files generated during build contains a randomly generated
private key which is thrown away at the end of the build. Committing a
private key to a source tree will not be a very good idea :-)
> In case this tricky plan will succeed and we will have libcamera
> inside AOSP, then rationale for adding libyaml into AOSP would be much
> easier to formulate.
>
> ср, 29 дек. 2021 г. в 12:24, Roman Stratiienko:
> > ср, 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.
> >
> > >
> > > 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.
> >
> > >
> > > > 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