[v10,03/13] config: Make GlobalConfiguration instance

23566 diff mbox series

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 | 8 ++++++++
 src/libcamera/camera_manager.cpp            | 8 ++++++++
 2 files changed, 16 insertions(+)

Hi Milan,

Thank you for the patch.

On Mon, Jun 16, 2025 at 10:47:21AM +0200, Milan Zamazal wrote:
> 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.

Why is it sub-optimal ? CameraManager is really the central component in
libcamera, so I think it's the natural location to handle the global
configuration file. The fact that the logger is a singleton is something
I would ideally like to fix.

> 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.

I think it would make sense to let applications pass a custom
configuration file to the CameraManager when creating it, so I don't
really think this is an issue, or dirty. If multiple instances of
CameraManager are created, they should be completely independent from
each other.

The implementation looks fine. Depending on the outcome of the
discussion in 02/13 about error handling, we may need to propagate
errors to applications.

> 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 | 8 ++++++++
>  src/libcamera/camera_manager.cpp            | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index 0150ca61f..e8082eb34 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,11 @@ 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
> +	{
> +		return configuration_;
> +	}
> +
>  	IPAManager *ipaManager() const { return ipaManager_.get(); }
>  
>  protected:
> @@ -66,6 +72,8 @@ private:
>  
>  	std::unique_ptr<IPAManager> ipaManager_;
>  	ProcessManager processManager_;
> +
> +	const GlobalConfiguration configuration_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index e62e7193c..f3b4ec708 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,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>  	o->cameraRemoved.emit(camera);
>  }
>  
> +/**
> + * \fn const GlobalConfiguration &CameraManager::Private::configuration() const
> + * \brief Get global configuration bound to the camera manager
> + *
> + * \return Reference to the configuration
> + */
> +
>  /**
>   * \fn CameraManager::Private::ipaManager() const
>   * \brief Retrieve the IPAManager