| Message ID | 20250611142431.33306-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 06. 11. 16:24 keltezéssel, Milan Zamazal írta: > Global configuration is accessed via a GlobalConfiguration instance. > The instance is conceptually a singleton, but singletons are not welcome > in libcamera so we must store the (preferably single) instance > somewhere. > > This patch creates a GlobalConfiguration instance in CameraManager and > defines the corresponding access method. CameraManager is typically > instantiated only once or a few times, it is accessible in > many places in libcamera and the configuration can be retrieved from it > and passed to other places if needed (it's read-only once created). > Using CameraManager for the purpose is still suboptimal and we use it > only due to lack of better options. An alternative could be Logger, > which is still a singleton and it's accessible from everywhere. But > with Logger, we have a chicken and egg problem -- GlobalConfiguration > should log contingent problems with the configuration when it's loaded > but if it is created in the logger then there are mutual infinite > recursive calls. Perhaps some acceptable workaround could be found, > which would also allow an easier logging configuration. > > If there are multiple CameraManager instances, there are also multiple > GlobalConfiguration instances. They may or may not contain the same > data, depending on whether the global configuration file in the file > system was changed in the meantime. This is dirty and a potential > source of trouble (although not expected to have much impact in > practice) but there is no idea how to do better if we want to avoid > having GlobalConfiguration singleton or dealing with the logging > problem. > > The configuration is stored in CameraManager privately, it's not meant > to be accessed by applications. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/camera_manager.h | 5 +++++ > src/libcamera/camera_manager.cpp | 11 +++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > index 0150ca61f..f03fe6343 100644 > --- a/include/libcamera/internal/camera_manager.h > +++ b/include/libcamera/internal/camera_manager.h > @@ -18,6 +18,7 @@ > #include <libcamera/base/thread.h> > #include <libcamera/base/thread_annotations.h> > > +#include "libcamera/internal/global_configuration.h" > #include "libcamera/internal/process.h" > > namespace libcamera { > @@ -38,6 +39,8 @@ public: > void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_); > void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_); > > + const GlobalConfiguration &configuration() const; It's quite trivial, so could you inline the function? > + > IPAManager *ipaManager() const { return ipaManager_.get(); } > > protected: > @@ -66,6 +69,8 @@ private: > > std::unique_ptr<IPAManager> ipaManager_; > ProcessManager processManager_; > + > + const GlobalConfiguration configuration_{ GlobalConfiguration() }; I think you can omit the `{ GlobalConfiguration() }` part. Regards, Barnabás Pőcze > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index e62e7193c..7361915bf 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -15,6 +15,7 @@ > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/device_enumerator.h" > +#include "libcamera/internal/global_configuration.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/pipeline_handler.h" > > @@ -254,6 +255,16 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera) > o->cameraRemoved.emit(camera); > } > > +/** > + * \brief Get global configuration bound to the camera manager > + * > + * \return Reference to the configuration > + */ > +const GlobalConfiguration &CameraManager::Private::configuration() const > +{ > + return configuration_; > +} > + > /** > * \fn CameraManager::Private::ipaManager() const > * \brief Retrieve the IPAManager > -- > 2.49.0 >
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. Changes in v9: - The configuration instance is now stored in CameraManager and accessed from there rather than being a separate singleton wrapped by global accessors. This solves the ugly problem of delayed initialization but I don’t like anything else about it. I played a bit with the idea of attaching it to Logger instead, see the commit message of patch 03 for some discussion, but stayed with the CameraManager proposal eventually. - I’ve given up on the logging configuration now when the configuration is stored in CameraManager and removed the corresponding patches. I think it’s possible to add it later, but for now, it’s already complicated enough. - Not much tested, let’s see first if the current implementation gets in an acceptable direction. 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 (13): yaml: Move yaml_parser.cpp to base config: Introduce global runtime configuration config: Make GlobalConfiguration instance config: Add configuration retrieval helpers 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} | 119 +++++++- include/libcamera/internal/camera_manager.h | 5 + .../libcamera/internal/global_configuration.h | 72 +++++ include/libcamera/internal/ipa_manager.h | 8 +- include/libcamera/internal/ipa_proxy.h | 5 +- include/libcamera/internal/meson.build | 1 + src/libcamera/base/global_configuration.cpp | 268 ++++++++++++++++++ src/libcamera/base/meson.build | 15 + src/libcamera/{ => base}/yaml_parser.cpp | 13 +- src/libcamera/camera_manager.cpp | 22 +- src/libcamera/ipa_manager.cpp | 37 ++- src/libcamera/ipa_proxy.cpp | 72 ++--- src/libcamera/meson.build | 14 - src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 2 + src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- .../pipeline/rpi/common/pipeline_base.cpp | 11 +- src/libcamera/pipeline/simple/simple.cpp | 13 + src/libcamera/pipeline/vimc/vimc.cpp | 3 +- src/libcamera/software_isp/TODO | 36 --- src/libcamera/software_isp/debayer_cpu.cpp | 32 ++- src/libcamera/software_isp/debayer_cpu.h | 10 +- src/libcamera/software_isp/software_isp.cpp | 6 +- test/ipa/ipa_interface_test.cpp | 3 +- .../module_ipa_proxy.cpp.tmpl | 4 +- .../module_ipa_proxy.h.tmpl | 2 +- 29 files changed, 633 insertions(+), 153 deletions(-) rename Documentation/{environment_variables.rst => runtime_configuration.rst} (60%) 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%)