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

[Barnabás Pőcze]

Hi


2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:

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

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