[v2,6/8] libcamera: camera: Hook into the LayerManager
diff mbox series

Message ID 20250703114225.2074071-7-paul.elder@ideasonboard.com
State New
Headers show
Series
  • Add Layers support
Related show

Commit Message

Paul Elder July 3, 2025, 11:42 a.m. UTC
Add hooks into the CameraManager to call into the LayerManager on all
relevant public-facing interface of Camera.

The entry point for each function into the LayerManager is has been
chosen based on the capabilities that we want to allow to Layer
implementations.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- move the LayerManager out of Camera into the CameraManager
- remove hooks for generateConfiguration() and streams()
- Camera now passes itself into the layer hooks, as it is required for
  the LayerManager to fetch the closure based on the camera to pass to
  each layer
---
 src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

Comments

Kieran Bingham July 23, 2025, 5:13 p.m. UTC | #1
Quoting Paul Elder (2025-07-03 12:42:21)
> Add hooks into the CameraManager to call into the LayerManager on all
> relevant public-facing interface of Camera.
> 
> The entry point for each function into the LayerManager is has been
> chosen based on the capabilities that we want to allow to Layer
> implementations.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - move the LayerManager out of Camera into the CameraManager
> - remove hooks for generateConfiguration() and streams()
> - Camera now passes itself into the layer hooks, as it is required for
>   the LayerManager to fetch the closure based on the camera to pass to
>   each layer
> ---
>  src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c3e656cab671..665fc5157349 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/thread.h>
>  
> +#include <libcamera/camera_manager.h>
>  #include <libcamera/color_space.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/framebuffer_allocator.h>
> @@ -27,6 +28,8 @@
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/camera_manager.h"
> +#include "libcamera/internal/layer_manager.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/request.h"
>  
> @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)
>  void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
>  {
>         Camera *camera = _o<Camera>();
> +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->bufferCompleted(camera, request, buffer);

I think we need some sort of Layers controller in the Camera class
directly. It should be responsible for the specific instances of the
Layers and the ordering/enablement from the (future) camera specific
configuration.

With that - these additions would be much 'cleaner' as well as we could
expect:

	layerController->bufferCompleted(...)

Which would be a part of the Camera::Private or something rather than
having to indirectly obtain the layerManager each time.

Otherwise - if I've mis-understood and the layerManager is already doing
that functionality - perhaps we should have a reference/instance to it
in the Camera::Private class so we don't have to grab it tyhrough a
pipe, cameraManager and private instance each time....

>         camera->bufferCompleted.emit(request, buffer);
>  }
>  
>  void Camera::Private::emitRequestCompleted(Request *request)
>  {
>         Camera *camera = _o<Camera>();
> +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->requestCompleted(camera, request);
>         camera->requestCompleted.emit(request);
>  }
>  
>  void Camera::Private::emitDisconnected()
>  {
>         Camera *camera = _o<Camera>();
> +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->disconnected(camera);
>         camera->disconnected.emit();
>  }
>  
> @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr<Private> d, const std::string &id,
>         _d()->id_ = id;
>         _d()->streams_ = streams;
>         _d()->validator_ = std::make_unique<CameraControlValidator>(this);
> +       _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,
> +                                                                 _d()->properties_,
> +                                                                 _d()->controlInfo_);
>  }
>  
>  Camera::~Camera()
>  {
> +       _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);

	Somehow I'd call the Camera::Private 'layers' though so we could
	do a nice short:

	layers->terminate(this);

>  }
>  
>  /**
> @@ -1032,6 +1045,9 @@ int Camera::acquire()
>                 return -EBUSY;
>         }
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->acquire(this);
> +
>         d->setState(Private::CameraAcquired);
>  
>         return 0;
> @@ -1064,6 +1080,9 @@ int Camera::release()
>                 d->pipe_->invokeMethod(&PipelineHandler::release,
>                                        ConnectionTypeBlocking, this);
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->release(this);
> +
>         d->setState(Private::CameraAvailable);
>  
>         return 0;
> @@ -1081,7 +1100,8 @@ int Camera::release()
>   */
>  const ControlInfoMap &Camera::controls() const
>  {
> -       return _d()->controlInfo_;
> +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> +       return layerManager->controls(this);

Somehow I'm surprised we're not passing in _d()->controlInfo() here ...
	return layerManager->controls(_d()->controlInfo_);

Does the layer manager access the existing camera control Info directly
somehow?

>  }
>  
>  /**
> @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const
>   */
>  const ControlList &Camera::properties() const
>  {
> -       return _d()->properties_;
> +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> +       return layerManager->properties(this);

Same thoughts as controls but perhaps it's just handled differently to
how I expect and the answer is elsewhere.


>  }
>  
>  /**
> @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)
>                 d->activeStreams_.insert(stream);
>         }
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->configure(this, config, d->controlInfo_);
> +
>         d->setState(Private::CameraConfigured);
>  
>         return 0;
> @@ -1282,6 +1306,9 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
>         /* Associate the request with the pipeline handler. */
>         d->pipe_->registerRequest(request.get());
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->createRequest(this, cookie, request.get());
> +
>         return request;
>  }
>  
> @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)
>                 }
>         }
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->queueRequest(this, request);
> +
>         d->pipe_->invokeMethod(&PipelineHandler::queueRequest,
>                                ConnectionTypeQueued, request);
>  
> @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)
>  
>         ASSERT(d->requestSequence_ == 0);
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->start(this, controls);
> +
>         ret = d->pipe_->invokeMethod(&PipelineHandler::start,
>                                      ConnectionTypeBlocking, this, controls);
>         if (ret)
> @@ -1446,6 +1479,9 @@ int Camera::stop()
>  
>         d->setState(Private::CameraStopping);
>  
> +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> +       layerManager->stop(this);
> +

I wonder if symetrically - we should stop the layers after stopping the
camera, as part of the stop call on the camera will invoke completion(or
cancellation?)?

I'm not entirely certain on this one - but it's just that as a wrapper
I think I'd expect:

  layers->start()
    camera->start()
    camera->stop()
  layers->stop
?

>         d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
>                                this);
>  
> -- 
> 2.47.2
>
Kieran Bingham July 23, 2025, 5:30 p.m. UTC | #2
Quoting Kieran Bingham (2025-07-23 18:13:08)
> Quoting Paul Elder (2025-07-03 12:42:21)
> > Add hooks into the CameraManager to call into the LayerManager on all
> > relevant public-facing interface of Camera.
> > 
> > The entry point for each function into the LayerManager is has been
> > chosen based on the capabilities that we want to allow to Layer
> > implementations.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - move the LayerManager out of Camera into the CameraManager

Oh - interesting. I think we do need a top level loader - but probably
per-camera instances of the Layers...

> > - remove hooks for generateConfiguration() and streams()
> > - Camera now passes itself into the layer hooks, as it is required for
> >   the LayerManager to fetch the closure based on the camera to pass to
> >   each layer
> > ---
> >  src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index c3e656cab671..665fc5157349 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -18,6 +18,7 @@
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/thread.h>
> >  
> > +#include <libcamera/camera_manager.h>
> >  #include <libcamera/color_space.h>
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/framebuffer_allocator.h>
> > @@ -27,6 +28,8 @@
> >  
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_controls.h"
> > +#include "libcamera/internal/camera_manager.h"
> > +#include "libcamera/internal/layer_manager.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >  #include "libcamera/internal/request.h"
> >  
> > @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)
> >  void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
> >  {
> >         Camera *camera = _o<Camera>();
> > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->bufferCompleted(camera, request, buffer);
> 
> I think we need some sort of Layers controller in the Camera class
> directly. It should be responsible for the specific instances of the
> Layers and the ordering/enablement from the (future) camera specific
> configuration.
> 
> With that - these additions would be much 'cleaner' as well as we could
> expect:
> 
>         layerController->bufferCompleted(...)
> 
> Which would be a part of the Camera::Private or something rather than
> having to indirectly obtain the layerManager each time.
> 
> Otherwise - if I've mis-understood and the layerManager is already doing
> that functionality - perhaps we should have a reference/instance to it
> in the Camera::Private class so we don't have to grab it tyhrough a
> pipe, cameraManager and private instance each time....
> 
> >         camera->bufferCompleted.emit(request, buffer);
> >  }
> >  
> >  void Camera::Private::emitRequestCompleted(Request *request)
> >  {
> >         Camera *camera = _o<Camera>();
> > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->requestCompleted(camera, request);
> >         camera->requestCompleted.emit(request);
> >  }
> >  
> >  void Camera::Private::emitDisconnected()
> >  {
> >         Camera *camera = _o<Camera>();
> > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->disconnected(camera);
> >         camera->disconnected.emit();
> >  }
> >  
> > @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr<Private> d, const std::string &id,
> >         _d()->id_ = id;
> >         _d()->streams_ = streams;
> >         _d()->validator_ = std::make_unique<CameraControlValidator>(this);
> > +       _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,
> > +                                                                 _d()->properties_,
> > +                                                                 _d()->controlInfo_);
> >  }
> >  
> >  Camera::~Camera()
> >  {
> > +       _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);
> 
>         Somehow I'd call the Camera::Private 'layers' though so we could
>         do a nice short:
> 
>         layers->terminate(this);
> 
> >  }
> >  
> >  /**
> > @@ -1032,6 +1045,9 @@ int Camera::acquire()
> >                 return -EBUSY;
> >         }
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->acquire(this);
> > +
> >         d->setState(Private::CameraAcquired);
> >  
> >         return 0;
> > @@ -1064,6 +1080,9 @@ int Camera::release()
> >                 d->pipe_->invokeMethod(&PipelineHandler::release,
> >                                        ConnectionTypeBlocking, this);
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->release(this);
> > +
> >         d->setState(Private::CameraAvailable);
> >  
> >         return 0;
> > @@ -1081,7 +1100,8 @@ int Camera::release()
> >   */
> >  const ControlInfoMap &Camera::controls() const
> >  {
> > -       return _d()->controlInfo_;
> > +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> > +       return layerManager->controls(this);
> 
> Somehow I'm surprised we're not passing in _d()->controlInfo() here ...
>         return layerManager->controls(_d()->controlInfo_);
> 
> Does the layer manager access the existing camera control Info directly
> somehow?

Wait - now I read the previous review comments. It's because the layers
aren't going to modify the camera state - so they get indirected/merged
of course...

I'd forgotten about that.

> 
> >  }
> >  
> >  /**
> > @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const
> >   */
> >  const ControlList &Camera::properties() const
> >  {
> > -       return _d()->properties_;
> > +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> > +       return layerManager->properties(this);
> 
> Same thoughts as controls but perhaps it's just handled differently to
> how I expect and the answer is elsewhere.

And same here of course ;-)


> 
> 
> >  }
> >  
> >  /**
> > @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)
> >                 d->activeStreams_.insert(stream);
> >         }
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->configure(this, config, d->controlInfo_);
> > +
> >         d->setState(Private::CameraConfigured);
> >  
> >         return 0;
> > @@ -1282,6 +1306,9 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> >         /* Associate the request with the pipeline handler. */
> >         d->pipe_->registerRequest(request.get());
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->createRequest(this, cookie, request.get());
> > +
> >         return request;
> >  }
> >  
> > @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)
> >                 }
> >         }
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->queueRequest(this, request);
> > +
> >         d->pipe_->invokeMethod(&PipelineHandler::queueRequest,
> >                                ConnectionTypeQueued, request);
> >  
> > @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)
> >  
> >         ASSERT(d->requestSequence_ == 0);
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->start(this, controls);
> > +
> >         ret = d->pipe_->invokeMethod(&PipelineHandler::start,
> >                                      ConnectionTypeBlocking, this, controls);
> >         if (ret)
> > @@ -1446,6 +1479,9 @@ int Camera::stop()
> >  
> >         d->setState(Private::CameraStopping);
> >  
> > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > +       layerManager->stop(this);
> > +
> 
> I wonder if symetrically - we should stop the layers after stopping the
> camera, as part of the stop call on the camera will invoke completion(or
> cancellation?)?
> 
> I'm not entirely certain on this one - but it's just that as a wrapper
> I think I'd expect:
> 
>   layers->start()
>     camera->start()
>     camera->stop()
>   layers->stop
> ?
> 
> >         d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> >                                this);
> >  
> > -- 
> > 2.47.2
> >
Paul Elder July 24, 2025, 8:28 a.m. UTC | #3
Quoting Kieran Bingham (2025-07-24 02:30:38)
> Quoting Kieran Bingham (2025-07-23 18:13:08)
> > Quoting Paul Elder (2025-07-03 12:42:21)
> > > Add hooks into the CameraManager to call into the LayerManager on all
> > > relevant public-facing interface of Camera.
> > > 
> > > The entry point for each function into the LayerManager is has been
> > > chosen based on the capabilities that we want to allow to Layer
> > > implementations.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v2:
> > > - move the LayerManager out of Camera into the CameraManager
> 
> Oh - interesting. I think we do need a top level loader - but probably

Yeah.

> per-camera instances of the Layers...

Hm yeah I guess we do need that in order to support per-camera layer queues.
Might as well do it now since it's a pretty core architectural change.

> 
> > > - remove hooks for generateConfiguration() and streams()
> > > - Camera now passes itself into the layer hooks, as it is required for
> > >   the LayerManager to fetch the closure based on the camera to pass to
> > >   each layer
> > > ---
> > >  src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index c3e656cab671..665fc5157349 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -18,6 +18,7 @@
> > >  #include <libcamera/base/log.h>
> > >  #include <libcamera/base/thread.h>
> > >  
> > > +#include <libcamera/camera_manager.h>
> > >  #include <libcamera/color_space.h>
> > >  #include <libcamera/control_ids.h>
> > >  #include <libcamera/framebuffer_allocator.h>
> > > @@ -27,6 +28,8 @@
> > >  
> > >  #include "libcamera/internal/camera.h"
> > >  #include "libcamera/internal/camera_controls.h"
> > > +#include "libcamera/internal/camera_manager.h"
> > > +#include "libcamera/internal/layer_manager.h"
> > >  #include "libcamera/internal/pipeline_handler.h"
> > >  #include "libcamera/internal/request.h"
> > >  
> > > @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)
> > >  void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
> > >  {
> > >         Camera *camera = _o<Camera>();
> > > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->bufferCompleted(camera, request, buffer);
> > 
> > I think we need some sort of Layers controller in the Camera class
> > directly. It should be responsible for the specific instances of the
> > Layers and the ordering/enablement from the (future) camera specific
> > configuration.
> > 
> > With that - these additions would be much 'cleaner' as well as we could
> > expect:
> > 
> >         layerController->bufferCompleted(...)
> > 
> > Which would be a part of the Camera::Private or something rather than
> > having to indirectly obtain the layerManager each time.
> > 
> > Otherwise - if I've mis-understood and the layerManager is already doing
> > that functionality - perhaps we should have a reference/instance to it
> > in the Camera::Private class so we don't have to grab it tyhrough a
> > pipe, cameraManager and private instance each time....
> > 
> > >         camera->bufferCompleted.emit(request, buffer);
> > >  }
> > >  
> > >  void Camera::Private::emitRequestCompleted(Request *request)
> > >  {
> > >         Camera *camera = _o<Camera>();
> > > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->requestCompleted(camera, request);
> > >         camera->requestCompleted.emit(request);
> > >  }
> > >  
> > >  void Camera::Private::emitDisconnected()
> > >  {
> > >         Camera *camera = _o<Camera>();
> > > +       LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->disconnected(camera);
> > >         camera->disconnected.emit();
> > >  }
> > >  
> > > @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr<Private> d, const std::string &id,
> > >         _d()->id_ = id;
> > >         _d()->streams_ = streams;
> > >         _d()->validator_ = std::make_unique<CameraControlValidator>(this);
> > > +       _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,
> > > +                                                                 _d()->properties_,
> > > +                                                                 _d()->controlInfo_);
> > >  }
> > >  
> > >  Camera::~Camera()
> > >  {
> > > +       _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);
> > 
> >         Somehow I'd call the Camera::Private 'layers' though so we could
> >         do a nice short:
> > 
> >         layers->terminate(this);
> > 
> > >  }
> > >  
> > >  /**
> > > @@ -1032,6 +1045,9 @@ int Camera::acquire()
> > >                 return -EBUSY;
> > >         }
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->acquire(this);
> > > +
> > >         d->setState(Private::CameraAcquired);
> > >  
> > >         return 0;
> > > @@ -1064,6 +1080,9 @@ int Camera::release()
> > >                 d->pipe_->invokeMethod(&PipelineHandler::release,
> > >                                        ConnectionTypeBlocking, this);
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->release(this);
> > > +
> > >         d->setState(Private::CameraAvailable);
> > >  
> > >         return 0;
> > > @@ -1081,7 +1100,8 @@ int Camera::release()
> > >   */
> > >  const ControlInfoMap &Camera::controls() const
> > >  {
> > > -       return _d()->controlInfo_;
> > > +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> > > +       return layerManager->controls(this);
> > 
> > Somehow I'm surprised we're not passing in _d()->controlInfo() here ...
> >         return layerManager->controls(_d()->controlInfo_);
> > 
> > Does the layer manager access the existing camera control Info directly
> > somehow?
> 
> Wait - now I read the previous review comments. It's because the layers
> aren't going to modify the camera state - so they get indirected/merged
> of course...
> 
> I'd forgotten about that.
> 
> > 
> > >  }
> > >  
> > >  /**
> > > @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const
> > >   */
> > >  const ControlList &Camera::properties() const
> > >  {
> > > -       return _d()->properties_;
> > > +       LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
> > > +       return layerManager->properties(this);
> > 
> > Same thoughts as controls but perhaps it's just handled differently to
> > how I expect and the answer is elsewhere.
> 
> And same here of course ;-)
> 
> 
> > 
> > 
> > >  }
> > >  
> > >  /**
> > > @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)
> > >                 d->activeStreams_.insert(stream);
> > >         }
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->configure(this, config, d->controlInfo_);
> > > +
> > >         d->setState(Private::CameraConfigured);
> > >  
> > >         return 0;
> > > @@ -1282,6 +1306,9 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > >         /* Associate the request with the pipeline handler. */
> > >         d->pipe_->registerRequest(request.get());
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->createRequest(this, cookie, request.get());
> > > +
> > >         return request;
> > >  }
> > >  
> > > @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)
> > >                 }
> > >         }
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->queueRequest(this, request);
> > > +
> > >         d->pipe_->invokeMethod(&PipelineHandler::queueRequest,
> > >                                ConnectionTypeQueued, request);
> > >  
> > > @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)
> > >  
> > >         ASSERT(d->requestSequence_ == 0);
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->start(this, controls);
> > > +
> > >         ret = d->pipe_->invokeMethod(&PipelineHandler::start,
> > >                                      ConnectionTypeBlocking, this, controls);
> > >         if (ret)
> > > @@ -1446,6 +1479,9 @@ int Camera::stop()
> > >  
> > >         d->setState(Private::CameraStopping);
> > >  
> > > +       LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
> > > +       layerManager->stop(this);
> > > +
> > 
> > I wonder if symetrically - we should stop the layers after stopping the
> > camera, as part of the stop call on the camera will invoke completion(or
> > cancellation?)?
> > 
> > I'm not entirely certain on this one - but it's just that as a wrapper
> > I think I'd expect:
> > 
> >   layers->start()
> >     camera->start()
> >     camera->stop()
> >   layers->stop
> > ?

Good point, yeah.


Thanks,

Paul

> > 
> > >         d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> > >                                this);
> > >  
> > > -- 
> > > 2.47.2
> > >

Patch
diff mbox series

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index c3e656cab671..665fc5157349 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -18,6 +18,7 @@ 
 #include <libcamera/base/log.h>
 #include <libcamera/base/thread.h>
 
+#include <libcamera/camera_manager.h>
 #include <libcamera/color_space.h>
 #include <libcamera/control_ids.h>
 #include <libcamera/framebuffer_allocator.h>
@@ -27,6 +28,8 @@ 
 
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/camera_controls.h"
+#include "libcamera/internal/camera_manager.h"
+#include "libcamera/internal/layer_manager.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/request.h"
 
@@ -741,18 +744,24 @@  void Camera::Private::setState(State state)
 void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
 {
 	Camera *camera = _o<Camera>();
+	LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
+	layerManager->bufferCompleted(camera, request, buffer);
 	camera->bufferCompleted.emit(request, buffer);
 }
 
 void Camera::Private::emitRequestCompleted(Request *request)
 {
 	Camera *camera = _o<Camera>();
+	LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
+	layerManager->requestCompleted(camera, request);
 	camera->requestCompleted.emit(request);
 }
 
 void Camera::Private::emitDisconnected()
 {
 	Camera *camera = _o<Camera>();
+	LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();
+	layerManager->disconnected(camera);
 	camera->disconnected.emit();
 }
 
@@ -943,10 +952,14 @@  Camera::Camera(std::unique_ptr<Private> d, const std::string &id,
 	_d()->id_ = id;
 	_d()->streams_ = streams;
 	_d()->validator_ = std::make_unique<CameraControlValidator>(this);
+	_d()->pipe()->cameraManager()->_d()->layerManager()->init(this,
+								  _d()->properties_,
+								  _d()->controlInfo_);
 }
 
 Camera::~Camera()
 {
+	_d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);
 }
 
 /**
@@ -1032,6 +1045,9 @@  int Camera::acquire()
 		return -EBUSY;
 	}
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->acquire(this);
+
 	d->setState(Private::CameraAcquired);
 
 	return 0;
@@ -1064,6 +1080,9 @@  int Camera::release()
 		d->pipe_->invokeMethod(&PipelineHandler::release,
 				       ConnectionTypeBlocking, this);
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->release(this);
+
 	d->setState(Private::CameraAvailable);
 
 	return 0;
@@ -1081,7 +1100,8 @@  int Camera::release()
  */
 const ControlInfoMap &Camera::controls() const
 {
-	return _d()->controlInfo_;
+	LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
+	return layerManager->controls(this);
 }
 
 /**
@@ -1094,7 +1114,8 @@  const ControlInfoMap &Camera::controls() const
  */
 const ControlList &Camera::properties() const
 {
-	return _d()->properties_;
+	LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();
+	return layerManager->properties(this);
 }
 
 /**
@@ -1242,6 +1263,9 @@  int Camera::configure(CameraConfiguration *config)
 		d->activeStreams_.insert(stream);
 	}
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->configure(this, config, d->controlInfo_);
+
 	d->setState(Private::CameraConfigured);
 
 	return 0;
@@ -1282,6 +1306,9 @@  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
 	/* Associate the request with the pipeline handler. */
 	d->pipe_->registerRequest(request.get());
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->createRequest(this, cookie, request.get());
+
 	return request;
 }
 
@@ -1366,6 +1393,9 @@  int Camera::queueRequest(Request *request)
 		}
 	}
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->queueRequest(this, request);
+
 	d->pipe_->invokeMethod(&PipelineHandler::queueRequest,
 			       ConnectionTypeQueued, request);
 
@@ -1402,6 +1432,9 @@  int Camera::start(const ControlList *controls)
 
 	ASSERT(d->requestSequence_ == 0);
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->start(this, controls);
+
 	ret = d->pipe_->invokeMethod(&PipelineHandler::start,
 				     ConnectionTypeBlocking, this, controls);
 	if (ret)
@@ -1446,6 +1479,9 @@  int Camera::stop()
 
 	d->setState(Private::CameraStopping);
 
+	LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();
+	layerManager->stop(this);
+
 	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
 			       this);