[libcamera-devel,v2,2/4] libcamera: camera_manager: Move {add, remove}Camera to internal
diff mbox series

Message ID 20230511225833.361699-3-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Add new Camera devices property
Related show

Commit Message

Kieran Bingham May 11, 2023, 10:58 p.m. UTC
The CameraManager exposes addCamera and removeCamera as public API
calls, while they should never be called from an application. These
calls are only expected to be used by PipelineHandlers to update the
CameraManager that a new Camera has been created and allow the Camera
Manager to expose it to applications.

Remove the public calls and update the private implementations such that
they can be used directly by the PipelineHandler through the internal
CameraManager::Private provided by the Extensible class.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/camera_manager.h          |  4 -
 include/libcamera/internal/camera_manager.h |  2 +-
 src/libcamera/camera_manager.cpp            | 87 +++++++++------------
 src/libcamera/pipeline_handler.cpp          |  6 +-
 4 files changed, 43 insertions(+), 56 deletions(-)

Comments

Jacopo Mondi May 13, 2023, 2:57 p.m. UTC | #1
Hi Kieran

On Thu, May 11, 2023 at 11:58:31PM +0100, Kieran Bingham via libcamera-devel wrote:
> The CameraManager exposes addCamera and removeCamera as public API
> calls, while they should never be called from an application. These
> calls are only expected to be used by PipelineHandlers to update the
> CameraManager that a new Camera has been created and allow the Camera
> Manager to expose it to applications.
>
> Remove the public calls and update the private implementations such that
> they can be used directly by the PipelineHandler through the internal
> CameraManager::Private provided by the Extensible class.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/camera_manager.h          |  4 -
>  include/libcamera/internal/camera_manager.h |  2 +-
>  src/libcamera/camera_manager.cpp            | 87 +++++++++------------
>  src/libcamera/pipeline_handler.cpp          |  6 +-
>  4 files changed, 43 insertions(+), 56 deletions(-)
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 4b1fb7568e83..9767acc42c89 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -34,10 +34,6 @@ public:
>  	std::shared_ptr<Camera> get(const std::string &id);
>  	std::shared_ptr<Camera> get(dev_t devnum);
>
> -	void addCamera(std::shared_ptr<Camera> camera,
> -		       const std::vector<dev_t> &devnums);
> -	void removeCamera(std::shared_ptr<Camera> camera);
> -
>  	static const std::string &version() { return version_; }
>
>  	Signal<std::shared_ptr<Camera>> cameraAdded;
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index 4bba3ac2eb29..8386e90e0f60 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -31,7 +31,7 @@ public:
>  	int start();
>  	void addCamera(std::shared_ptr<Camera> camera,
>  		       const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> -	void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +	void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>
>  	/*
>  	 * This mutex protects
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index b95284ba5a80..34f09a4d181b 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -148,9 +148,25 @@ void CameraManager::Private::cleanup()
>  	enumerator_.reset(nullptr);
>  }
>
> +/**
> + * \brief Add a camera to the camera manager
> + * \param[in] camera The camera to be added
> + * \param[in] devnums The device numbers to associate with \a camera
> + *
> + * This function is called by pipeline handlers to register the cameras they
> + * handle with the camera manager. Registered cameras are immediately made
> + * available to the system.
> + *
> + * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> + * to Camera instances.
> + *
> + * \context This function shall be called from the CameraManager thread.
> + */
>  void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
>  				       const std::vector<dev_t> &devnums)
>  {
> +	ASSERT(Thread::current() == this);
> +

Is this still needed now that this function is only available to the
internal library componentes ?

>  	MutexLocker locker(mutex_);
>
>  	for (const std::shared_ptr<Camera> &c : cameras_) {
> @@ -167,15 +183,31 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
>  	unsigned int index = cameras_.size() - 1;
>  	for (dev_t devnum : devnums)
>  		camerasByDevnum_[devnum] = cameras_[index];
> +
> +	/* Report the addition to the public signal */
> +	CameraManager *const o = LIBCAMERA_O_PTR();
> +	o->cameraAdded.emit(cameras_[index]);
>  }
>
> -void CameraManager::Private::removeCamera(Camera *camera)
> +/**
> + * \brief Remove a camera from the camera manager
> + * \param[in] camera The camera to be removed
> + *
> + * This function is called by pipeline handlers to unregister cameras from the
> + * camera manager. Unregistered cameras won't be reported anymore by the
> + * cameras() and get() calls, but references may still exist in applications.
> + *
> + * \context This function shall be called from the CameraManager thread.

ditto as per above

> + */
> +void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>  {
> +	ASSERT(Thread::current() == this);
> +

same here

>  	MutexLocker locker(mutex_);
>
>  	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
>  				 [camera](std::shared_ptr<Camera> &c) {
> -					 return c.get() == camera;
> +					 return c.get() == camera.get();
>  				 });
>  	if (iter == cameras_.end())
>  		return;
> @@ -185,12 +217,16 @@ void CameraManager::Private::removeCamera(Camera *camera)
>
>  	auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
>  				   [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> -					   return p.second.lock().get() == camera;
> +					   return p.second.lock().get() == camera.get();
>  				   });
>  	if (iter_d != camerasByDevnum_.end())
>  		camerasByDevnum_.erase(iter_d);
>
>  	cameras_.erase(iter);
> +
> +	/* Report the removal to the public signal */
> +	CameraManager *const o = LIBCAMERA_O_PTR();
> +	o->cameraRemoved.emit(camera);
>  }
>
>  /**
> @@ -383,51 +419,6 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>   * perform any blocking operation.
>   */
>
> -/**
> - * \brief Add a camera to the camera manager
> - * \param[in] camera The camera to be added
> - * \param[in] devnums The device numbers to associate with \a camera
> - *
> - * This function is called by pipeline handlers to register the cameras they
> - * handle with the camera manager. Registered cameras are immediately made
> - * available to the system.
> - *
> - * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> - * to Camera instances.
> - *
> - * \context This function shall be called from the CameraManager thread.
> - */
> -void CameraManager::addCamera(std::shared_ptr<Camera> camera,
> -			      const std::vector<dev_t> &devnums)
> -{
> -	Private *const d = _d();
> -
> -	ASSERT(Thread::current() == d);
> -
> -	d->addCamera(camera, devnums);
> -	cameraAdded.emit(camera);
> -}
> -
> -/**
> - * \brief Remove a camera from the camera manager
> - * \param[in] camera The camera to be removed
> - *
> - * This function is called by pipeline handlers to unregister cameras from the
> - * camera manager. Unregistered cameras won't be reported anymore by the
> - * cameras() and get() calls, but references may still exist in applications.
> - *
> - * \context This function shall be called from the CameraManager thread.
> - */
> -void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
> -{
> -	Private *const d = _d();
> -
> -	ASSERT(Thread::current() == d);
> -
> -	d->removeCamera(camera.get());
> -	cameraRemoved.emit(camera);
> -}
> -
>  /**
>   * \fn const std::string &CameraManager::version()
>   * \brief Retrieve the libcamera version string
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index f72613b8e515..49092ea88a58 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -16,10 +16,10 @@
>  #include <libcamera/base/utils.h>
>
>  #include <libcamera/camera.h>
> -#include <libcamera/camera_manager.h>
>  #include <libcamera/framebuffer.h>
>
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/camera_manager.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/media_device.h"
> @@ -624,7 +624,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
>  		}
>  	}
>
> -	manager_->addCamera(std::move(camera), devnums);
> +	manager_->_d()->addCamera(std::move(camera), devnums);
>  }
>
>  /**
> @@ -691,7 +691,7 @@ void PipelineHandler::disconnect()
>  			continue;
>
>  		camera->disconnect();
> -		manager_->removeCamera(camera);
> +		manager_->_d()->removeCamera(camera);

All minors as well
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j

>  	}
>  }
>
> --
> 2.34.1
>
Kieran Bingham May 13, 2023, 3:37 p.m. UTC | #2
Quoting Jacopo Mondi via libcamera-devel (2023-05-13 15:57:45)
> Hi Kieran
> 
> On Thu, May 11, 2023 at 11:58:31PM +0100, Kieran Bingham via libcamera-devel wrote:
> > The CameraManager exposes addCamera and removeCamera as public API
> > calls, while they should never be called from an application. These
> > calls are only expected to be used by PipelineHandlers to update the
> > CameraManager that a new Camera has been created and allow the Camera
> > Manager to expose it to applications.
> >
> > Remove the public calls and update the private implementations such that
> > they can be used directly by the PipelineHandler through the internal
> > CameraManager::Private provided by the Extensible class.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/camera_manager.h          |  4 -
> >  include/libcamera/internal/camera_manager.h |  2 +-
> >  src/libcamera/camera_manager.cpp            | 87 +++++++++------------
> >  src/libcamera/pipeline_handler.cpp          |  6 +-
> >  4 files changed, 43 insertions(+), 56 deletions(-)
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 4b1fb7568e83..9767acc42c89 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -34,10 +34,6 @@ public:
> >       std::shared_ptr<Camera> get(const std::string &id);
> >       std::shared_ptr<Camera> get(dev_t devnum);
> >
> > -     void addCamera(std::shared_ptr<Camera> camera,
> > -                    const std::vector<dev_t> &devnums);
> > -     void removeCamera(std::shared_ptr<Camera> camera);
> > -
> >       static const std::string &version() { return version_; }
> >
> >       Signal<std::shared_ptr<Camera>> cameraAdded;
> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> > index 4bba3ac2eb29..8386e90e0f60 100644
> > --- a/include/libcamera/internal/camera_manager.h
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -31,7 +31,7 @@ public:
> >       int start();
> >       void addCamera(std::shared_ptr<Camera> camera,
> >                      const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > -     void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +     void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >
> >       /*
> >        * This mutex protects
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index b95284ba5a80..34f09a4d181b 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -148,9 +148,25 @@ void CameraManager::Private::cleanup()
> >       enumerator_.reset(nullptr);
> >  }
> >
> > +/**
> > + * \brief Add a camera to the camera manager
> > + * \param[in] camera The camera to be added
> > + * \param[in] devnums The device numbers to associate with \a camera
> > + *
> > + * This function is called by pipeline handlers to register the cameras they
> > + * handle with the camera manager. Registered cameras are immediately made
> > + * available to the system.
> > + *
> > + * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> > + * to Camera instances.
> > + *
> > + * \context This function shall be called from the CameraManager thread.
> > + */
> >  void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >                                      const std::vector<dev_t> &devnums)
> >  {
> > +     ASSERT(Thread::current() == this);
> > +
> 
> Is this still needed now that this function is only available to the
> internal library componentes ?

It was there before, and will be compiled out for release so I don't
think it hurts. If anything it ensures the same guarantee for new
pipeline handlers that get developed, so I'd keep it, (and the others).


> 
> >       MutexLocker locker(mutex_);
> >
> >       for (const std::shared_ptr<Camera> &c : cameras_) {
> > @@ -167,15 +183,31 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >       unsigned int index = cameras_.size() - 1;
> >       for (dev_t devnum : devnums)
> >               camerasByDevnum_[devnum] = cameras_[index];
> > +
> > +     /* Report the addition to the public signal */
> > +     CameraManager *const o = LIBCAMERA_O_PTR();
> > +     o->cameraAdded.emit(cameras_[index]);
> >  }
> >
> > -void CameraManager::Private::removeCamera(Camera *camera)
> > +/**
> > + * \brief Remove a camera from the camera manager
> > + * \param[in] camera The camera to be removed
> > + *
> > + * This function is called by pipeline handlers to unregister cameras from the
> > + * camera manager. Unregistered cameras won't be reported anymore by the
> > + * cameras() and get() calls, but references may still exist in applications.
> > + *
> > + * \context This function shall be called from the CameraManager thread.
> 
> ditto as per above
> 
> > + */
> > +void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
> >  {
> > +     ASSERT(Thread::current() == this);
> > +
> 
> same here
> 
> >       MutexLocker locker(mutex_);
> >
> >       auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> >                                [camera](std::shared_ptr<Camera> &c) {
> > -                                      return c.get() == camera;
> > +                                      return c.get() == camera.get();
> >                                });
> >       if (iter == cameras_.end())
> >               return;
> > @@ -185,12 +217,16 @@ void CameraManager::Private::removeCamera(Camera *camera)
> >
> >       auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> >                                  [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> > -                                        return p.second.lock().get() == camera;
> > +                                        return p.second.lock().get() == camera.get();
> >                                  });
> >       if (iter_d != camerasByDevnum_.end())
> >               camerasByDevnum_.erase(iter_d);
> >
> >       cameras_.erase(iter);
> > +
> > +     /* Report the removal to the public signal */
> > +     CameraManager *const o = LIBCAMERA_O_PTR();
> > +     o->cameraRemoved.emit(camera);
> >  }
> >
> >  /**
> > @@ -383,51 +419,6 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> >   * perform any blocking operation.
> >   */
> >
> > -/**
> > - * \brief Add a camera to the camera manager
> > - * \param[in] camera The camera to be added
> > - * \param[in] devnums The device numbers to associate with \a camera
> > - *
> > - * This function is called by pipeline handlers to register the cameras they
> > - * handle with the camera manager. Registered cameras are immediately made
> > - * available to the system.
> > - *
> > - * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> > - * to Camera instances.
> > - *
> > - * \context This function shall be called from the CameraManager thread.
> > - */
> > -void CameraManager::addCamera(std::shared_ptr<Camera> camera,
> > -                           const std::vector<dev_t> &devnums)
> > -{
> > -     Private *const d = _d();
> > -
> > -     ASSERT(Thread::current() == d);
> > -
> > -     d->addCamera(camera, devnums);
> > -     cameraAdded.emit(camera);
> > -}
> > -
> > -/**
> > - * \brief Remove a camera from the camera manager
> > - * \param[in] camera The camera to be removed
> > - *
> > - * This function is called by pipeline handlers to unregister cameras from the
> > - * camera manager. Unregistered cameras won't be reported anymore by the
> > - * cameras() and get() calls, but references may still exist in applications.
> > - *
> > - * \context This function shall be called from the CameraManager thread.
> > - */
> > -void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
> > -{
> > -     Private *const d = _d();
> > -
> > -     ASSERT(Thread::current() == d);
> > -
> > -     d->removeCamera(camera.get());
> > -     cameraRemoved.emit(camera);
> > -}
> > -
> >  /**
> >   * \fn const std::string &CameraManager::version()
> >   * \brief Retrieve the libcamera version string
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index f72613b8e515..49092ea88a58 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -16,10 +16,10 @@
> >  #include <libcamera/base/utils.h>
> >
> >  #include <libcamera/camera.h>
> > -#include <libcamera/camera_manager.h>
> >  #include <libcamera/framebuffer.h>
> >
> >  #include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/camera_manager.h"
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/framebuffer.h"
> >  #include "libcamera/internal/media_device.h"
> > @@ -624,7 +624,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >               }
> >       }
> >
> > -     manager_->addCamera(std::move(camera), devnums);
> > +     manager_->_d()->addCamera(std::move(camera), devnums);
> >  }
> >
> >  /**
> > @@ -691,7 +691,7 @@ void PipelineHandler::disconnect()
> >                       continue;
> >
> >               camera->disconnect();
> > -             manager_->removeCamera(camera);
> > +             manager_->_d()->removeCamera(camera);
> 
> All minors as well
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
> 
> >       }
> >  }
> >
> > --
> > 2.34.1
> >

Patch
diff mbox series

diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index 4b1fb7568e83..9767acc42c89 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -34,10 +34,6 @@  public:
 	std::shared_ptr<Camera> get(const std::string &id);
 	std::shared_ptr<Camera> get(dev_t devnum);
 
-	void addCamera(std::shared_ptr<Camera> camera,
-		       const std::vector<dev_t> &devnums);
-	void removeCamera(std::shared_ptr<Camera> camera);
-
 	static const std::string &version() { return version_; }
 
 	Signal<std::shared_ptr<Camera>> cameraAdded;
diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
index 4bba3ac2eb29..8386e90e0f60 100644
--- a/include/libcamera/internal/camera_manager.h
+++ b/include/libcamera/internal/camera_manager.h
@@ -31,7 +31,7 @@  public:
 	int start();
 	void addCamera(std::shared_ptr<Camera> camera,
 		       const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
-	void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
+	void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
 
 	/*
 	 * This mutex protects
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index b95284ba5a80..34f09a4d181b 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -148,9 +148,25 @@  void CameraManager::Private::cleanup()
 	enumerator_.reset(nullptr);
 }
 
+/**
+ * \brief Add a camera to the camera manager
+ * \param[in] camera The camera to be added
+ * \param[in] devnums The device numbers to associate with \a camera
+ *
+ * This function is called by pipeline handlers to register the cameras they
+ * handle with the camera manager. Registered cameras are immediately made
+ * available to the system.
+ *
+ * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
+ * to Camera instances.
+ *
+ * \context This function shall be called from the CameraManager thread.
+ */
 void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
 				       const std::vector<dev_t> &devnums)
 {
+	ASSERT(Thread::current() == this);
+
 	MutexLocker locker(mutex_);
 
 	for (const std::shared_ptr<Camera> &c : cameras_) {
@@ -167,15 +183,31 @@  void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
 	unsigned int index = cameras_.size() - 1;
 	for (dev_t devnum : devnums)
 		camerasByDevnum_[devnum] = cameras_[index];
+
+	/* Report the addition to the public signal */
+	CameraManager *const o = LIBCAMERA_O_PTR();
+	o->cameraAdded.emit(cameras_[index]);
 }
 
-void CameraManager::Private::removeCamera(Camera *camera)
+/**
+ * \brief Remove a camera from the camera manager
+ * \param[in] camera The camera to be removed
+ *
+ * This function is called by pipeline handlers to unregister cameras from the
+ * camera manager. Unregistered cameras won't be reported anymore by the
+ * cameras() and get() calls, but references may still exist in applications.
+ *
+ * \context This function shall be called from the CameraManager thread.
+ */
+void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
 {
+	ASSERT(Thread::current() == this);
+
 	MutexLocker locker(mutex_);
 
 	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
 				 [camera](std::shared_ptr<Camera> &c) {
-					 return c.get() == camera;
+					 return c.get() == camera.get();
 				 });
 	if (iter == cameras_.end())
 		return;
@@ -185,12 +217,16 @@  void CameraManager::Private::removeCamera(Camera *camera)
 
 	auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
 				   [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
-					   return p.second.lock().get() == camera;
+					   return p.second.lock().get() == camera.get();
 				   });
 	if (iter_d != camerasByDevnum_.end())
 		camerasByDevnum_.erase(iter_d);
 
 	cameras_.erase(iter);
+
+	/* Report the removal to the public signal */
+	CameraManager *const o = LIBCAMERA_O_PTR();
+	o->cameraRemoved.emit(camera);
 }
 
 /**
@@ -383,51 +419,6 @@  std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
  * perform any blocking operation.
  */
 
-/**
- * \brief Add a camera to the camera manager
- * \param[in] camera The camera to be added
- * \param[in] devnums The device numbers to associate with \a camera
- *
- * This function is called by pipeline handlers to register the cameras they
- * handle with the camera manager. Registered cameras are immediately made
- * available to the system.
- *
- * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
- * to Camera instances.
- *
- * \context This function shall be called from the CameraManager thread.
- */
-void CameraManager::addCamera(std::shared_ptr<Camera> camera,
-			      const std::vector<dev_t> &devnums)
-{
-	Private *const d = _d();
-
-	ASSERT(Thread::current() == d);
-
-	d->addCamera(camera, devnums);
-	cameraAdded.emit(camera);
-}
-
-/**
- * \brief Remove a camera from the camera manager
- * \param[in] camera The camera to be removed
- *
- * This function is called by pipeline handlers to unregister cameras from the
- * camera manager. Unregistered cameras won't be reported anymore by the
- * cameras() and get() calls, but references may still exist in applications.
- *
- * \context This function shall be called from the CameraManager thread.
- */
-void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
-{
-	Private *const d = _d();
-
-	ASSERT(Thread::current() == d);
-
-	d->removeCamera(camera.get());
-	cameraRemoved.emit(camera);
-}
-
 /**
  * \fn const std::string &CameraManager::version()
  * \brief Retrieve the libcamera version string
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index f72613b8e515..49092ea88a58 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -16,10 +16,10 @@ 
 #include <libcamera/base/utils.h>
 
 #include <libcamera/camera.h>
-#include <libcamera/camera_manager.h>
 #include <libcamera/framebuffer.h>
 
 #include "libcamera/internal/camera.h"
+#include "libcamera/internal/camera_manager.h"
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/media_device.h"
@@ -624,7 +624,7 @@  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
 		}
 	}
 
-	manager_->addCamera(std::move(camera), devnums);
+	manager_->_d()->addCamera(std::move(camera), devnums);
 }
 
 /**
@@ -691,7 +691,7 @@  void PipelineHandler::disconnect()
 			continue;
 
 		camera->disconnect();
-		manager_->removeCamera(camera);
+		manager_->_d()->removeCamera(camera);
 	}
 }