[{"id":32525,"web_url":"https://patchwork.libcamera.org/comment/32525/","msgid":"","date":"2024-12-04T17:45:02","subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n\n> Currently, libcamera can be configured in runtime using several\n> environment variables. With introducing more and more variables, this\n> mechanism reaches its limits. It would be simpler and more flexible if\n> it was possible to configure libcamera in a single file.\n> \n> For example, there have been a request for defining pipeline precedence\n> in runtime. We want to compile in multiple pipelines, in order to have\n> them accessible within single packages in distributions. And then being\n> able to select among the pipelines manually as needed based on the\n> particular hardware or operating system environment. Having the\n> configuration file then allows easy switching between hardware, GPU or\n> CPU IPAs. Another possible use case is tuning image output, especially\n> with software ISP, to user liking. For example, some users may prefer\n> higher contrast without the need to use the corresponding knobs, if\n> present at all, in every application. The configuration file can also\n> be used to enable or disable experimental features and avoid the need to\n> track local patches changing configuration options hard-wired in the\n> code when working on new features.\n> \n> This patch introduces basic support for configuration files.\n> GlobalConfiguration class reads, stores and accesses the configuration.\n> Its instance is stored as a private singleton accessed using a static\n> method of the class. There is currently no reason to have more than one\n> instance.\n> \n> libcamera configuration can be specified using a system-wide\n> configuration file or a user configuration file. The user configuration\n> file takes precedence if present. There is currently no way to merge\n> multiple configuration files, the one found is used as the only\n> configuration file. If no configuration file is present, nothing\n> changes to the current libcamera behavior (except for some log\n> messages related to configuration file lookup).\n> \n> The configuration file is a YAML file. We already have a mechanism for\n> handling YAML configuration files in libcamera and the given\n> infrastructure can be reused for the purpose. However, the\n> configuration type is abstracted to make contingent future change of the\n> underlying class easier while retaining (most of) the original API.\n> \n> The configuration is versioned. This has currently no particular\n> meaning but is likely to have its purpose in future, especially once\n> configuration validation is introduced.\n> \n> The configuration YAML file looks as follows:\n> \n> ---\n> version: 1\n> configuration:\n> WHATEVER CONFIGURATION NEEDED\n> \n> There is no logging about reading the configuration file and contingent\n> errors. This is on purpose because logging will query configuration,\n> which can lead to various problems when done during configuration\n> initialization. Reporting the errors will be added later.\n> \n> The global configuration is intended to be used as a hidden singleton,\n> with static class methods around it providing the configuration\n> information. The constructor must be still public so that unique_ptr\n> can be used.\n> \n> A complication arises from the fact that when the configuration is\n> loaded, logging may be called and logging will ask for the\n> configuration. This is error-prone and may lead to subtle problems.\n> For this reason, the global configuration is instantiated to a pointer,\n> with an empty configuration initially. The real configuration will be\n> created through initialize() method. It will be clearer how it helps in\n> the followup patch introducing logging configuration.\n> \n> Logging is also the most notable component from base that uses global\n> configuration. In order to be able to do it, the global configuration\n> must be put to base.\n> \n> This patch introduces just the basic idea. Actually using the\n> configuration in the corresponding places (everything what is currently\n> configurable via environment variables should be configurable in the\n> file configuration) and other enhancements are implemented in the\n> followup patches.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> .../libcamera/internal/global_configuration.h | 41 +++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/base/global_configuration.cpp | 154 ++++++++++++++++++\n> src/libcamera/base/meson.build | 1 +\n> 4 files changed, 197 insertions(+)\n> create mode 100644 include/libcamera/internal/global_configuration.h\n> create mode 100644 src/libcamera/base/global_configuration.cpp\n> \n> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n> new file mode 100644\n> index 000000000..628d85cb8\n> --- /dev/null\n> +++ b/include/libcamera/internal/global_configuration.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024 Red Hat, inc.\n> + *\n> + * global_configuration.h - Global configuration handling\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +class GlobalConfiguration\n> +{\n> +public:\n> +\tusing Configuration = const YamlObject &;\n> +\n> +\t/* The constructor must be public to be able to use unique_ptr. */\n\nIf one does\n\n std::unique_ptr(new T)\n\nthen the constructor does not need to be public.\n\n\n> +\tGlobalConfiguration();\n> +\tstatic void initialize();\n> +\n> +\tstatic unsigned int version();\n> +\tstatic Configuration configuration();\n> +\n> +private:\n> +\tstatic const std::vector globalConfigurationFiles;\n> +\n> +\tstatic std::unique_ptr instance_;\n> +\n> +\tstd::unique_ptr configuration_;\n> +\n> +\tbool loadFile(const std::filesystem::path &fileName);\n> +\tvoid load();\n> +\tstatic Configuration get();\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1c5eef9ca..6fc6406d7 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n> 'dma_buf_allocator.h',\n> 'formats.h',\n> 'framebuffer.h',\n> + 'global_configuration.h',\n> 'ipa_data_serializer.h',\n> 'ipa_manager.h',\n> 'ipa_module.h',\n> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp\n> new file mode 100644\n> index 000000000..7dcf97265\n> --- /dev/null\n> +++ b/src/libcamera/base/global_configuration.cpp\n> @@ -0,0 +1,154 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024 Red Hat, inc.\n> + *\n> + * global_configuration.cpp - Global configuration handling\n> + */\n> +\n> +#include \"libcamera/internal/global_configuration.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +std::unique_ptr GlobalConfiguration::instance_ =\n> +\tstd::make_unique();\n\nWhy is a unique_ptr needed here?\n\n\n> +\n> +/**\n> + * \\brief Do not create GlobalConfiguration instance directly, use initialize()\n> + */\n> +void GlobalConfiguration::initialize()\n> +{\n> +\tstd::unique_ptr instance =\n> +\t\tstd::make_unique();\n> +\tinstance->load();\n> +\tinstance_ = std::move(instance);\n> +}\n> +\n> +/**\n> + * \\class GlobalConfiguration\n> + * \\brief Support for global libcamera configuration\n> + *\n> + * The configuration file is a YAML file and the configuration itself is stored\n> + * under `configuration' top-level item.\n> + *\n> + * The configuration file is looked up in user's home directory first and if it\n> + * is not found then in system-wide configuration directories. If multiple\n> + * configuration files exist then only the first one found is used and no\n> + * configuration merging is performed.\n> + *\n> + * The class is used as a private singleton and the configuration can be\n> + * accessed using GlobalConfiguration::configuration().\n> + */\n> +\n> +/**\n> + * \\typedef GlobalConfiguration::Configuration\n> + * \\brief Type representing global libcamera configuration\n> + *\n> + * All code outside GlobalConfiguration must use this type declaration and not\n> + * the underlying type.\n> + */\n> +\n> +GlobalConfiguration::GlobalConfiguration()\n> +\t: configuration_(std::make_unique())\n> +{\n> +}\n> +\n> +const std::vector\n> +\tGlobalConfiguration::globalConfigurationFiles = {\n> +\t\tstd::filesystem::path(LIBCAMERA_SYSCONF_DIR) / \"configuration.yaml\",\n> +\t\tstd::filesystem::path(\"/etc/libcamera/configuration.yaml\"),\n> +\t};\n\nI think the above array can go into an anonymous namespace as there does not\nappear to be used anywhere else.\n\n\n> +\n> +void GlobalConfiguration::load()\n> +{\n> +\tstd::filesystem::path userConfigurationDirectory;\n> +\tchar *xdgConfigHome = utils::secure_getenv(\"XDG_CONFIG_HOME\");\n> +\tif (xdgConfigHome) {\n> +\t\tuserConfigurationDirectory = xdgConfigHome;\n> +\t} else {\n> +\t\tconst char *home = utils::secure_getenv(\"HOME\");\n> +\t\tif (home)\n> +\t\t\tuserConfigurationDirectory =\n> +\t\t\t\tstd::filesystem::path(home) / \".config\";\n> +\t}\n> +\n> +\tif (!userConfigurationDirectory.empty()) {\n> +\t\tstd::filesystem::path user_configuration_file =\n> +\t\t\tuserConfigurationDirectory / \"libcamera\" / \"configuration.yaml\";\n> +\t\tif (loadFile(user_configuration_file))\n> +\t\t\treturn;\n> +\t}\n> +\n> +\tfor (auto path : globalConfigurationFiles)\n\nconst auto &path\n\n\n> +\t\tif (loadFile(path))\n> +\t\t\treturn;\n> +}\n> +\n> +bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n> +{\n> +\tFile file(fileName);\n> +\tif (!file.exists()) {\n> +\t\treturn false;\n> +\t}\n> +\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly))\n> +\t\treturn true;\n> +\n> +\tauto root = YamlParser::parse(file);\n> +\tif (!root)\n> +\t\treturn true;\n> +\tconfiguration_ = std::move(root);\n> +\n> +\treturn true;\n> +}\n> +\n> +GlobalConfiguration::Configuration GlobalConfiguration::get()\n> +{\n> +\treturn *instance_->configuration_;\n> +}\n> +\n> +/**\n> + * \\brief Return configuration version\n> + *\n> + * The version is (optionally) declared in the configuration file in the\n> + * top-level section `version', alongside `configuration'. This has currently no\n> + * real use but may be needed in future if configuration incompatibilities\n> + * occur.\n> + *\n> + * \\return Configuration version as declared in the configuration file or 0 if\n> + * no version is declared there\n> + */\n> +unsigned int GlobalConfiguration::version()\n> +{\n> +\treturn get()[\"version\"].get().value_or(0);\n> +}\n> +\n> +/**\n> + * \\brief Return libcamera global configuration\n> + *\n> + * This returns the whole configuration stored in the top-level section\n> + * `configuration' of the YAML configuration file.\n> + *\n> + * The requested part of the configuration can be accessed using \\a YamlObject\n> + * methods.\n> + *\n> + * \\note \\a YamlObject type itself shouldn't be used in type declarations to\n> + * avoid trouble if we decide to change the underlying data objects in future.\n> + *\n> + * \\return The whole configuration section\n> + */\n> +GlobalConfiguration::Configuration GlobalConfiguration::configuration()\n> +{\n> +\treturn get()[\"configuration\"];\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> index 94843eb95..4c0032845 100644\n> --- a/src/libcamera/base/meson.build\n> +++ b/src/libcamera/base/meson.build\n> @@ -16,6 +16,7 @@ libcamera_base_internal_sources = files([\n> 'event_dispatcher_poll.cpp',\n> 'event_notifier.cpp',\n> 'file.cpp',\n> + 'global_configuration.cpp',\n> 'log.cpp',\n> 'memfd.cpp',\n> 'message.cpp',\n> --\n> 2.44.1\n> \n> \n\nInspired by pipewire, I think the following two would be quite useful:\n 1. `LIBCAMERA_CONFIG_DIR` environmental variable, which denotes a (list of) directory(s)\n that is searched first.\n 2. `LIBCAMERA_CONFIG_NAME` environmental variable, which denotes\n a) the path of the configuration file to load if it is an absolute path; or\n b) the path of the configuration file to load, relative to the configuration\n directories\n\n (1) would make it easy to integrate with `meson devenv`\n (2) would make it easy to switch between configurations, which is not too easy\n to do if the filename is hard-coded\n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E810CBDB1C\n\tfor ;\n\tWed, 4 Dec 2024 17:45:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 167D9660E1;\n\tWed, 4 Dec 2024 18:45:13 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 319F1618B5\n\tfor ;\n\tWed, 4 Dec 2024 18:45:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"ibEuc43U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1733334309; x=1733593509;\n\tbh=0E4Rq4o+bkc7KuxsZk0X8Hd6qCFJ3ANY/t/un/HPNxE=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=ibEuc43URzwAod4uksPEM6plSpOUiUFEzlypTvQ5QzLzlraLnyshPTorI2tTOJ6FV\n\tlPdBBW4VmkzMia70xStS09M2u+6pZOHjWBnImg0wjf+6AcZAUmPd4Nv2+VgQkaFQ/Q\n\t0YuHTp19cqBE4qc+7K5q/Fyk0A+2T/jBZIbeLaH3oxs51O/g/vuoi5lfSL9vpSMS3k\n\tBc4iOHDReuTUndKg7e4wywkC6YUMkCdhKKtf7Cz7/lVasD6fUBp/WBba4mN4k1YQOk\n\teWAN/oWpQiWaSzKnx6VhlneQ7AFyS+ckfHluYavE3uZ3SJ/AcBfqqlW2D1EJRkoLFq\n\twToq/TNVGnvWQ==","Date":"Wed, 04 Dec 2024 17:45:02 +0000","To":"Milan Zamazal ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tNaushir Patuck ","Subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","Message-ID":"","In-Reply-To":"<20241001102810.479285-4-mzamazal@redhat.com>","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-4-mzamazal@redhat.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"a528539f35a02552cbb3f90c041f5da07fdeaad6","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32536,"web_url":"https://patchwork.libcamera.org/comment/32536/","msgid":"<874j3i1i05.fsf@redhat.com>","date":"2024-12-05T11:33:46","subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nthank you for review.\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n>\n> 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n>\n>> Currently, libcamera can be configured in runtime using several\n>> environment variables. With introducing more and more variables, this\n>> mechanism reaches its limits. It would be simpler and more flexible if\n>> it was possible to configure libcamera in a single file.\n>> \n>> For example, there have been a request for defining pipeline precedence\n>> in runtime. We want to compile in multiple pipelines, in order to have\n>> them accessible within single packages in distributions. And then being\n>> able to select among the pipelines manually as needed based on the\n>> particular hardware or operating system environment. Having the\n>> configuration file then allows easy switching between hardware, GPU or\n>> CPU IPAs. Another possible use case is tuning image output, especially\n>> with software ISP, to user liking. For example, some users may prefer\n>> higher contrast without the need to use the corresponding knobs, if\n>> present at all, in every application. The configuration file can also\n>> be used to enable or disable experimental features and avoid the need to\n>> track local patches changing configuration options hard-wired in the\n>> code when working on new features.\n>> \n>> This patch introduces basic support for configuration files.\n>> GlobalConfiguration class reads, stores and accesses the configuration.\n>> Its instance is stored as a private singleton accessed using a static\n>> method of the class. There is currently no reason to have more than one\n>> instance.\n>> \n>> libcamera configuration can be specified using a system-wide\n>> configuration file or a user configuration file. The user configuration\n>> file takes precedence if present. There is currently no way to merge\n>> multiple configuration files, the one found is used as the only\n>> configuration file. If no configuration file is present, nothing\n>> changes to the current libcamera behavior (except for some log\n>> messages related to configuration file lookup).\n>> \n>> The configuration file is a YAML file. We already have a mechanism for\n>> handling YAML configuration files in libcamera and the given\n>> infrastructure can be reused for the purpose. However, the\n>> configuration type is abstracted to make contingent future change of the\n>> underlying class easier while retaining (most of) the original API.\n>> \n>> The configuration is versioned. This has currently no particular\n>> meaning but is likely to have its purpose in future, especially once\n>> configuration validation is introduced.\n>> \n>> The configuration YAML file looks as follows:\n>> \n>> ---\n>> version: 1\n>> configuration:\n>> WHATEVER CONFIGURATION NEEDED\n>> \n>> There is no logging about reading the configuration file and contingent\n>> errors. This is on purpose because logging will query configuration,\n>> which can lead to various problems when done during configuration\n>> initialization. Reporting the errors will be added later.\n>> \n>> The global configuration is intended to be used as a hidden singleton,\n>> with static class methods around it providing the configuration\n>> information. The constructor must be still public so that unique_ptr\n>> can be used.\n>> \n>> A complication arises from the fact that when the configuration is\n>> loaded, logging may be called and logging will ask for the\n>> configuration. This is error-prone and may lead to subtle problems.\n>> For this reason, the global configuration is instantiated to a pointer,\n>> with an empty configuration initially. The real configuration will be\n>> created through initialize() method. It will be clearer how it helps in\n>> the followup patch introducing logging configuration.\n>> \n>> Logging is also the most notable component from base that uses global\n>> configuration. In order to be able to do it, the global configuration\n>> must be put to base.\n>> \n>> This patch introduces just the basic idea. Actually using the\n>> configuration in the corresponding places (everything what is currently\n>> configurable via environment variables should be configurable in the\n>> file configuration) and other enhancements are implemented in the\n>> followup patches.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../libcamera/internal/global_configuration.h | 41 +++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/libcamera/base/global_configuration.cpp | 154 ++++++++++++++++++\n>> src/libcamera/base/meson.build | 1 +\n>> 4 files changed, 197 insertions(+)\n>> create mode 100644 include/libcamera/internal/global_configuration.h\n>> create mode 100644 src/libcamera/base/global_configuration.cpp\n>> \n>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n>> new file mode 100644\n>> index 000000000..628d85cb8\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/global_configuration.h\n>> @@ -0,0 +1,41 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024 Red Hat, inc.\n>> + *\n>> + * global_configuration.h - Global configuration handling\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/yaml_parser.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +class GlobalConfiguration\n>> +{\n>> +public:\n>> +\tusing Configuration = const YamlObject &;\n>> +\n>> +\t/* The constructor must be public to be able to use unique_ptr. */\n>\n> If one does\n>\n> std::unique_ptr(new T)\n>\n> then the constructor does not need to be public.\n\nAh, cool.\n\n>> +\tGlobalConfiguration();\n>> +\tstatic void initialize();\n>> +\n>> +\tstatic unsigned int version();\n>> +\tstatic Configuration configuration();\n>> +\n>> +private:\n>> +\tstatic const std::vector globalConfigurationFiles;\n>> +\n>> +\tstatic std::unique_ptr instance_;\n>> +\n>> +\tstd::unique_ptr configuration_;\n>> +\n>> +\tbool loadFile(const std::filesystem::path &fileName);\n>> +\tvoid load();\n>> +\tstatic Configuration get();\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index 1c5eef9ca..6fc6406d7 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n>> 'dma_buf_allocator.h',\n>> 'formats.h',\n>> 'framebuffer.h',\n>> + 'global_configuration.h',\n>> 'ipa_data_serializer.h',\n>> 'ipa_manager.h',\n>> 'ipa_module.h',\n>> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp\n>> new file mode 100644\n>> index 000000000..7dcf97265\n>> --- /dev/null\n>> +++ b/src/libcamera/base/global_configuration.cpp\n>> @@ -0,0 +1,154 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024 Red Hat, inc.\n>> + *\n>> + * global_configuration.cpp - Global configuration handling\n>> + */\n>> +\n>> +#include \"libcamera/internal/global_configuration.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/yaml_parser.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +std::unique_ptr GlobalConfiguration::instance_ =\n>> +\tstd::make_unique();\n>\n> Why is a unique_ptr needed here?\n\nMost notably to stick with the libcamera policy of avoiding using plain\npointers. And for the instance disposal in initialize() below.\n\n>> +\n>> +/**\n>> + * \\brief Do not create GlobalConfiguration instance directly, use initialize()\n>> + */\n>> +void GlobalConfiguration::initialize()\n>> +{\n>> +\tstd::unique_ptr instance =\n>> +\t\tstd::make_unique();\n>> +\tinstance->load();\n>> +\tinstance_ = std::move(instance);\n>> +}\n>> +\n>> +/**\n>> + * \\class GlobalConfiguration\n>> + * \\brief Support for global libcamera configuration\n>> + *\n>> + * The configuration file is a YAML file and the configuration itself is stored\n>> + * under `configuration' top-level item.\n>> + *\n>> + * The configuration file is looked up in user's home directory first and if it\n>> + * is not found then in system-wide configuration directories. If multiple\n>> + * configuration files exist then only the first one found is used and no\n>> + * configuration merging is performed.\n>> + *\n>> + * The class is used as a private singleton and the configuration can be\n>> + * accessed using GlobalConfiguration::configuration().\n>> + */\n>> +\n>> +/**\n>> + * \\typedef GlobalConfiguration::Configuration\n>> + * \\brief Type representing global libcamera configuration\n>> + *\n>> + * All code outside GlobalConfiguration must use this type declaration and not\n>> + * the underlying type.\n>> + */\n>> +\n>> +GlobalConfiguration::GlobalConfiguration()\n>> +\t: configuration_(std::make_unique())\n>> +{\n>> +}\n>> +\n>> +const std::vector\n>> +\tGlobalConfiguration::globalConfigurationFiles = {\n>> +\t\tstd::filesystem::path(LIBCAMERA_SYSCONF_DIR) / \"configuration.yaml\",\n>> +\t\tstd::filesystem::path(\"/etc/libcamera/configuration.yaml\"),\n>> +\t};\n>\n> I think the above array can go into an anonymous namespace as there does not\n> appear to be used anywhere else.\n\nOK.\n\n>> +\n>> +void GlobalConfiguration::load()\n>> +{\n>> +\tstd::filesystem::path userConfigurationDirectory;\n>> +\tchar *xdgConfigHome = utils::secure_getenv(\"XDG_CONFIG_HOME\");\n>> +\tif (xdgConfigHome) {\n>> +\t\tuserConfigurationDirectory = xdgConfigHome;\n>> +\t} else {\n>> +\t\tconst char *home = utils::secure_getenv(\"HOME\");\n>> +\t\tif (home)\n>> +\t\t\tuserConfigurationDirectory =\n>> +\t\t\t\tstd::filesystem::path(home) / \".config\";\n>> +\t}\n>> +\n>> +\tif (!userConfigurationDirectory.empty()) {\n>> +\t\tstd::filesystem::path user_configuration_file =\n>> +\t\t\tuserConfigurationDirectory / \"libcamera\" / \"configuration.yaml\";\n>> +\t\tif (loadFile(user_configuration_file))\n>> +\t\t\treturn;\n>> +\t}\n>> +\n>> +\tfor (auto path : globalConfigurationFiles)\n>\n> const auto &path\n\nOK.\n\n>> +\t\tif (loadFile(path))\n>> +\t\t\treturn;\n>> +}\n>> +\n>> +bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n>> +{\n>> +\tFile file(fileName);\n>> +\tif (!file.exists()) {\n>> +\t\treturn false;\n>> +\t}\n>> +\n>> +\tif (!file.open(File::OpenModeFlag::ReadOnly))\n>> +\t\treturn true;\n>> +\n>> +\tauto root = YamlParser::parse(file);\n>> +\tif (!root)\n>> +\t\treturn true;\n>> +\tconfiguration_ = std::move(root);\n>> +\n>> +\treturn true;\n>> +}\n>> +\n>> +GlobalConfiguration::Configuration GlobalConfiguration::get()\n>> +{\n>> +\treturn *instance_->configuration_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Return configuration version\n>> + *\n>> + * The version is (optionally) declared in the configuration file in the\n>> + * top-level section `version', alongside `configuration'. This has currently no\n>> + * real use but may be needed in future if configuration incompatibilities\n>> + * occur.\n>> + *\n>> + * \\return Configuration version as declared in the configuration file or 0 if\n>> + * no version is declared there\n>> + */\n>> +unsigned int GlobalConfiguration::version()\n>> +{\n>> +\treturn get()[\"version\"].get().value_or(0);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Return libcamera global configuration\n>> + *\n>> + * This returns the whole configuration stored in the top-level section\n>> + * `configuration' of the YAML configuration file.\n>> + *\n>> + * The requested part of the configuration can be accessed using \\a YamlObject\n>> + * methods.\n>> + *\n>> + * \\note \\a YamlObject type itself shouldn't be used in type declarations to\n>> + * avoid trouble if we decide to change the underlying data objects in future.\n>> + *\n>> + * \\return The whole configuration section\n>> + */\n>> +GlobalConfiguration::Configuration GlobalConfiguration::configuration()\n>> +{\n>> +\treturn get()[\"configuration\"];\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n>> index 94843eb95..4c0032845 100644\n>> --- a/src/libcamera/base/meson.build\n>> +++ b/src/libcamera/base/meson.build\n>> @@ -16,6 +16,7 @@ libcamera_base_internal_sources = files([\n>> 'event_dispatcher_poll.cpp',\n>> 'event_notifier.cpp',\n>> 'file.cpp',\n>> + 'global_configuration.cpp',\n>> 'log.cpp',\n>> 'memfd.cpp',\n>> 'message.cpp',\n>> --\n>> 2.44.1\n>> \n>> \n>\n> Inspired by pipewire, I think the following two would be quite useful:\n> 1. `LIBCAMERA_CONFIG_DIR` environmental variable, which denotes a (list of) directory(s)\n> that is searched first.\n> 2. `LIBCAMERA_CONFIG_NAME` environmental variable, which denotes\n> a) the path of the configuration file to load if it is an absolute path; or\n> b) the path of the configuration file to load, relative to the configuration\n> directories\n>\n> (1) would make it easy to integrate with `meson devenv`\n> (2) would make it easy to switch between configurations, which is not too easy\n> to do if the filename is hard-coded\n\nYes, this would be useful and I think somebody has already suggested\nmaking the configuration location flexible. I'll add the variables, in\nadditional patches.\n\n> Regards,\n> Barnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D95BBDB1C\n\tfor ;\n\tThu, 5 Dec 2024 11:33:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D6D4660F3;\n\tThu, 5 Dec 2024 12:33:54 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B887B6608C\n\tfor ;\n\tThu, 5 Dec 2024 12:33:52 +0100 (CET)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-392-I_qtQQq2MditsYQlx-Sf9Q-1; Thu, 05 Dec 2024 06:33:50 -0500","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-385d7d51ac8so413231f8f.1\n\tfor ;\n\tThu, 05 Dec 2024 03:33:50 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3861fd468f4sm1712276f8f.60.2024.12.05.03.33.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 05 Dec 2024 03:33:47 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"AgzCQ4Fe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1733398431;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=nDet851zQ+qvvscxtUeXmQpBRhWKL9dpjbJmwjLE3Zo=;\n\tb=AgzCQ4Feg+Ow4zB41hfvzC+S+zG2ZhfalMd5V0TEK2Oex+11RfHt3BxXdrzByQCizQgnvb\n\tfPHU12KCg7EFNBhsRbiLSu2BKOwGuww8koq8PQreqpzxk6e18DgzJ+QHOjsWPRX9N3z7aH\n\tVYy2Yasl/PUFbW2S8BrgC8KDcLEyYY4=","X-MC-Unique":"I_qtQQq2MditsYQlx-Sf9Q-1","X-Mimecast-MFC-AGG-ID":"I_qtQQq2MditsYQlx-Sf9Q","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733398429; x=1734003229;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=G5eSxxUGNivojP8EjUe3qvEH8s7qaDN78VRYeMOHG+4=;\n\tb=nX1Hf5KseUCJhkHkMl2b9f6Gi5zhExcfxkC91lwR5ImK2gLVVit5ptxSRw6B7ew12E\n\tboX9xoncA5P5ws5yEdFWahZJ82fGGeRLHpqmjVjtVh1ghO8Jzgy50asup+ZWMUatZblO\n\tr1FMX6Uo2kuec2Pqjv4qTT8HT+ColB+zuJv9l60mzufHQGH1AmTVEulpNqGM2/HwzY7E\n\t3zy9MEg9oJRVplEVpVgCdDXUjhUZK2jK29q5Z+F+fy1Sx1Cu3u28KrNRaeQgG7A0fj5B\n\tuiyazG8gAwakOpLIQcE26U41F5KcUQq4em0XNnHR5qIniqEFwGF1yyq4N16xyk0WGHLc\n\tchWw==","X-Gm-Message-State":"AOJu0YwiU6QU2Qg0ohiCUNfLmoaGqJHOdEBi8hZ9bQ/EZCMpsA74u//n\n\trL5W21+nz2kquT99iGfdpt2xA5lY41TT3pjxnLeCXjRltpH5JrHmG40DtGAzTFBC/zMNjfTBCTI\n\tdjtsDL6AVGEXtx5miRArC/05gpURcIaozenpLcUynT51lUh/PKcGa8wU/K2J8spkHzx1OQh8=","X-Gm-Gg":"ASbGncsW+oOMeWRZcTmcj+k6XJy4gyk/QFyWrGhg4iL8PuEAjgaC17ivEvax2XcoUl1\n\tyJTq7BYZvMOzoWdbVB7PEQyYBtxfeAJ8ExwDYR7HcXRIi8PQbgWoc+EEIXPicI4EJxfd/EktIYN\n\tR2ym0pxMm7rtrrV+aQG6tyv482MAMvZBxq+ZWYrq4NNegKOK4G08tPHZsQjusBoz0FSCsHlF6Xn\n\tFhelFatO0jHTacqEkQTXLDXud+Fsljrw6AbJ96iyU8RQE/davGRW4P7BbMX0KAmWMMPO04=","X-Received":["by 2002:a05:6000:4009:b0:385:fae4:424e with SMTP id\n\tffacd0b85a97d-385fd5327bdmr8222300f8f.52.1733398428957; \n\tThu, 05 Dec 2024 03:33:48 -0800 (PST)","by 2002:a05:6000:4009:b0:385:fae4:424e with SMTP id\n\tffacd0b85a97d-385fd5327bdmr8222232f8f.52.1733398427651; \n\tThu, 05 Dec 2024 03:33:47 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGvcM+B63hHJvWuehGKBpjLg7n/ozZyeWUXwYzDDlOVWPn90rnJjmreY5pDeIj0Vbp3OvP4uw==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Naushir Patuck ","Subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","In-Reply-To":"\n\t( =?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Wed,\n\t04 Dec 2024 17:45:02 +0000\")","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-4-mzamazal@redhat.com>\n\t","Date":"Thu, 05 Dec 2024 12:33:46 +0100","Message-ID":"<874j3i1i05.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"27wFdcjLWRNeJD9iuDFRUv8izhkxWO_3kNlu3GBKO7s_1733398429","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32543,"web_url":"https://patchwork.libcamera.org/comment/32543/","msgid":"","date":"2024-12-05T14:06:16","subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal írta:\n\n> Hi Barnabás,\n> \n> thank you for review.\n> \n> Barnabás Pőcze writes:\n> \n> > Hi\n> >\n> >\n> > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n> >\n> >> Currently, libcamera can be configured in runtime using several\n> >> environment variables. With introducing more and more variables, this\n> >> mechanism reaches its limits. It would be simpler and more flexible if\n> >> it was possible to configure libcamera in a single file.\n> >>\n> >> For example, there have been a request for defining pipeline precedence\n> >> in runtime. We want to compile in multiple pipelines, in order to have\n> >> them accessible within single packages in distributions. And then being\n> >> able to select among the pipelines manually as needed based on the\n> >> particular hardware or operating system environment. Having the\n> >> configuration file then allows easy switching between hardware, GPU or\n> >> CPU IPAs. Another possible use case is tuning image output, especially\n> >> with software ISP, to user liking. For example, some users may prefer\n> >> higher contrast without the need to use the corresponding knobs, if\n> >> present at all, in every application. The configuration file can also\n> >> be used to enable or disable experimental features and avoid the need to\n> >> track local patches changing configuration options hard-wired in the\n> >> code when working on new features.\n> >>\n> >> This patch introduces basic support for configuration files.\n> >> GlobalConfiguration class reads, stores and accesses the configuration.\n> >> Its instance is stored as a private singleton accessed using a static\n> >> method of the class. There is currently no reason to have more than one\n> >> instance.\n> >>\n> >> libcamera configuration can be specified using a system-wide\n> >> configuration file or a user configuration file. The user configuration\n> >> file takes precedence if present. There is currently no way to merge\n> >> multiple configuration files, the one found is used as the only\n> >> configuration file. If no configuration file is present, nothing\n> >> changes to the current libcamera behavior (except for some log\n> >> messages related to configuration file lookup).\n> >>\n> >> The configuration file is a YAML file. We already have a mechanism for\n> >> handling YAML configuration files in libcamera and the given\n> >> infrastructure can be reused for the purpose. However, the\n> >> configuration type is abstracted to make contingent future change of the\n> >> underlying class easier while retaining (most of) the original API.\n> >>\n> >> The configuration is versioned. This has currently no particular\n> >> meaning but is likely to have its purpose in future, especially once\n> >> configuration validation is introduced.\n> >>\n> >> The configuration YAML file looks as follows:\n> >>\n> >> ---\n> >> version: 1\n> >> configuration:\n> >> WHATEVER CONFIGURATION NEEDED\n> >>\n> >> There is no logging about reading the configuration file and contingent\n> >> errors. This is on purpose because logging will query configuration,\n> >> which can lead to various problems when done during configuration\n> >> initialization. Reporting the errors will be added later.\n> >>\n> >> The global configuration is intended to be used as a hidden singleton,\n> >> with static class methods around it providing the configuration\n> >> information. The constructor must be still public so that unique_ptr\n> >> can be used.\n> >>\n> >> A complication arises from the fact that when the configuration is\n> >> loaded, logging may be called and logging will ask for the\n> >> configuration. This is error-prone and may lead to subtle problems.\n> >> For this reason, the global configuration is instantiated to a pointer,\n> >> with an empty configuration initially. The real configuration will be\n> >> created through initialize() method. It will be clearer how it helps in\n> >> the followup patch introducing logging configuration.\n> >>\n> >> Logging is also the most notable component from base that uses global\n> >> configuration. In order to be able to do it, the global configuration\n> >> must be put to base.\n> >>\n> >> This patch introduces just the basic idea. Actually using the\n> >> configuration in the corresponding places (everything what is currently\n> >> configurable via environment variables should be configurable in the\n> >> file configuration) and other enhancements are implemented in the\n> >> followup patches.\n> >>\n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> .../libcamera/internal/global_configuration.h | 41 +++++\n> >> include/libcamera/internal/meson.build | 1 +\n> >> src/libcamera/base/global_configuration.cpp | 154 ++++++++++++++++++\n> >> src/libcamera/base/meson.build | 1 +\n> >> 4 files changed, 197 insertions(+)\n> >> create mode 100644 include/libcamera/internal/global_configuration.h\n> >> create mode 100644 src/libcamera/base/global_configuration.cpp\n> >>\n> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n> >> new file mode 100644\n> >> index 000000000..628d85cb8\n> >> --- /dev/null\n> >> +++ b/include/libcamera/internal/global_configuration.h\n> >> @@ -0,0 +1,41 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024 Red Hat, inc.\n> >> + *\n> >> + * global_configuration.h - Global configuration handling\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/yaml_parser.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +class GlobalConfiguration\n> >> +{\n> >> +public:\n> >> +\tusing Configuration = const YamlObject &;\n> >> +\n> >> +\t/* The constructor must be public to be able to use unique_ptr. */\n> >\n> > If one does\n> >\n> > std::unique_ptr(new T)\n> >\n> > then the constructor does not need to be public.\n> \n> Ah, cool.\n> \n> >> +\tGlobalConfiguration();\n> >> +\tstatic void initialize();\n> >> +\n> >> +\tstatic unsigned int version();\n> >> +\tstatic Configuration configuration();\n> >> +\n> >> +private:\n> >> +\tstatic const std::vector globalConfigurationFiles;\n> >> +\n> >> +\tstatic std::unique_ptr instance_;\n> >> +\n> >> +\tstd::unique_ptr configuration_;\n> >> +\n> >> +\tbool loadFile(const std::filesystem::path &fileName);\n> >> +\tvoid load();\n> >> +\tstatic Configuration get();\n> >> +};\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> >> index 1c5eef9ca..6fc6406d7 100644\n> >> --- a/include/libcamera/internal/meson.build\n> >> +++ b/include/libcamera/internal/meson.build\n> >> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n> >> 'dma_buf_allocator.h',\n> >> 'formats.h',\n> >> 'framebuffer.h',\n> >> + 'global_configuration.h',\n> >> 'ipa_data_serializer.h',\n> >> 'ipa_manager.h',\n> >> 'ipa_module.h',\n> >> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp\n> >> new file mode 100644\n> >> index 000000000..7dcf97265\n> >> --- /dev/null\n> >> +++ b/src/libcamera/base/global_configuration.cpp\n> >> @@ -0,0 +1,154 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024 Red Hat, inc.\n> >> + *\n> >> + * global_configuration.cpp - Global configuration handling\n> >> + */\n> >> +\n> >> +#include \"libcamera/internal/global_configuration.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/yaml_parser.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +std::unique_ptr GlobalConfiguration::instance_ =\n> >> +\tstd::make_unique();\n> >\n> > Why is a unique_ptr needed here?\n> \n> Most notably to stick with the libcamera policy of avoiding using plain\n> pointers. And for the instance disposal in initialize() below.\n> [...]\n\nI was thinking that there is no need to dynamically allocate a `GlobalConfiguration`\ninstance. In fact, now I think `GlobalConfiguration` could simply be a namespace.\nAll private members of the current class would go into an anonymous namespace,\nand all public members would just be functions in the namespace.\n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E07ABDB1C\n\tfor ;\n\tThu, 5 Dec 2024 14:06:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27484660FD;\n\tThu, 5 Dec 2024 15:06:23 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A4546608C\n\tfor ;\n\tThu, 5 Dec 2024 15:06:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"hsHwYrQD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1733407580; x=1733666780;\n\tbh=RNV+2pqlZEUXmIFa40/Ew5Qu3/dAe/Sn+tXhfsOwIM4=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=hsHwYrQD3uF93cHX58W2eRIzNxg4vAiQMJy07LmtJbufoPZy5r0fftHbt/HHfnhBL\n\tHYI470fuKY/pXIz3mbXNuyQ5pX6ZaA3j79OQ/GkKNvqHF0sfXZAx9KS8+CqOnbiOHm\n\tU5gQ6GruBg1HroN5Ygqy1jkxdWhYtmT+XA+Z4VEFXp7CJhF0yM9oVYIaW4Ok75qyqr\n\tFzMFDnKFAqikIv/lEBIlySkmlBCN9Jpn/v8I9f1vF4b3lv3rqi/SnicKnu94cyZe0V\n\twYb/sedKQoe1c0IxvbmY6q2my/VPDp7qxkKUqWw9d7EFhEdG3n90yUGX5nA7w9Sd3M\n\ttfSpmiPg+/1TA==","Date":"Thu, 05 Dec 2024 14:06:16 +0000","To":"Milan Zamazal ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tNaushir Patuck ","Subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","Message-ID":"","In-Reply-To":"<874j3i1i05.fsf@redhat.com>","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-4-mzamazal@redhat.com>\n\t\n\t<874j3i1i05.fsf@redhat.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"e0af8b64b00034389d80c8404b333b1f271112f9","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32551,"web_url":"https://patchwork.libcamera.org/comment/32551/","msgid":"<87cyi6ym5u.fsf@redhat.com>","date":"2024-12-05T19:17:33","subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Barnabás Pőcze writes:\n\n> Hi\n>\n>\n> 2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal írta:\n>\n>> Hi Barnabás,\n>> \n>> thank you for review.\n>> \n>> Barnabás Pőcze writes:\n>> \n>> > Hi\n>> >\n>> >\n>> > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n>> >\n>> >> Currently, libcamera can be configured in runtime using several\n>> >> environment variables. With introducing more and more variables, this\n>> >> mechanism reaches its limits. It would be simpler and more flexible if\n>> >> it was possible to configure libcamera in a single file.\n>> >>\n>> >> For example, there have been a request for defining pipeline precedence\n>> >> in runtime. We want to compile in multiple pipelines, in order to have\n>> >> them accessible within single packages in distributions. And then being\n>> >> able to select among the pipelines manually as needed based on the\n>> >> particular hardware or operating system environment. Having the\n>> >> configuration file then allows easy switching between hardware, GPU or\n>> >> CPU IPAs. Another possible use case is tuning image output, especially\n>> >> with software ISP, to user liking. For example, some users may prefer\n>> >> higher contrast without the need to use the corresponding knobs, if\n>> >> present at all, in every application. The configuration file can also\n>> >> be used to enable or disable experimental features and avoid the need to\n>> >> track local patches changing configuration options hard-wired in the\n>> >> code when working on new features.\n>> >>\n>> >> This patch introduces basic support for configuration files.\n>> >> GlobalConfiguration class reads, stores and accesses the configuration.\n>> >> Its instance is stored as a private singleton accessed using a static\n>> >> method of the class. There is currently no reason to have more than one\n>> >> instance.\n>> >>\n>> >> libcamera configuration can be specified using a system-wide\n>> >> configuration file or a user configuration file. The user configuration\n>> >> file takes precedence if present. There is currently no way to merge\n>> >> multiple configuration files, the one found is used as the only\n>> >> configuration file. If no configuration file is present, nothing\n>> >> changes to the current libcamera behavior (except for some log\n>> >> messages related to configuration file lookup).\n>> >>\n>> >> The configuration file is a YAML file. We already have a mechanism for\n>> >> handling YAML configuration files in libcamera and the given\n>> >> infrastructure can be reused for the purpose. However, the\n>> >> configuration type is abstracted to make contingent future change of the\n>> >> underlying class easier while retaining (most of) the original API.\n>> >>\n>> >> The configuration is versioned. This has currently no particular\n>> >> meaning but is likely to have its purpose in future, especially once\n>> >> configuration validation is introduced.\n>> >>\n>> >> The configuration YAML file looks as follows:\n>> >>\n>> >> ---\n>> >> version: 1\n>> >> configuration:\n>> >> WHATEVER CONFIGURATION NEEDED\n>> >>\n>> >> There is no logging about reading the configuration file and contingent\n>> >> errors. This is on purpose because logging will query configuration,\n>> >> which can lead to various problems when done during configuration\n>> >> initialization. Reporting the errors will be added later.\n>> >>\n>> >> The global configuration is intended to be used as a hidden singleton,\n>> >> with static class methods around it providing the configuration\n>> >> information. The constructor must be still public so that unique_ptr\n>> >> can be used.\n>> >>\n>> >> A complication arises from the fact that when the configuration is\n>> >> loaded, logging may be called and logging will ask for the\n>> >> configuration. This is error-prone and may lead to subtle problems.\n>> >> For this reason, the global configuration is instantiated to a pointer,\n>> >> with an empty configuration initially. The real configuration will be\n>> >> created through initialize() method. It will be clearer how it helps in\n>> >> the followup patch introducing logging configuration.\n>> >>\n>> >> Logging is also the most notable component from base that uses global\n>> >> configuration. In order to be able to do it, the global configuration\n>> >> must be put to base.\n>> >>\n>> >> This patch introduces just the basic idea. Actually using the\n>> >> configuration in the corresponding places (everything what is currently\n>> >> configurable via environment variables should be configurable in the\n>> >> file configuration) and other enhancements are implemented in the\n>> >> followup patches.\n>> >>\n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> .../libcamera/internal/global_configuration.h | 41 +++++\n>> >> include/libcamera/internal/meson.build | 1 +\n>> >> src/libcamera/base/global_configuration.cpp | 154 ++++++++++++++++++\n>> >> src/libcamera/base/meson.build | 1 +\n>> >> 4 files changed, 197 insertions(+)\n>> >> create mode 100644 include/libcamera/internal/global_configuration.h\n>> >> create mode 100644 src/libcamera/base/global_configuration.cpp\n>> >>\n>> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n>> >> new file mode 100644\n>> >> index 000000000..628d85cb8\n>> >> --- /dev/null\n>> >> +++ b/include/libcamera/internal/global_configuration.h\n>> >> @@ -0,0 +1,41 @@\n>> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> >> +/*\n>> >> + * Copyright (C) 2024 Red Hat, inc.\n>> >> + *\n>> >> + * global_configuration.h - Global configuration handling\n>> >> + */\n>> >> +\n>> >> +#pragma once\n>> >> +\n>> >> +#include \n>> >> +#include \n>> >> +\n>> >> +#include \"libcamera/internal/yaml_parser.h\"\n>> >> +\n>> >> +namespace libcamera {\n>> >> +\n>> >> +class GlobalConfiguration\n>> >> +{\n>> >> +public:\n>> >> +\tusing Configuration = const YamlObject &;\n>> >> +\n>> >> +\t/* The constructor must be public to be able to use unique_ptr. */\n>> >\n>> > If one does\n>> >\n>> > std::unique_ptr(new T)\n>> >\n>> > then the constructor does not need to be public.\n>> \n>> Ah, cool.\n>> \n>> >> +\tGlobalConfiguration();\n>> >> +\tstatic void initialize();\n>> >> +\n>> >> +\tstatic unsigned int version();\n>> >> +\tstatic Configuration configuration();\n>> >> +\n>> >> +private:\n>> >> +\tstatic const std::vector globalConfigurationFiles;\n>> >> +\n>> >> +\tstatic std::unique_ptr instance_;\n>> >> +\n>> >> +\tstd::unique_ptr configuration_;\n>> >> +\n>> >> +\tbool loadFile(const std::filesystem::path &fileName);\n>> >> +\tvoid load();\n>> >> +\tstatic Configuration get();\n>> >> +};\n>> >> +\n>> >> +} /* namespace libcamera */\n>> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> >> index 1c5eef9ca..6fc6406d7 100644\n>> >> --- a/include/libcamera/internal/meson.build\n>> >> +++ b/include/libcamera/internal/meson.build\n>> >> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n>> >> 'dma_buf_allocator.h',\n>> >> 'formats.h',\n>> >> 'framebuffer.h',\n>> >> + 'global_configuration.h',\n>> >> 'ipa_data_serializer.h',\n>> >> 'ipa_manager.h',\n>> >> 'ipa_module.h',\n>> >> diff --git a/src/libcamera/base/global_configuration.cpp\n>> >> b/src/libcamera/base/global_configuration.cpp\n>> >> new file mode 100644\n>> >> index 000000000..7dcf97265\n>> >> --- /dev/null\n>> >> +++ b/src/libcamera/base/global_configuration.cpp\n>> >> @@ -0,0 +1,154 @@\n>> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> >> +/*\n>> >> + * Copyright (C) 2024 Red Hat, inc.\n>> >> + *\n>> >> + * global_configuration.cpp - Global configuration handling\n>> >> + */\n>> >> +\n>> >> +#include \"libcamera/internal/global_configuration.h\"\n>> >> +\n>> >> +#include \n>> >> +#include \n>> >> +#include \n>> >> +\n>> >> +#include \n>> >> +#include \n>> >> +#include \n>> >> +\n>> >> +#include \"libcamera/internal/yaml_parser.h\"\n>> >> +\n>> >> +namespace libcamera {\n>> >> +\n>> >> +std::unique_ptr GlobalConfiguration::instance_ =\n>> >> +\tstd::make_unique();\n>> >\n>> > Why is a unique_ptr needed here?\n>> \n>> Most notably to stick with the libcamera policy of avoiding using plain\n>> pointers. And for the instance disposal in initialize() below.\n>> [...]\n>\n> I was thinking that there is no need to dynamically allocate a `GlobalConfiguration`\n> instance. In fact, now I think `GlobalConfiguration` could simply be a namespace.\n> All private members of the current class would go into an anonymous namespace,\n> and all public members would just be functions in the namespace.\n\nI see, we can get rid of `instance_' this way and just store the\nconfiguration. OK, this would be probably better.\n\n>\n> Regards,\n> Barnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4BB8BC323E\n\tfor ;\n\tThu, 5 Dec 2024 19:17:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D92F6610E;\n\tThu, 5 Dec 2024 20:17:41 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 463E66608C\n\tfor ;\n\tThu, 5 Dec 2024 20:17:40 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-154-nIWGAv7ENIano2lzXmkHVA-1; Thu, 05 Dec 2024 14:17:38 -0500","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-434a04437cdso8286225e9.2\n\tfor ;\n\tThu, 05 Dec 2024 11:17:37 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-434d5280497sm69206385e9.23.2024.12.05.11.17.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 05 Dec 2024 11:17:34 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"G/8tlPdA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1733426259;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=I2mSEADvXNVWoAjSBtBVeDpC9JuBc8ikLHYzmTvmofg=;\n\tb=G/8tlPdAugyatA5TyZPLxMWEUif6BFcSug0Q6l0bbc0pc3HrX/+OJuP1XqzZmgr/jy0z5d\n\tog0UK8YHZYpD3oFZrkNMAm3qs40HPIpasNxw9ICUSxggp2l7WPxSZ/wdtRycFcTIPlI54i\n\twyG/Xyu3egJPgOB/w+s1Je52415+URs=","X-MC-Unique":"nIWGAv7ENIano2lzXmkHVA-1","X-Mimecast-MFC-AGG-ID":"nIWGAv7ENIano2lzXmkHVA","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733426256; x=1734031056;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=0JTgDC2RoYSd53OkrjAMBqBkEW32EnNtSg4HWtJV7Xs=;\n\tb=s304g3JDuwsKseWDhatJHoscTvtwx1opIFPYWIr6+VbGEGd4xcCJe1qr8Vk9Ci6iXE\n\tMqOVhp2y4+Zc8BH06q+C0XEDmVLJE1HPbwfErjIx/Htw11YiSXjYVY+x+9ChXVayLEDS\n\tTBKcd7lIdi6v3Fm6s5C0WVqAtgdt8joet9Yvn1aceVb7Ia9v4WqAIEvoZx4pOeZe0jCb\n\t8a7zCAYA2mUtvmBgiBeGSSG+0W/CQJj2xQnW6C+LHAIJBJ0nhb/UijGl90mIaoJS/c3N\n\tyA/VsP7gDsEABbLa6Wa3KqCAhTtj2Q0OIHtOrW9iy8x4khhkJ0PqAeKh1BEqAXES8zEN\n\tqFYQ==","X-Gm-Message-State":"AOJu0YzOlrQnWwvV6Q/gXZ+isvzH3Q/PSNVWo7icOXj6IEa1fgS2nWxJ\n\tMZodF1VrMu0N09oze84Wl+AKxJJeRef6JZpplgwFSz04BziryHRqIeicGfj0n2qK1gtJzPdWuhp\n\tjqcQSh3/ytimh1UEgBqazYlctQYvJaH4EQyBOyCs4yY969HuNR8Zrj285LxrLx4B5+Bz6zjnNHs\n\te5e9A=","X-Gm-Gg":"ASbGncvHEtOHxfLa+LCjqVk/IDJV6+T6umz52On5GHPjuYJuxpex1jkpzJ6BkHFORrV\n\twh5tE2h6D8eHbfYp5rOjyOmjPwf3LrrdHj5oijizAWLwDUl2x+/hRnDxE5z2c7HRzbvzU4QTIrV\n\tBgQkzKGkZ5z6Lw27oc90eOuKqX5DL7jN20xEImUXCRboZhRlNKGXXAPkufzQaZlWntZdIcqEgEr\n\trHvaWttVuXF4NfKNzX3xdJWXQqdL/96QMtUTedMnaOJqt8UMsT6CTTuVb6Am4Qmu46Ak50=","X-Received":["by 2002:a05:600c:1388:b0:434:a239:d2fe with SMTP id\n\t5b1f17b1804b1-434dded5da5mr3303685e9.28.1733426256323; \n\tThu, 05 Dec 2024 11:17:36 -0800 (PST)","by 2002:a05:600c:1388:b0:434:a239:d2fe with SMTP id\n\t5b1f17b1804b1-434dded5da5mr3303545e9.28.1733426255889; \n\tThu, 05 Dec 2024 11:17:35 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFn5dHHEyPsIPc+Y+tUdGpplbNQ9Ce1JKqt8EY/4sLx6MFWa7gNbnZwxbzMtGcL9uXdKy+WMQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Naushir Patuck ","Subject":"Re: [PATCH v5 03/15] config: Introduce global runtime configuration","In-Reply-To":"\n\t( =?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Thu,\n\t05 Dec 2024 14:06:16 +0000\")","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-4-mzamazal@redhat.com>\n\t\n\t<874j3i1i05.fsf@redhat.com>\n\t","Date":"Thu, 05 Dec 2024 20:17:33 +0100","Message-ID":"<87cyi6ym5u.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"MV9ye_G6vx8zcTJ1dediAWAOsrodN12EsqPX1pLjLVk_1733426257","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]