[v11,02/12] config: Make GlobalConfiguration instance

23629 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

2025. 06. 24. 10:35 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.

I think one possible approach is the following:

   (1) when the DSO is loaded, initialize logging based on the environmental variables;
   (2) when a CameraManager is constructed, apply the logging related configuration.

Of course if multiple `CameraManager`s instantiated, the logging related
settings of the last will "win". But I don't think this is an issue in
practice.

I think we could also get rid of the global logger in many places by
having a `Logger` interface that is propagated to object. There are
still classes like `ControlList`, which, I would say, exist outside
the umbrella of the `CameraManager` class, so a global logger instance
is unavoidable if such classes want to use logging. This of course
brings up the problem of log categories, which are also singleton.

I believe it could be useful in some cases because an application could
itself supply such a `Logger` instance (on a per CameraManager basis)
and redirect libcamera logs into its own stream of log messages if it so wishes.


Regards,
Barnabás Pőcze


> 
> 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(+)
> 
> 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

Hi Barnabás,

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

> Hi
>
> 2025. 06. 24. 10:35 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.
>
> I think one possible approach is the following:
>
>   (1) when the DSO is loaded, initialize logging based on the environmental variables;
>   (2) when a CameraManager is constructed, apply the logging related configuration.

I tried to follow this suggestion from you when working on v9 but
eventually decided to leave it aside to not complicate the series with
more stuff.  I have the WIP patches and may be able to try again once
the basic support is merged; but if the plan is to remove the global
logger then I think it'd be wasted work.  But I can add the idea to the
commit message.

> Of course if multiple `CameraManager`s instantiated, the logging related
> settings of the last will "win". But I don't think this is an issue in
> practice.
>
> I think we could also get rid of the global logger in many places by
> having a `Logger` interface that is propagated to object. There are
> still classes like `ControlList`, which, I would say, exist outside
> the umbrella of the `CameraManager` class, so a global logger instance
> is unavoidable if such classes want to use logging. This of course
> brings up the problem of log categories, which are also singleton.
>
> I believe it could be useful in some cases because an application could
> itself supply such a `Logger` instance (on a per CameraManager basis)
> and redirect libcamera logs into its own stream of log messages if it so wishes.

Having Logger in CameraManager would allow passing the configuration to
a Logger constructor, which would make things easier.

> Regards,
> Barnabás Pőcze
>
>
>> 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(+)
>> 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