Message ID | 20250912142915.53949-3-mzamazal@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
On Fri, Sep 12, 2025 at 04:29:03PM +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. I think it's actually optimal :-) But let's not dwell on that, the result looks good to me, no need to update the commit message. > 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 the private CameraManager. It's > accessible within libcamera (via CameraManager) but it's not meant to be > accessed by applications. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/camera_manager.h | 12 ++++++++++-- > src/libcamera/camera_manager.cpp | 8 ++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > index 5dfbe1f65..b8b2966b5 100644 > --- a/include/libcamera/internal/camera_manager.h > +++ b/include/libcamera/internal/camera_manager.h > @@ -7,8 +7,6 @@ > > #pragma once > > -#include <libcamera/camera_manager.h> > - This was done on purpose, to ensure that camera_manager.h is self-contained. I'll restore this when applying the patch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > #include <memory> > #include <sys/types.h> > #include <vector> > @@ -18,6 +16,9 @@ > #include <libcamera/base/thread.h> > #include <libcamera/base/thread_annotations.h> > > +#include <libcamera/camera_manager.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: > @@ -65,6 +71,8 @@ private: > std::unique_ptr<DeviceEnumerator> enumerator_; > > std::unique_ptr<IPAManager> ipaManager_; > + > + 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
diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h index 5dfbe1f65..b8b2966b5 100644 --- a/include/libcamera/internal/camera_manager.h +++ b/include/libcamera/internal/camera_manager.h @@ -7,8 +7,6 @@ #pragma once -#include <libcamera/camera_manager.h> - #include <memory> #include <sys/types.h> #include <vector> @@ -18,6 +16,9 @@ #include <libcamera/base/thread.h> #include <libcamera/base/thread_annotations.h> +#include <libcamera/camera_manager.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: @@ -65,6 +71,8 @@ private: std::unique_ptr<DeviceEnumerator> enumerator_; std::unique_ptr<IPAManager> ipaManager_; + + 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