[v2,5/8] libcamera: camera_manager: Add LayerManager

23735 diff mbox series

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

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