[PATCH v5 03/15] config: Introduce global runtime configuration

[Milan Zamazal]

Hi Barnabás,

thank you for review.

Barnabás Pőcze <pobrn at protonmail.com> writes:

> Hi
>
>
> 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>
>> Currently, libcamera can be configured in runtime using several
>> environment variables.  With introducing more and more variables, this
>> mechanism reaches its limits.  It would be simpler and more flexible if
>> it was possible to configure libcamera in a single file.
>> 
>> For example, there have been a request for defining pipeline precedence
>> in runtime.  We want to compile in multiple pipelines, in order to have
>> them accessible within single packages in distributions.  And then being
>> able to select among the pipelines manually as needed based on the
>> particular hardware or operating system environment.  Having the
>> configuration file then allows easy switching between hardware, GPU or
>> CPU IPAs.  Another possible use case is tuning image output, especially
>> with software ISP, to user liking.  For example, some users may prefer
>> higher contrast without the need to use the corresponding knobs, if
>> present at all, in every application.  The configuration file can also
>> be used to enable or disable experimental features and avoid the need to
>> track local patches changing configuration options hard-wired in the
>> code when working on new features.
>> 
>> This patch introduces basic support for configuration files.
>> GlobalConfiguration class reads, stores and accesses the configuration.
>> Its instance is stored as a private singleton accessed using a static
>> method of the class.  There is currently no reason to have more than one
>> instance.
>> 
>> libcamera configuration can be specified using a system-wide
>> configuration file or a user configuration file.  The user configuration
>> file takes precedence if present.  There is currently no way to merge
>> multiple configuration files, the one found is used as the only
>> configuration file.  If no configuration file is present, nothing
>> changes to the current libcamera behavior (except for some log
>> messages related to configuration file lookup).
>> 
>> The configuration file is a YAML file.  We already have a mechanism for
>> handling YAML configuration files in libcamera and the given
>> infrastructure can be reused for the purpose.  However, the
>> configuration type is abstracted to make contingent future change of the
>> underlying class easier while retaining (most of) the original API.
>> 
>> The configuration is versioned.  This has currently no particular
>> meaning but is likely to have its purpose in future, especially once
>> configuration validation is introduced.
>> 
>> The configuration YAML file looks as follows:
>> 
>>   ---
>>   version: 1
>>   configuration:
>>     WHATEVER CONFIGURATION NEEDED
>> 
>> There is no logging about reading the configuration file and contingent
>> errors.  This is on purpose because logging will query configuration,
>> which can lead to various problems when done during configuration
>> initialization.  Reporting the errors will be added later.
>> 
>> The global configuration is intended to be used as a hidden singleton,
>> with static class methods around it providing the configuration
>> information.  The constructor must be still public so that unique_ptr
>> can be used.
>> 
>> A complication arises from the fact that when the configuration is
>> loaded, logging may be called and logging will ask for the
>> configuration.  This is error-prone and may lead to subtle problems.
>> For this reason, the global configuration is instantiated to a pointer,
>> with an empty configuration initially.  The real configuration will be
>> created through initialize() method.  It will be clearer how it helps in
>> the followup patch introducing logging configuration.
>> 
>> Logging is also the most notable component from base that uses global
>> configuration.  In order to be able to do it, the global configuration
>> must be put to base.
>> 
>> This patch introduces just the basic idea.  Actually using the
>> configuration in the corresponding places (everything what is currently
>> configurable via environment variables should be configurable in the
>> file configuration) and other enhancements are implemented in the
>> followup patches.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  .../libcamera/internal/global_configuration.h |  41 +++++
>>  include/libcamera/internal/meson.build        |   1 +
>>  src/libcamera/base/global_configuration.cpp   | 154 ++++++++++++++++++
>>  src/libcamera/base/meson.build                |   1 +
>>  4 files changed, 197 insertions(+)
>>  create mode 100644 include/libcamera/internal/global_configuration.h
>>  create mode 100644 src/libcamera/base/global_configuration.cpp
>> 
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> new file mode 100644
>> index 000000000..628d85cb8
>> --- /dev/null
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -0,0 +1,41 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024 Red Hat, inc.
>> + *
>> + * global_configuration.h - Global configuration handling
>> + */
>> +
>> +#pragma once
>> +
>> +#include <filesystem>
>> +#include <vector>
>> +
>> +#include "libcamera/internal/yaml_parser.h"
>> +
>> +namespace libcamera {
>> +
>> +class GlobalConfiguration
>> +{
>> +public:
>> +	using Configuration = const YamlObject &;
>> +
>> +	/* The constructor must be public to be able to use unique_ptr. */
>
> If one does
>
>   std::unique_ptr<T>(new T)
>
> then the constructor does not need to be public.

Ah, cool.

>> +	GlobalConfiguration();
>> +	static void initialize();
>> +
>> +	static unsigned int version();
>> +	static Configuration configuration();
>> +
>> +private:
>> +	static const std::vector<std::filesystem::path> globalConfigurationFiles;
>> +
>> +	static std::unique_ptr<GlobalConfiguration> instance_;
>> +
>> +	std::unique_ptr<YamlObject> configuration_;
>> +
>> +	bool loadFile(const std::filesystem::path &fileName);
>> +	void load();
>> +	static Configuration get();
>> +};
>> +
>> +} /* namespace libcamera */
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 1c5eef9ca..6fc6406d7 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([
>>      'dma_buf_allocator.h',
>>      'formats.h',
>>      'framebuffer.h',
>> +    'global_configuration.h',
>>      'ipa_data_serializer.h',
>>      'ipa_manager.h',
>>      'ipa_module.h',
>> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
>> new file mode 100644
>> index 000000000..7dcf97265
>> --- /dev/null
>> +++ b/src/libcamera/base/global_configuration.cpp
>> @@ -0,0 +1,154 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024 Red Hat, inc.
>> + *
>> + * global_configuration.cpp - Global configuration handling
>> + */
>> +
>> +#include "libcamera/internal/global_configuration.h"
>> +
>> +#include <filesystem>
>> +#include <memory>
>> +#include <sys/types.h>
>> +
>> +#include <libcamera/base/file.h>
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include "libcamera/internal/yaml_parser.h"
>> +
>> +namespace libcamera {
>> +
>> +std::unique_ptr<GlobalConfiguration> GlobalConfiguration::instance_ =
>> +	std::make_unique<GlobalConfiguration>();
>
> Why is a unique_ptr needed here?

Most notably to stick with the libcamera policy of avoiding using plain
pointers.  And for the instance disposal in initialize() below.

>> +
>> +/**
>> + * \brief Do not create GlobalConfiguration instance directly, use initialize()
>> + */
>> +void GlobalConfiguration::initialize()
>> +{
>> +	std::unique_ptr<GlobalConfiguration> instance =
>> +		std::make_unique<GlobalConfiguration>();
>> +	instance->load();
>> +	instance_ = std::move(instance);
>> +}
>> +
>> +/**
>> + * \class GlobalConfiguration
>> + * \brief Support for global libcamera configuration
>> + *
>> + * The configuration file is a YAML file and the configuration itself is stored
>> + * under `configuration' top-level item.
>> + *
>> + * The configuration file is looked up in user's home directory first and if it
>> + * is not found then in system-wide configuration directories. If multiple
>> + * configuration files exist then only the first one found is used and no
>> + * configuration merging is performed.
>> + *
>> + * The class is used as a private singleton and the configuration can be
>> + * accessed using GlobalConfiguration::configuration().
>> + */
>> +
>> +/**
>> + * \typedef GlobalConfiguration::Configuration
>> + * \brief Type representing global libcamera configuration
>> + *
>> + * All code outside GlobalConfiguration must use this type declaration and not
>> + * the underlying type.
>> + */
>> +
>> +GlobalConfiguration::GlobalConfiguration()
>> +	: configuration_(std::make_unique<YamlObject>())
>> +{
>> +}
>> +
>> +const std::vector<std::filesystem::path>
>> +	GlobalConfiguration::globalConfigurationFiles = {
>> +		std::filesystem::path(LIBCAMERA_SYSCONF_DIR) / "configuration.yaml",
>> +		std::filesystem::path("/etc/libcamera/configuration.yaml"),
>> +	};
>
> I think the above array can go into an anonymous namespace as there does not
> appear to be used anywhere else.

OK.

>> +
>> +void GlobalConfiguration::load()
>> +{
>> +	std::filesystem::path userConfigurationDirectory;
>> +	char *xdgConfigHome = utils::secure_getenv("XDG_CONFIG_HOME");
>> +	if (xdgConfigHome) {
>> +		userConfigurationDirectory = xdgConfigHome;
>> +	} else {
>> +		const char *home = utils::secure_getenv("HOME");
>> +		if (home)
>> +			userConfigurationDirectory =
>> +				std::filesystem::path(home) / ".config";
>> +	}
>> +
>> +	if (!userConfigurationDirectory.empty()) {
>> +		std::filesystem::path user_configuration_file =
>> +			userConfigurationDirectory / "libcamera" / "configuration.yaml";
>> +		if (loadFile(user_configuration_file))
>> +			return;
>> +	}
>> +
>> +	for (auto path : globalConfigurationFiles)
>
> const auto &path

OK.

>> +		if (loadFile(path))
>> +			return;
>> +}
>> +
>> +bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
>> +{
>> +	File file(fileName);
>> +	if (!file.exists()) {
>> +		return false;
>> +	}
>> +
>> +	if (!file.open(File::OpenModeFlag::ReadOnly))
>> +		return true;
>> +
>> +	auto root = YamlParser::parse(file);
>> +	if (!root)
>> +		return true;
>> +	configuration_ = std::move(root);
>> +
>> +	return true;
>> +}
>> +
>> +GlobalConfiguration::Configuration GlobalConfiguration::get()
>> +{
>> +	return *instance_->configuration_;
>> +}
>> +
>> +/**
>> + * \brief Return configuration version
>> + *
>> + * The version is (optionally) declared in the configuration file in the
>> + * top-level section `version', alongside `configuration'. This has currently no
>> + * real use but may be needed in future if configuration incompatibilities
>> + * occur.
>> + *
>> + * \return Configuration version as declared in the configuration file or 0 if
>> + * no version is declared there
>> + */
>> +unsigned int GlobalConfiguration::version()
>> +{
>> +	return get()["version"].get<unsigned int>().value_or(0);
>> +}
>> +
>> +/**
>> + * \brief Return libcamera global configuration
>> + *
>> + * This returns the whole configuration stored in the top-level section
>> + * `configuration' of the YAML configuration file.
>> + *
>> + * The requested part of the configuration can be accessed using \a YamlObject
>> + * methods.
>> + *
>> + * \note \a YamlObject type itself shouldn't be used in type declarations to
>> + * avoid trouble if we decide to change the underlying data objects in future.
>> + *
>> + * \return The whole configuration section
>> + */
>> +GlobalConfiguration::Configuration GlobalConfiguration::configuration()
>> +{
>> +	return get()["configuration"];
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
>> index 94843eb95..4c0032845 100644
>> --- a/src/libcamera/base/meson.build
>> +++ b/src/libcamera/base/meson.build
>> @@ -16,6 +16,7 @@ libcamera_base_internal_sources = files([
>>      'event_dispatcher_poll.cpp',
>>      'event_notifier.cpp',
>>      'file.cpp',
>> +    'global_configuration.cpp',
>>      'log.cpp',
>>      'memfd.cpp',
>>      'message.cpp',
>> --
>> 2.44.1
>> 
>> 
>
> Inspired by pipewire, I think the following two would be quite useful:
>   1. `LIBCAMERA_CONFIG_DIR` environmental variable, which denotes a (list of) directory(s)
>      that is searched first.
>   2. `LIBCAMERA_CONFIG_NAME` environmental variable, which denotes
>       a) the path of the configuration file to load if it is an absolute path; or
>       b) the path of the configuration file to load, relative to the configuration
>          directories
>
>   (1) would make it easy to integrate with `meson devenv`
>   (2) would make it easy to switch between configurations, which is not too easy
>       to do if the filename is hard-coded

Yes, this would be useful and I think somebody has already suggested
making the configuration location flexible.  I'll add the variables, in
additional patches.

> Regards,
> Barnabás Pőcze