[libcamera-devel,13/19] libcamera: camera_manager: Run the camera manager in a thread

2700 diff mbox series

Relying on the application event loop to process all our internal events
is a bad idea for multiple reasons. In many cases the user of libcamera
can't provide an event loop, for instance when running through one of
the adaptation layers. The Android camera HAL and V4L2 compatibility
layer create a thread for this reason, and the GStreamer element would
need to do so as well. Furthermore, relying on the application event
loop pushes libcamera's realtime constraints to the application, which
isn't manageable.

For these reasons it's desirable to always run the camera manager, the
pipeline handlers and the cameras in a separate thread. Doing so isn't
too complicated, it only involves creating the thread internally when
starting the camera manager, and synchronizing a few methods of the
Camera class. Do so as a first step towards defining the threading model
of libcamera.

Making CameraManager::cameras() thread-safe requires returning a copy of
the cameras vector instead of a reference. This is also required for
hot-plugging support and is thus desirable.

The event dispatcher interface is still exposed to applications, to
enable cross-thread signal delivery if desired.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/Doxyfile.in          |   1 +
 include/libcamera/camera_manager.h |  13 +-
 src/libcamera/camera_manager.cpp   | 295 +++++++++++++++++++++--------
 3 files changed, 224 insertions(+), 85 deletions(-)

Hi Laurent,

Thanks for your work.

On 2020-01-20 02:24:31 +0200, Laurent Pinchart wrote:
> Relying on the application event loop to process all our internal events
> is a bad idea for multiple reasons. In many cases the user of libcamera
> can't provide an event loop, for instance when running through one of
> the adaptation layers. The Android camera HAL and V4L2 compatibility
> layer create a thread for this reason, and the GStreamer element would
> need to do so as well. Furthermore, relying on the application event
> loop pushes libcamera's realtime constraints to the application, which
> isn't manageable.
> 
> For these reasons it's desirable to always run the camera manager, the
> pipeline handlers and the cameras in a separate thread. Doing so isn't
> too complicated, it only involves creating the thread internally when
> starting the camera manager, and synchronizing a few methods of the
> Camera class. Do so as a first step towards defining the threading model
> of libcamera.
> 
> Making CameraManager::cameras() thread-safe requires returning a copy of
> the cameras vector instead of a reference. This is also required for
> hot-plugging support and is thus desirable.
> 
> The event dispatcher interface is still exposed to applications, to
> enable cross-thread signal delivery if desired.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch is quiet hard to review as it do multiple things, would it be 
possible to split it a bit? I think it can be split into

- Change the behavior of CameraManager::cameras()
- Breakout of CameraManager::Private
- New thread related code.

Small comments bellow tho.

> ---
>  Documentation/Doxyfile.in          |   1 +
>  include/libcamera/camera_manager.h |  13 +-
>  src/libcamera/camera_manager.cpp   | 295 +++++++++++++++++++++--------
>  3 files changed, 224 insertions(+), 85 deletions(-)
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 734672ed15dc..1c46b04b3f7e 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -879,6 +879,7 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
>                           libcamera::BoundMethodPackBase \
>                           libcamera::BoundMethodStatic \
>                           libcamera::SignalBase \
> +                         libcamera::*::Private \
>                           std::*
>  
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 094197668698..079f848aec79 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,7 +7,6 @@
>  #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
>  #define __LIBCAMERA_CAMERA_MANAGER_H__
>  
> -#include <map>
>  #include <memory>
>  #include <string>
>  #include <sys/types.h>
> @@ -18,9 +17,7 @@
>  namespace libcamera {
>  
>  class Camera;
> -class DeviceEnumerator;
>  class EventDispatcher;
> -class PipelineHandler;
>  
>  class CameraManager : public Object
>  {
> @@ -33,7 +30,7 @@ public:
>  	int start();
>  	void stop();
>  
> -	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> +	std::vector<std::shared_ptr<Camera>> cameras() const;
>  	std::shared_ptr<Camera> get(const std::string &name);
>  	std::shared_ptr<Camera> get(dev_t devnum);
>  
> @@ -46,13 +43,11 @@ public:
>  	EventDispatcher *eventDispatcher();
>  
>  private:
> -	std::unique_ptr<DeviceEnumerator> enumerator_;
> -	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> -	std::vector<std::shared_ptr<Camera>> cameras_;
> -	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> -
>  	static const std::string version_;
>  	static CameraManager *self_;
> +
> +	class Private;
> +	std::unique_ptr<Private> p_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index a71caf8536bb..64aa89975701 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -7,6 +7,9 @@
>  
>  #include <libcamera/camera_manager.h>
>  
> +#include <condition_variable>
> +#include <map>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/event_dispatcher.h>
>  
> @@ -26,6 +29,184 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(Camera)
>  
> +class CameraManager::Private : public Thread
> +{
> +public:
> +	Private(CameraManager *cm);
> +
> +	int start();
> +	void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> +	void removeCamera(Camera *camera);
> +
> +	/*
> +	 * This mutex protects
> +	 *
> +	 * - initialized_ and status_ during initialization
> +	 * - cameras_ and camerasByDevnum_ after initialization
> +	 */
> +	Mutex mutex_;
> +	std::vector<std::shared_ptr<Camera>> cameras_;
> +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> +
> +protected:
> +	void run() override;
> +
> +private:
> +	int init();
> +	void cleanup();
> +
> +	CameraManager *cm_;
> +
> +	std::condition_variable cv_;
> +	bool initialized_;
> +	int status_;
> +
> +	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> +	std::unique_ptr<DeviceEnumerator> enumerator_;
> +};
> +
> +CameraManager::Private::Private(CameraManager *cm)
> +	: cm_(cm), initialized_(false)
> +{
> +}
> +
> +int CameraManager::Private::start()
> +{
> +	/* Start the thread and wait for initialization to complete. */
> +	Thread::start();
> +
> +	{
> +		MutexLocker locker(mutex_);
> +		cv_.wait(locker, [&] { return initialized_; });
> +	}

For my education, is this in its a own block so that locker will go out 
of scope and free the locker after the wait returns?

> +
> +	/* If a failure happened during initialization, stop the thread. */
> +	if (status_ < 0) {
> +		exit();
> +		wait();
> +		return status_;
> +	}
> +
> +	return 0;
> +}
> +
> +void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> +				       dev_t devnum)
> +{
> +	MutexLocker locker(mutex_);
> +
> +	for (std::shared_ptr<Camera> c : cameras_) {
> +		if (c->name() == camera->name()) {
> +			LOG(Camera, Warning)
> +				<< "Registering camera with duplicate name '"
> +				<< camera->name() << "'";
> +			break;
> +		}
> +	}
> +
> +	cameras_.push_back(std::move(camera));
> +
> +	if (devnum) {
> +		unsigned int index = cameras_.size() - 1;
> +		camerasByDevnum_[devnum] = cameras_[index];
> +	}
> +}
> +
> +void CameraManager::Private::removeCamera(Camera *camera)
> +{
> +	MutexLocker locker(mutex_);
> +
> +	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> +				 [camera](std::shared_ptr<Camera> &c) {
> +					 return c.get() == camera;
> +				 });
> +	if (iter == cameras_.end())
> +		return;
> +
> +	LOG(Camera, Debug)
> +		<< "Unregistering camera '" << camera->name() << "'";
> +
> +	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;
> +				   });
> +	if (iter_d != camerasByDevnum_.end())
> +		camerasByDevnum_.erase(iter_d);
> +
> +	cameras_.erase(iter);
> +}
> +
> +void CameraManager::Private::run()
> +{
> +	LOG(Camera, Debug) << "Starting camera manager";
> +
> +	int ret = init();
> +
> +	mutex_.lock();
> +	status_ = ret;
> +	initialized_ = true;
> +	mutex_.unlock();
> +	cv_.notify_one();
> +
> +	if (ret < 0)
> +		return;
> +
> +	/* Now start processing events and messages. */
> +	exec();
> +
> +	cleanup();
> +}
> +
> +int CameraManager::Private::init()
> +{
> +	enumerator_ = DeviceEnumerator::create();
> +	if (!enumerator_ || enumerator_->enumerate())
> +		return -ENODEV;
> +
> +	/*
> +	 * TODO: Try to read handlers and order from configuration
> +	 * file and only fallback on all handlers if there is no
> +	 * configuration file.
> +	 */
> +	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> +
> +	for (PipelineHandlerFactory *factory : factories) {
> +		/*
> +		 * Try each pipeline handler until it exhaust
> +		 * all pipelines it can provide.
> +		 */
> +		while (1) {
> +			std::shared_ptr<PipelineHandler> pipe = factory->create(cm_);
> +			if (!pipe->match(enumerator_.get()))
> +				break;
> +
> +			LOG(Camera, Debug)
> +				<< "Pipeline handler \"" << factory->name()
> +				<< "\" matched";
> +			pipes_.push_back(std::move(pipe));
> +		}
> +	}
> +
> +	/* TODO: register hot-plug callback here */
> +
> +	return 0;
> +}
> +
> +void CameraManager::Private::cleanup()
> +{
> +	/* TODO: unregister hot-plug callback here */
> +
> +	/*
> +	 * Release all references to cameras and pipeline handlers to ensure
> +	 * they all get destroyed before the device enumerator deletes the
> +	 * media devices.
> +	 */
> +	pipes_.clear();
> +	cameras_.clear();
> +
> +	enumerator_.reset(nullptr);
> +}
> +
>  /**
>   * \class CameraManager
>   * \brief Provide access and manage all cameras in the system
> @@ -62,7 +243,7 @@ LOG_DEFINE_CATEGORY(Camera)
>  CameraManager *CameraManager::self_ = nullptr;
>  
>  CameraManager::CameraManager()
> -	: enumerator_(nullptr)
> +	: p_(new CameraManager::Private(this))
>  {
>  	if (self_)
>  		LOG(Camera, Fatal)
> @@ -73,6 +254,8 @@ CameraManager::CameraManager()
>  
>  CameraManager::~CameraManager()
>  {
> +	stop();
> +
>  	self_ = nullptr;
>  }
>  
> @@ -88,41 +271,16 @@ CameraManager::~CameraManager()
>   */
>  int CameraManager::start()
>  {
> -	if (enumerator_)
> -		return -EBUSY;
> -
>  	LOG(Camera, Info) << "libcamera " << version_;
>  
> -	enumerator_ = DeviceEnumerator::create();
> -	if (!enumerator_ || enumerator_->enumerate())
> -		return -ENODEV;
> -
> -	/*
> -	 * TODO: Try to read handlers and order from configuration
> -	 * file and only fallback on all handlers if there is no
> -	 * configuration file.
> -	 */
> -	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> +	int ret = p_->start();
> +	if (ret < 0) {
> +		LOG(Camera, Error) << "Failed to start camera manager: "
> +				   << strerror(-ret);
>  
> -	for (PipelineHandlerFactory *factory : factories) {
> -		/*
> -		 * Try each pipeline handler until it exhaust
> -		 * all pipelines it can provide.
> -		 */
> -		while (1) {
> -			std::shared_ptr<PipelineHandler> pipe = factory->create(this);
> -			if (!pipe->match(enumerator_.get()))
> -				break;
> -
> -			LOG(Camera, Debug)
> -				<< "Pipeline handler \"" << factory->name()
> -				<< "\" matched";
> -			pipes_.push_back(std::move(pipe));
> -		}
> +		return ret;
>  	}
>  
> -	/* TODO: register hot-plug callback here */
> -
>  	return 0;

I think you can simplify this to

        int ret = p_->start();
        if (ret)
		LOG(Camera, Error) << "Failed to start camera manager: "
				   << strerror(-ret);

        return ret;


as start() shall return 0 on success.

>  }
>  
> @@ -138,17 +296,8 @@ int CameraManager::start()
>   */
>  void CameraManager::stop()
>  {
> -	/* TODO: unregister hot-plug callback here */
> -
> -	/*
> -	 * Release all references to cameras and pipeline handlers to ensure
> -	 * they all get destroyed before the device enumerator deletes the
> -	 * media devices.
> -	 */
> -	pipes_.clear();
> -	cameras_.clear();
> -
> -	enumerator_.reset(nullptr);
> +	p_->exit();
> +	p_->wait();
>  }
>  
>  /**
> @@ -158,8 +307,16 @@ void CameraManager::stop()
>   * Before calling this function the caller is responsible for ensuring that
>   * the camera manager is running.
>   *
> + * \context This function is \threadsafe.
> + *
>   * \return List of all available cameras
>   */
> +std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
> +{
> +	MutexLocker locker(p_->mutex_);
> +
> +	return p_->cameras_;
> +}
>  
>  /**
>   * \brief Get a camera based on name
> @@ -168,11 +325,15 @@ void CameraManager::stop()
>   * Before calling this function the caller is responsible for ensuring that
>   * the camera manager is running.
>   *
> + * \context This function is \threadsafe.
> + *
>   * \return Shared pointer to Camera object or nullptr if camera not found
>   */
>  std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  {
> -	for (std::shared_ptr<Camera> camera : cameras_) {
> +	MutexLocker locker(p_->mutex_);
> +
> +	for (std::shared_ptr<Camera> camera : p_->cameras_) {
>  		if (camera->name() == name)
>  			return camera;
>  	}
> @@ -191,13 +352,17 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>   * Before calling this function the caller is responsible for ensuring that
>   * the camera manager is running.
>   *
> + * \context This function is \threadsafe.
> + *
>   * \return Shared pointer to Camera object, which is empty if the camera is
>   * not found
>   */
>  std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>  {
> -	auto iter = camerasByDevnum_.find(devnum);
> -	if (iter == camerasByDevnum_.end())
> +	MutexLocker locker(p_->mutex_);
> +
> +	auto iter = p_->camerasByDevnum_.find(devnum);
> +	if (iter == p_->camerasByDevnum_.end())
>  		return nullptr;
>  
>  	return iter->second.lock();
> @@ -214,24 +379,14 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>   *
>   * \a devnum is 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, dev_t devnum)
>  {
> -	for (std::shared_ptr<Camera> c : cameras_) {
> -		if (c->name() == camera->name()) {
> -			LOG(Camera, Warning)
> -				<< "Registering camera with duplicate name '"
> -				<< camera->name() << "'";
> -			break;
> -		}
> -	}
> -
> -	cameras_.push_back(std::move(camera));
> +	ASSERT(Thread::current() == p_.get());
>  
> -	if (devnum) {
> -		unsigned int index = cameras_.size() - 1;
> -		camerasByDevnum_[devnum] = cameras_[index];
> -	}
> +	p_->addCamera(camera, devnum);
>  }
>  
>  /**
> @@ -241,32 +396,20 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
>   * 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(Camera *camera)
>  {
> -	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> -				 [camera](std::shared_ptr<Camera> &c) {
> -					 return c.get() == camera;
> -				 });
> -	if (iter == cameras_.end())
> -		return;
> -
> -	LOG(Camera, Debug)
> -		<< "Unregistering camera '" << camera->name() << "'";
> -
> -	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;
> -				   });
> -	if (iter_d != camerasByDevnum_.end())
> -		camerasByDevnum_.erase(iter_d);
> +	ASSERT(Thread::current() == p_.get());
>  
> -	cameras_.erase(iter);
> +	p_->removeCamera(camera);
>  }
>  
>  /**
>   * \fn const std::string &CameraManager::version()
>   * \brief Retrieve the libcamera version string
> + * \context This function is \a threadsafe.
>   * \return The libcamera version string
>   */
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Wed, Jan 22, 2020 at 05:13:19PM +0100, Niklas Söderlund wrote:
> On 2020-01-20 02:24:31 +0200, Laurent Pinchart wrote:
> > Relying on the application event loop to process all our internal events
> > is a bad idea for multiple reasons. In many cases the user of libcamera
> > can't provide an event loop, for instance when running through one of
> > the adaptation layers. The Android camera HAL and V4L2 compatibility
> > layer create a thread for this reason, and the GStreamer element would
> > need to do so as well. Furthermore, relying on the application event
> > loop pushes libcamera's realtime constraints to the application, which
> > isn't manageable.
> > 
> > For these reasons it's desirable to always run the camera manager, the
> > pipeline handlers and the cameras in a separate thread. Doing so isn't
> > too complicated, it only involves creating the thread internally when
> > starting the camera manager, and synchronizing a few methods of the
> > Camera class. Do so as a first step towards defining the threading model
> > of libcamera.
> > 
> > Making CameraManager::cameras() thread-safe requires returning a copy of
> > the cameras vector instead of a reference. This is also required for
> > hot-plugging support and is thus desirable.
> > 
> > The event dispatcher interface is still exposed to applications, to
> > enable cross-thread signal delivery if desired.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This patch is quiet hard to review as it do multiple things, would it be 
> possible to split it a bit? I think it can be split into
> 
> - Change the behavior of CameraManager::cameras()
> - Breakout of CameraManager::Private
> - New thread related code.

I'll do so.

> Small comments bellow tho.
> 
> > ---
> >  Documentation/Doxyfile.in          |   1 +
> >  include/libcamera/camera_manager.h |  13 +-
> >  src/libcamera/camera_manager.cpp   | 295 +++++++++++++++++++++--------
> >  3 files changed, 224 insertions(+), 85 deletions(-)
> > 
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index 734672ed15dc..1c46b04b3f7e 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -879,6 +879,7 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
> >                           libcamera::BoundMethodPackBase \
> >                           libcamera::BoundMethodStatic \
> >                           libcamera::SignalBase \
> > +                         libcamera::*::Private \
> >                           std::*
> >  
> >  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 094197668698..079f848aec79 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -7,7 +7,6 @@
> >  #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> >  #define __LIBCAMERA_CAMERA_MANAGER_H__
> >  
> > -#include <map>
> >  #include <memory>
> >  #include <string>
> >  #include <sys/types.h>
> > @@ -18,9 +17,7 @@
> >  namespace libcamera {
> >  
> >  class Camera;
> > -class DeviceEnumerator;
> >  class EventDispatcher;
> > -class PipelineHandler;
> >  
> >  class CameraManager : public Object
> >  {
> > @@ -33,7 +30,7 @@ public:
> >  	int start();
> >  	void stop();
> >  
> > -	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > +	std::vector<std::shared_ptr<Camera>> cameras() const;
> >  	std::shared_ptr<Camera> get(const std::string &name);
> >  	std::shared_ptr<Camera> get(dev_t devnum);
> >  
> > @@ -46,13 +43,11 @@ public:
> >  	EventDispatcher *eventDispatcher();
> >  
> >  private:
> > -	std::unique_ptr<DeviceEnumerator> enumerator_;
> > -	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> > -	std::vector<std::shared_ptr<Camera>> cameras_;
> > -	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> > -
> >  	static const std::string version_;
> >  	static CameraManager *self_;
> > +
> > +	class Private;
> > +	std::unique_ptr<Private> p_;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index a71caf8536bb..64aa89975701 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -7,6 +7,9 @@
> >  
> >  #include <libcamera/camera_manager.h>
> >  
> > +#include <condition_variable>
> > +#include <map>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/event_dispatcher.h>
> >  
> > @@ -26,6 +29,184 @@ namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(Camera)
> >  
> > +class CameraManager::Private : public Thread
> > +{
> > +public:
> > +	Private(CameraManager *cm);
> > +
> > +	int start();
> > +	void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> > +	void removeCamera(Camera *camera);
> > +
> > +	/*
> > +	 * This mutex protects
> > +	 *
> > +	 * - initialized_ and status_ during initialization
> > +	 * - cameras_ and camerasByDevnum_ after initialization
> > +	 */
> > +	Mutex mutex_;
> > +	std::vector<std::shared_ptr<Camera>> cameras_;
> > +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> > +
> > +protected:
> > +	void run() override;
> > +
> > +private:
> > +	int init();
> > +	void cleanup();
> > +
> > +	CameraManager *cm_;
> > +
> > +	std::condition_variable cv_;
> > +	bool initialized_;
> > +	int status_;
> > +
> > +	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> > +	std::unique_ptr<DeviceEnumerator> enumerator_;
> > +};
> > +
> > +CameraManager::Private::Private(CameraManager *cm)
> > +	: cm_(cm), initialized_(false)
> > +{
> > +}
> > +
> > +int CameraManager::Private::start()
> > +{
> > +	/* Start the thread and wait for initialization to complete. */
> > +	Thread::start();
> > +
> > +	{
> > +		MutexLocker locker(mutex_);
> > +		cv_.wait(locker, [&] { return initialized_; });
> > +	}
> 
> For my education, is this in its a own block so that locker will go out 
> of scope and free the locker after the wait returns?

Correct.

> > +
> > +	/* If a failure happened during initialization, stop the thread. */
> > +	if (status_ < 0) {
> > +		exit();
> > +		wait();
> > +		return status_;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> > +				       dev_t devnum)
> > +{
> > +	MutexLocker locker(mutex_);
> > +
> > +	for (std::shared_ptr<Camera> c : cameras_) {
> > +		if (c->name() == camera->name()) {
> > +			LOG(Camera, Warning)
> > +				<< "Registering camera with duplicate name '"
> > +				<< camera->name() << "'";
> > +			break;
> > +		}
> > +	}
> > +
> > +	cameras_.push_back(std::move(camera));
> > +
> > +	if (devnum) {
> > +		unsigned int index = cameras_.size() - 1;
> > +		camerasByDevnum_[devnum] = cameras_[index];
> > +	}
> > +}
> > +
> > +void CameraManager::Private::removeCamera(Camera *camera)
> > +{
> > +	MutexLocker locker(mutex_);
> > +
> > +	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> > +				 [camera](std::shared_ptr<Camera> &c) {
> > +					 return c.get() == camera;
> > +				 });
> > +	if (iter == cameras_.end())
> > +		return;
> > +
> > +	LOG(Camera, Debug)
> > +		<< "Unregistering camera '" << camera->name() << "'";
> > +
> > +	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;
> > +				   });
> > +	if (iter_d != camerasByDevnum_.end())
> > +		camerasByDevnum_.erase(iter_d);
> > +
> > +	cameras_.erase(iter);
> > +}
> > +
> > +void CameraManager::Private::run()
> > +{
> > +	LOG(Camera, Debug) << "Starting camera manager";
> > +
> > +	int ret = init();
> > +
> > +	mutex_.lock();
> > +	status_ = ret;
> > +	initialized_ = true;
> > +	mutex_.unlock();
> > +	cv_.notify_one();
> > +
> > +	if (ret < 0)
> > +		return;
> > +
> > +	/* Now start processing events and messages. */
> > +	exec();
> > +
> > +	cleanup();
> > +}
> > +
> > +int CameraManager::Private::init()
> > +{
> > +	enumerator_ = DeviceEnumerator::create();
> > +	if (!enumerator_ || enumerator_->enumerate())
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * TODO: Try to read handlers and order from configuration
> > +	 * file and only fallback on all handlers if there is no
> > +	 * configuration file.
> > +	 */
> > +	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> > +
> > +	for (PipelineHandlerFactory *factory : factories) {
> > +		/*
> > +		 * Try each pipeline handler until it exhaust
> > +		 * all pipelines it can provide.
> > +		 */
> > +		while (1) {
> > +			std::shared_ptr<PipelineHandler> pipe = factory->create(cm_);
> > +			if (!pipe->match(enumerator_.get()))
> > +				break;
> > +
> > +			LOG(Camera, Debug)
> > +				<< "Pipeline handler \"" << factory->name()
> > +				<< "\" matched";
> > +			pipes_.push_back(std::move(pipe));
> > +		}
> > +	}
> > +
> > +	/* TODO: register hot-plug callback here */
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraManager::Private::cleanup()
> > +{
> > +	/* TODO: unregister hot-plug callback here */
> > +
> > +	/*
> > +	 * Release all references to cameras and pipeline handlers to ensure
> > +	 * they all get destroyed before the device enumerator deletes the
> > +	 * media devices.
> > +	 */
> > +	pipes_.clear();
> > +	cameras_.clear();
> > +
> > +	enumerator_.reset(nullptr);
> > +}
> > +
> >  /**
> >   * \class CameraManager
> >   * \brief Provide access and manage all cameras in the system
> > @@ -62,7 +243,7 @@ LOG_DEFINE_CATEGORY(Camera)
> >  CameraManager *CameraManager::self_ = nullptr;
> >  
> >  CameraManager::CameraManager()
> > -	: enumerator_(nullptr)
> > +	: p_(new CameraManager::Private(this))
> >  {
> >  	if (self_)
> >  		LOG(Camera, Fatal)
> > @@ -73,6 +254,8 @@ CameraManager::CameraManager()
> >  
> >  CameraManager::~CameraManager()
> >  {
> > +	stop();
> > +
> >  	self_ = nullptr;
> >  }
> >  
> > @@ -88,41 +271,16 @@ CameraManager::~CameraManager()
> >   */
> >  int CameraManager::start()
> >  {
> > -	if (enumerator_)
> > -		return -EBUSY;
> > -
> >  	LOG(Camera, Info) << "libcamera " << version_;
> >  
> > -	enumerator_ = DeviceEnumerator::create();
> > -	if (!enumerator_ || enumerator_->enumerate())
> > -		return -ENODEV;
> > -
> > -	/*
> > -	 * TODO: Try to read handlers and order from configuration
> > -	 * file and only fallback on all handlers if there is no
> > -	 * configuration file.
> > -	 */
> > -	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> > +	int ret = p_->start();
> > +	if (ret < 0) {
> > +		LOG(Camera, Error) << "Failed to start camera manager: "
> > +				   << strerror(-ret);
> >  
> > -	for (PipelineHandlerFactory *factory : factories) {
> > -		/*
> > -		 * Try each pipeline handler until it exhaust
> > -		 * all pipelines it can provide.
> > -		 */
> > -		while (1) {
> > -			std::shared_ptr<PipelineHandler> pipe = factory->create(this);
> > -			if (!pipe->match(enumerator_.get()))
> > -				break;
> > -
> > -			LOG(Camera, Debug)
> > -				<< "Pipeline handler \"" << factory->name()
> > -				<< "\" matched";
> > -			pipes_.push_back(std::move(pipe));
> > -		}
> > +		return ret;
> >  	}
> >  
> > -	/* TODO: register hot-plug callback here */
> > -
> >  	return 0;
> 
> I think you can simplify this to
> 
>         int ret = p_->start();
>         if (ret)
> 		LOG(Camera, Error) << "Failed to start camera manager: "
> 				   << strerror(-ret);
> 
>         return ret;
> 
> 
> as start() shall return 0 on success.

Done.

> >  }
> >  
> > @@ -138,17 +296,8 @@ int CameraManager::start()
> >   */
> >  void CameraManager::stop()
> >  {
> > -	/* TODO: unregister hot-plug callback here */
> > -
> > -	/*
> > -	 * Release all references to cameras and pipeline handlers to ensure
> > -	 * they all get destroyed before the device enumerator deletes the
> > -	 * media devices.
> > -	 */
> > -	pipes_.clear();
> > -	cameras_.clear();
> > -
> > -	enumerator_.reset(nullptr);
> > +	p_->exit();
> > +	p_->wait();
> >  }
> >  
> >  /**
> > @@ -158,8 +307,16 @@ void CameraManager::stop()
> >   * Before calling this function the caller is responsible for ensuring that
> >   * the camera manager is running.
> >   *
> > + * \context This function is \threadsafe.
> > + *
> >   * \return List of all available cameras
> >   */
> > +std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
> > +{
> > +	MutexLocker locker(p_->mutex_);
> > +
> > +	return p_->cameras_;
> > +}
> >  
> >  /**
> >   * \brief Get a camera based on name
> > @@ -168,11 +325,15 @@ void CameraManager::stop()
> >   * Before calling this function the caller is responsible for ensuring that
> >   * the camera manager is running.
> >   *
> > + * \context This function is \threadsafe.
> > + *
> >   * \return Shared pointer to Camera object or nullptr if camera not found
> >   */
> >  std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> >  {
> > -	for (std::shared_ptr<Camera> camera : cameras_) {
> > +	MutexLocker locker(p_->mutex_);
> > +
> > +	for (std::shared_ptr<Camera> camera : p_->cameras_) {
> >  		if (camera->name() == name)
> >  			return camera;
> >  	}
> > @@ -191,13 +352,17 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> >   * Before calling this function the caller is responsible for ensuring that
> >   * the camera manager is running.
> >   *
> > + * \context This function is \threadsafe.
> > + *
> >   * \return Shared pointer to Camera object, which is empty if the camera is
> >   * not found
> >   */
> >  std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> >  {
> > -	auto iter = camerasByDevnum_.find(devnum);
> > -	if (iter == camerasByDevnum_.end())
> > +	MutexLocker locker(p_->mutex_);
> > +
> > +	auto iter = p_->camerasByDevnum_.find(devnum);
> > +	if (iter == p_->camerasByDevnum_.end())
> >  		return nullptr;
> >  
> >  	return iter->second.lock();
> > @@ -214,24 +379,14 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> >   *
> >   * \a devnum is 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, dev_t devnum)
> >  {
> > -	for (std::shared_ptr<Camera> c : cameras_) {
> > -		if (c->name() == camera->name()) {
> > -			LOG(Camera, Warning)
> > -				<< "Registering camera with duplicate name '"
> > -				<< camera->name() << "'";
> > -			break;
> > -		}
> > -	}
> > -
> > -	cameras_.push_back(std::move(camera));
> > +	ASSERT(Thread::current() == p_.get());
> >  
> > -	if (devnum) {
> > -		unsigned int index = cameras_.size() - 1;
> > -		camerasByDevnum_[devnum] = cameras_[index];
> > -	}
> > +	p_->addCamera(camera, devnum);
> >  }
> >  
> >  /**
> > @@ -241,32 +396,20 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> >   * 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(Camera *camera)
> >  {
> > -	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> > -				 [camera](std::shared_ptr<Camera> &c) {
> > -					 return c.get() == camera;
> > -				 });
> > -	if (iter == cameras_.end())
> > -		return;
> > -
> > -	LOG(Camera, Debug)
> > -		<< "Unregistering camera '" << camera->name() << "'";
> > -
> > -	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;
> > -				   });
> > -	if (iter_d != camerasByDevnum_.end())
> > -		camerasByDevnum_.erase(iter_d);
> > +	ASSERT(Thread::current() == p_.get());
> >  
> > -	cameras_.erase(iter);
> > +	p_->removeCamera(camera);
> >  }
> >  
> >  /**
> >   * \fn const std::string &CameraManager::version()
> >   * \brief Retrieve the libcamera version string
> > + * \context This function is \a threadsafe.
> >   * \return The libcamera version string
> >   */
> >