Message ID | 20250703114225.2074071-6-paul.elder@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Quoting Paul Elder (2025-07-03 12:42:20) > Add to the CameraManager a LayerManager member. This allows us to have > one LayerManager that handles loading all the layer shared objects, and > then each Camera can access the layer via the LayerManager via through > the CameraManager. I'm not sure if that's right ? Wouldn't each Camera be able to decide to have different layers enabled (perhaps as part of the configuration file). I.e. ... should we enable layers per-camera ? For instance - a sync algo layer - would be per camera with specific options on each camera instance. Although - we likely want to only load the shared objects once - and then use them on a configurable basis in the camera object... So I think the LayerManager is probably accurate for being the loading mechanism - and that indeed can be part of the CameraManager as they have to be loaded before the Cameras are constructed... So I think this patch indeed is suitable... but I wonder how the other patches might adapt around it or later. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v2 > --- > include/libcamera/internal/camera_manager.h | 3 +++ > src/libcamera/camera_manager.cpp | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > index 0150ca61f7de..ef5b0688de23 100644 > --- a/include/libcamera/internal/camera_manager.h > +++ b/include/libcamera/internal/camera_manager.h > @@ -25,6 +25,7 @@ namespace libcamera { > class Camera; > class DeviceEnumerator; > class IPAManager; > +class LayerManager; > class PipelineHandlerFactoryBase; > > class CameraManager::Private : public Extensible::Private, public Thread > @@ -39,6 +40,7 @@ public: > void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_); > > IPAManager *ipaManager() const { return ipaManager_.get(); } > + LayerManager *layerManager() const { return layerManager_.get(); } > > protected: > void run() override; > @@ -66,6 +68,7 @@ private: > > std::unique_ptr<IPAManager> ipaManager_; > ProcessManager processManager_; > + std::unique_ptr<LayerManager> layerManager_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index e62e7193cfdc..0f202fc6314e 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/layer_manager.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/pipeline_handler.h" > > @@ -41,6 +42,7 @@ CameraManager::Private::Private() > : initialized_(false) > { > ipaManager_ = std::make_unique<IPAManager>(); > + layerManager_ = std::make_unique<LayerManager>(); > } > > int CameraManager::Private::start() > -- > 2.47.2 >
Quoting Kieran Bingham (2025-07-24 01:49:08) > Quoting Paul Elder (2025-07-03 12:42:20) > > Add to the CameraManager a LayerManager member. This allows us to have > > one LayerManager that handles loading all the layer shared objects, and > > then each Camera can access the layer via the LayerManager via through > > the CameraManager. > > I'm not sure if that's right ? Wouldn't each Camera be able to decide to > have different layers enabled (perhaps as part of the configuration > file). We'd need the configuration file for that. For the time being it's by environment variable, so all layers will be enabled for all cameras. > > I.e. ... should we enable layers per-camera ? After we get configuration file support, yes. > > For instance - a sync algo layer - would be per camera with specific > options on each camera instance. This also needs configuration file support. At the moment (also how Raspberry Pi does it), there's a control to tell the sync algo whether it's server or client, but all the other sync algo parameters are hardcoded and need configuration file support to support configuration. > > Although - we likely want to only load the shared objects once - and > then use them on a configurable basis in the camera object... Yes. > > So I think the LayerManager is probably accurate for being the loading > mechanism - and that indeed can be part of the CameraManager as they > have to be loaded before the Cameras are constructed... > > So I think this patch indeed is suitable... but I wonder how the other > patches might adapt around it or later. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks, Paul > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v2 > > --- > > include/libcamera/internal/camera_manager.h | 3 +++ > > src/libcamera/camera_manager.cpp | 2 ++ > > 2 files changed, 5 insertions(+) > > > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > > index 0150ca61f7de..ef5b0688de23 100644 > > --- a/include/libcamera/internal/camera_manager.h > > +++ b/include/libcamera/internal/camera_manager.h > > @@ -25,6 +25,7 @@ namespace libcamera { > > class Camera; > > class DeviceEnumerator; > > class IPAManager; > > +class LayerManager; > > class PipelineHandlerFactoryBase; > > > > class CameraManager::Private : public Extensible::Private, public Thread > > @@ -39,6 +40,7 @@ public: > > void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_); > > > > IPAManager *ipaManager() const { return ipaManager_.get(); } > > + LayerManager *layerManager() const { return layerManager_.get(); } > > > > protected: > > void run() override; > > @@ -66,6 +68,7 @@ private: > > > > std::unique_ptr<IPAManager> ipaManager_; > > ProcessManager processManager_; > > + std::unique_ptr<LayerManager> layerManager_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index e62e7193cfdc..0f202fc6314e 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/layer_manager.h" > > #include "libcamera/internal/ipa_manager.h" > > #include "libcamera/internal/pipeline_handler.h" > > > > @@ -41,6 +42,7 @@ CameraManager::Private::Private() > > : initialized_(false) > > { > > ipaManager_ = std::make_unique<IPAManager>(); > > + layerManager_ = std::make_unique<LayerManager>(); > > } > > > > int CameraManager::Private::start() > > -- > > 2.47.2 > >
diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h index 0150ca61f7de..ef5b0688de23 100644 --- a/include/libcamera/internal/camera_manager.h +++ b/include/libcamera/internal/camera_manager.h @@ -25,6 +25,7 @@ namespace libcamera { class Camera; class DeviceEnumerator; class IPAManager; +class LayerManager; class PipelineHandlerFactoryBase; class CameraManager::Private : public Extensible::Private, public Thread @@ -39,6 +40,7 @@ public: void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_); IPAManager *ipaManager() const { return ipaManager_.get(); } + LayerManager *layerManager() const { return layerManager_.get(); } protected: void run() override; @@ -66,6 +68,7 @@ private: std::unique_ptr<IPAManager> ipaManager_; ProcessManager processManager_; + std::unique_ptr<LayerManager> layerManager_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index e62e7193cfdc..0f202fc6314e 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/layer_manager.h" #include "libcamera/internal/ipa_manager.h" #include "libcamera/internal/pipeline_handler.h" @@ -41,6 +42,7 @@ CameraManager::Private::Private() : initialized_(false) { ipaManager_ = std::make_unique<IPAManager>(); + layerManager_ = std::make_unique<LayerManager>(); } int CameraManager::Private::start()
Add to the CameraManager a LayerManager member. This allows us to have one LayerManager that handles loading all the layer shared objects, and then each Camera can access the layer via the LayerManager via through the CameraManager. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New in v2 --- include/libcamera/internal/camera_manager.h | 3 +++ src/libcamera/camera_manager.cpp | 2 ++ 2 files changed, 5 insertions(+)