[PATCH v8 00/15] Add global configuration file

[Milan Zamazal]

Hi Barnabás,

Barnabás Pőcze <barnabas.pocze at ideasonboard.com> writes:

> Hi
>
>
> 2025. 04. 30. 14:14 keltezéssel, Milan Zamazal írta:
>> This patch series introduces global configuration file for libcamera, to
>> provide runtime configuration means other than environment variables.
>> Instead of, or in addition to, the growing list of configuration
>> environment variables, the whole configuration can be specified in a
>> single configuration file.  This is both simpler and more flexible.
>> This is not a replacement for specific configuration files already
>> present in libcamera.
>> The patches implement what is needed to introduce a configuration file
>> that can handle the current environment variables and software ISP
>> TODOs.  They demonstrate how to deal with the key points that must be
>> considered.  See commit messages for more details.
>> The configuration file is a YAML file.  It is looked up in user’s home
>> directory or, if not present, in system-wide libcamera directories.
>> Both the directories and file name of the configuration file can be
>> overridden using newly introduced environment variables.
>> The current configuration environment variables are still supported and
>> take precedence, if defined, over the corresponding entries in the YAML
>> file.
>> This patch series is not exhaustive, there can be future enhancements,
>> most notably configuration file validation to avoid confusions caused by
>> typos etc.
>> Not everything has been tested because some of the patches are related
>> to specific hardware.
>> Also, not everything is necessarily C++ elegant, suggestions for
>> improvements are welcome.  Note that there is an obstacle in that
>> configuration initialization may call logging and logging queries
>> configuration, as explained in the commit messages.  This complicates
>> transparent encapsulation.
>
> I think generally the direction is to eliminate singletons from libcamera.[0][1]
> So I am not sure that a new one should be introduced. I believe we should proceed
> towards a design where the configuration is per-CameraManager (e.g. the constructor
> loads the configuration), and then it is propagated to other components. (As far as
> I can see, this shouldn't be too much trouble: pipeline handlers can reach the CameraManager
> easily, and `DebayerCpu` can receive the settings in its constructor via `SoftwareISP`
> from the simple pipeline handler, `IPAManager` can receive it in its constructor).

I'm not sure we want the configuration being bound to CameraManager; the
configuration can contain anything and logging may be just one example
of something more global.  I'd prefer to avoid reworking the
configuration mechanism in future.

> Secondly, I would consider dropping the logging related settings from the configuration
> file, at least for now. It is likely not the most interesting part, 

I think logging belongs to the configuration and is a good thing to
think about as a proof of the design.  Even if we possibly eventually
decide to omit it due to technical difficulties, we should think about
the overall design more first and have a good answer to the question
"why other software can configure logging in its configuration file and
libcamera not?".

> and even with the singleton design, I am not fan of the quite fragile
> `GlobalConfiguration::initialize()` call in the `IPAManager`
> constructor.

I suppose nobody is but the question is how to do better without
imposing artificial restrictions on the configuration and/or introducing
another ugliness.

> (With per-CameraManager configuration, I could
> imagine that the constructor initializes the logger based on the configuration. Of
> course if two CameraManagers load different configuration, then it is entirely possible
> that they will overwrite each other's log settings, but unless we want to inject
> separate logger objects into each object of interest, I don't really see a nice
> solution; but it's probably not a big issue anyways.)

>From the practical point of view, the problem of different
configurations is indeed unlikely to be a big issue, the configuration
would change only if the user edited it.  But a design raising such
questions smells and sounds like a call for trouble (configuration
changing in runtime unexpectedly, possibly in incompatible/unexpected
ways, ...), I'd rather avoid it.

> Thoughts?
>
> [0]: https://bugs.libcamera.org/show_bug.cgi?id=246
> [1]: https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049888.html
>
>
> Regards,
> Barnabás Pőcze
>
>
>> Changes in v8:
>> - Rebased on current master.
>> - Anniversary edition: 400 days since v1 posted. ;-)
>> Changes in v7:
>> - Rebased on current master.
>> - Tuning file path configuration updated for recent changes.
>>    A significant change is that the tuning file configuration is no
>>    longer sensor dependent as there is apparently no access to the sensor
>>    info in the IPA proxy.
>> - Minor improvements of some commit messages.
>> Changes in v6:
>> - Rebased on master.
>> - File names from file header descriptions removed.
>> - Indentation fix in moved code as requested by checkstyle.py.
>> - Unneeded const_cast's removed.
>> - Using GlobalConfiguration namespace rather than a class.
>> - Path configuration options are defined as sequences in YAML.
>> - A patch introducing LIBCAMERA_CONFIG_NAME added.
>> - A patch introducing LIBCAMERA_CONFIG_DIR added.
>> - Miscellaneous minor code changes suggested by Barnabás.
>> Changes in v5:
>> - A pointer is used to store the global configuration singleton rather
>>    than a static variable.  This makes the things more robust and fixes
>>    the problem with re-entrancy on logging and a failing
>>    camera_reconfigure test.
>> - In relation to the change above, a new initialization method
>>    GlobalConfiguration::initialize() was introduced that replaces the
>>    initialization calls in CameraManager and IPAManager.
>> - Logging YAML errors when reading the configuration was also fixed.
>> - Global configuration is placed to base directly, without an
>>    intermediate patch.
>> - An `optional' value comparison simplified.
>> - A temporary typo in a comment fixed.
>> - Unused stdint.h include removed.
>> Changes in v4:
>> - Rebased on current master.
>> - Configuration option for LIBCAMERA_IPA_PROXY_PATH added.
>> - Added a patch to include stdlib.h instead of cstdlib in yaml_parser.cpp.
>> Changes in v3:
>> - Added a configuration item for the newly introduced
>>    LIBCAMERA_PIPELINES_MATCH_LIST variable.
>> - A minor indentation fix.
>> - Fixed `pipelines.' x `pipeline.' configuration item naming mismatch.
>> - Tuning files are looked up now by particular cameras attached rather than
>>    being specified for the whole pipeline.
>> - Helpers use std::string& instead of char* for confPath arguments.
>> - Protection against returning YamlObject::empty as a regular value (the
>>    problem has been probably exposed by addition of
>>    LIBCAMERA_PIPELINES_MATCH_LIST).
>> Changes in v2:
>> - Rebased on master.
>> - Various cleanups, documentation improvements and minor fixes.
>> - Configuration option for LIBCAMERA_RPI_TUNING_FILE added (Naush).
>> - Two more patches for software ISP configuration added.
>> Milan Zamazal (15):
>>    yaml: Move yaml_parser.cpp to base
>>    config: Introduce global runtime configuration
>>    config: Look up logging levels in the configuration file
>>    config: Add configuration retrieval helpers
>>    config: Look up log file in configuration file
>>    config: Look up log color configuration in configuration file
>>    config: Look up rpi config path in configuration file
>>    config: Look up IPA configurables in configuration file
>>    config: Look up pipelines match list in configuration file
>>    config: Allow enabling software ISP in runtime
>>    config: Add global configuration file documentation
>>    libcamera: software_isp: Make input buffer copying configurable
>>    libcamera: software_isp: Make measurement configurable
>>    config: Make configuration file configurable
>>    config: Make configuration directories configurable
>>   Documentation/documentation-contents.rst      |   2 +-
>>   Documentation/index.rst                       |   2 +-
>>   Documentation/meson.build                     |   2 +-
>>   ...ariables.rst => runtime_configuration.rst} | 133 +++++++-
>>   .../libcamera/internal/global_configuration.h |  60 ++++
>>   include/libcamera/internal/meson.build        |   1 +
>>   src/libcamera/base/global_configuration.cpp   | 289 ++++++++++++++++++
>>   src/libcamera/base/log.cpp                    |  40 +--
>>   src/libcamera/base/meson.build                |  15 +
>>   src/libcamera/{ => base}/yaml_parser.cpp      |  13 +-
>>   src/libcamera/camera_manager.cpp              |  10 +-
>>   src/libcamera/ipa_manager.cpp                 |  35 ++-
>>   src/libcamera/ipa_proxy.cpp                   |  65 ++--
>>   src/libcamera/meson.build                     |  14 -
>>   .../pipeline/rpi/common/pipeline_base.cpp     |   8 +-
>>   src/libcamera/pipeline/simple/simple.cpp      |  12 +
>>   src/libcamera/process.cpp                     |  11 +-
>>   src/libcamera/software_isp/TODO               |  36 ---
>>   src/libcamera/software_isp/debayer_cpu.cpp    |  30 +-
>>   src/libcamera/software_isp/debayer_cpu.h      |   7 +-
>>   20 files changed, 630 insertions(+), 155 deletions(-)
>>   rename Documentation/{environment_variables.rst => runtime_configuration.rst} (58%)
>>   create mode 100644 include/libcamera/internal/global_configuration.h
>>   create mode 100644 src/libcamera/base/global_configuration.cpp
>>   rename src/libcamera/{ => base}/yaml_parser.cpp (98%)
>>