| Message ID | 20241001102810.479285-4-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@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@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. > + 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? > + > +/** > + * \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. > + > +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 > + 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 Regards, Barnabás Pőcze
Hi Barnabás, thank you for review. Barnabás Pőcze <pobrn@protonmail.com> writes: > Hi > > > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@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@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
Hi 2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > Hi Barnabás, > > thank you for review. > > Barnabás Pőcze <pobrn@protonmail.com> writes: > > > Hi > > > > > > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@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@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. > [...] I was thinking that there is no need to dynamically allocate a `GlobalConfiguration` instance. In fact, now I think `GlobalConfiguration` could simply be a namespace. All private members of the current class would go into an anonymous namespace, and all public members would just be functions in the namespace. Regards, Barnabás Pőcze
Barnabás Pőcze <pobrn@protonmail.com> writes: > Hi > > > 2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > >> Hi Barnabás, >> >> thank you for review. >> >> Barnabás Pőcze <pobrn@protonmail.com> writes: >> >> > Hi >> > >> > >> > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@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@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. >> [...] > > I was thinking that there is no need to dynamically allocate a `GlobalConfiguration` > instance. In fact, now I think `GlobalConfiguration` could simply be a namespace. > All private members of the current class would go into an anonymous namespace, > and all public members would just be functions in the namespace. I see, we can get rid of `instance_' this way and just store the configuration. OK, this would be probably better. > > Regards, > Barnabás Pőcze
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. */ + 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>(); + +/** + * \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"), + }; + +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) + 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',
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@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