[v16,02/12] config: Make GlobalConfiguration instance
diff mbox series

Message ID 20250729073201.5369-3-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal July 29, 2025, 7:31 a.m. UTC
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.
One possible way to deal with this is to look at the environment
variables only during logging initialisation and apply the logging
configuration when a CameraManager is constructed.  Considering there
are intentions to remove the Logger singleton, let's omit logging
configuration for now.

If there are multiple CameraManager instances, there are also multiple
GlobalConfiguration instances, each CameraManager instance is meant to
be fully independent, including configuration.  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.

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

Comments

Paul Elder Sept. 8, 2025, 10:24 a.m. UTC | #1
Hi Milan,

Thanks for the patch.

Quoting Milan Zamazal (2025-07-29 16:31:50)
> 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.
> One possible way to deal with this is to look at the environment
> variables only during logging initialisation and apply the logging
> configuration when a CameraManager is constructed.  Considering there
> are intentions to remove the Logger singleton, let's omit logging
> configuration for now.

I think putting it in CameraManager is fine. We have other singletons like
IPAManager and LayerManager living there. It's probably better than making it
static.

> 
> If there are multiple CameraManager instances, there are also multiple
> GlobalConfiguration instances, each CameraManager instance is meant to
> be fully independent, including configuration.  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.
> 
> The configuration is stored in CameraManager privately, it's not meant

s/in CameraManager privately/in the private CameraManager/ maybe? "Privately"
makes it sound like it goes in the private section of the CameraManager class.

> to be accessed by applications.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

The commit title I think should be
"libcamera: camera_manager: Construct GlobalConfiguration instance"
as this is touching camera_manager code, not global_configuration code.

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.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 f81794bfd..dca3d9a83 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"
>  
> @@ -258,6 +259,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
> -- 
> 2.50.1
>
Milan Zamazal Sept. 8, 2025, 1:48 p.m. UTC | #2
Hi Paul,

thank you for review.

Paul Elder <paul.elder@ideasonboard.com> writes:

> Hi Milan,
>
> Thanks for the patch.
>
> Quoting Milan Zamazal (2025-07-29 16:31:50)
>> 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.
>> One possible way to deal with this is to look at the environment
>> variables only during logging initialisation and apply the logging
>> configuration when a CameraManager is constructed.  Considering there
>> are intentions to remove the Logger singleton, let's omit logging
>> configuration for now.
>
> I think putting it in CameraManager is fine. We have other singletons like
> IPAManager and LayerManager living there. It's probably better than making it
> static.
>
>> 
>> If there are multiple CameraManager instances, there are also multiple
>> GlobalConfiguration instances, each CameraManager instance is meant to
>> be fully independent, including configuration.  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.
>> 
>> The configuration is stored in CameraManager privately, it's not meant
>
> s/in CameraManager privately/in the private CameraManager/ maybe? "Privately"
> makes it sound like it goes in the private section of the CameraManager class.

Yes, it's meant to be accessible from within libcamera, I'll change the
wording.

>> to be accessed by applications.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>
> The commit title I think should be
> "libcamera: camera_manager: Construct GlobalConfiguration instance"
> as this is touching camera_manager code, not global_configuration code.

OK.

> Looks good to me.
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.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 f81794bfd..dca3d9a83 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"
>>  
>> @@ -258,6 +259,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
>> -- 
>> 2.50.1
>>

Patch
diff mbox series

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 f81794bfd..dca3d9a83 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"
 
@@ -258,6 +259,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