[libcamera-devel] camera_manager: Formalize shared ownership

18462 diff mbox series

diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index 4b1fb756..3d5bf1ea 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -16,6 +16,7 @@ 
 #include <libcamera/base/object.h>
 #include <libcamera/base/signal.h>
 
+
 namespace libcamera {
 
 class Camera;
@@ -47,7 +48,6 @@  private:
 	LIBCAMERA_DISABLE_COPY(CameraManager)
 
 	static const std::string version_;
-	static CameraManager *self_;
 };
 
 } /* namespace libcamera */
diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
new file mode 100644
index 00000000..ec1d625b
--- /dev/null
+++ b/include/libcamera/internal/camera_manager.h
@@ -0,0 +1,60 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2018, Google Inc.
+ *
+ * camera_manager.h - Camera management
+ */
+
+#pragma once
+
+
+#include <map>
+#include <memory>
+#include <vector>
+#include <libcamera/base/mutex.h>
+#include <libcamera/base/thread.h>
+#include <libcamera/camera.h>
+#include "libcamera/internal/device_enumerator.h"
+
+namespace libcamera {
+
+class CamerasList : public Thread
+{
+public:
+	CamerasList();
+	~CamerasList();
+
+	int start();
+	void stop();
+	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_);
+
+	/*
+	 * This mutex protects
+	 *
+	 * - initialized_ and status_ during initialization
+	 * - cameras_ and camerasByDevnum_ after initialization
+	 */
+	mutable Mutex mutex_;
+	std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
+	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
+	static std::weak_ptr<CamerasList> singleton_;
+
+	void createPipelineHandlers();
+	void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
+	void prepare(std::shared_ptr<CamerasList> self);
+
+protected:
+	void run() override;
+
+private:
+	int init();
+	ConditionVariable cv_;
+	bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
+	int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
+
+	std::unique_ptr<DeviceEnumerator> enumerator_;
+};
+
+}
\ No newline at end of file
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 4c4dfe62..fd07ceda 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -27,6 +27,7 @@  namespace libcamera {
 class Camera;
 class CameraConfiguration;
 class CameraManager;
+class CamerasList;
 class DeviceEnumerator;
 class DeviceMatch;
 class FrameBuffer;
@@ -38,7 +39,7 @@  class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,
 			public Object
 {
 public:
-	PipelineHandler(CameraManager *manager);
+	PipelineHandler(std::shared_ptr<CamerasList> manager);
 	virtual ~PipelineHandler();
 
 	virtual bool match(DeviceEnumerator *enumerator) = 0;
@@ -79,7 +80,7 @@  protected:
 
 	virtual void releaseDevice(Camera *camera);
 
-	CameraManager *manager_;
+	std::shared_ptr<CamerasList> manager_;
 
 private:
 	void unlockMediaDevices();
@@ -109,7 +110,7 @@  public:
 	PipelineHandlerFactoryBase(const char *name);
 	virtual ~PipelineHandlerFactoryBase() = default;
 
-	std::shared_ptr<PipelineHandler> create(CameraManager *manager) const;
+	std::shared_ptr<PipelineHandler> create(std::shared_ptr<CamerasList> manager) const;
 
 	const std::string &name() const { return name_; }
 
@@ -119,7 +120,7 @@  private:
 	static void registerType(PipelineHandlerFactoryBase *factory);
 
 	virtual std::unique_ptr<PipelineHandler>
-	createInstance(CameraManager *manager) const = 0;
+	createInstance(std::shared_ptr<CamerasList> manager) const = 0;
 
 	std::string name_;
 };
@@ -134,7 +135,7 @@  public:
 	}
 
 	std::unique_ptr<PipelineHandler>
-	createInstance(CameraManager *manager) const override
+	createInstance(std::shared_ptr<CamerasList> manager) const override
 	{
 		return std::make_unique<_PipelineHandler>(manager);
 	}
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index c1edefda..85eca629 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -16,6 +16,7 @@ 
 #include <libcamera/base/thread.h>
 #include <libcamera/base/utils.h>
 
+#include "libcamera/internal/camera_manager.h"
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/pipeline_handler.h"
@@ -33,52 +34,46 @@  namespace libcamera {
 
 LOG_DEFINE_CATEGORY(Camera)
 
-class CameraManager::Private : public Extensible::Private, public Thread
+
+class CameraManager::Private : public Extensible::Private
 {
 	LIBCAMERA_DECLARE_PUBLIC(CameraManager)
 
 public:
-	Private();
-
 	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_);
-
-	/*
-	 * This mutex protects
-	 *
-	 * - initialized_ and status_ during initialization
-	 * - cameras_ and camerasByDevnum_ after initialization
-	 */
-	mutable Mutex mutex_;
-	std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
-	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
-
-protected:
-	void run() override;
-
+	std::shared_ptr<CamerasList> cl_;
 private:
-	int init();
-	void createPipelineHandlers();
-	void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
-
-	ConditionVariable cv_;
-	bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
-	int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
-
-	std::unique_ptr<DeviceEnumerator> enumerator_;
-
 	IPAManager ipaManager_;
 	ProcessManager processManager_;
 };
 
-CameraManager::Private::Private()
+std::weak_ptr<CamerasList> CamerasList::singleton_;
+
+int CameraManager::Private::start()
+{
+	if (CamerasList::singleton_.use_count() > 0) {
+		cl_ = CamerasList::singleton_.lock();
+		if (cl_->isRunning()) {
+			return 0;
+		}
+	} else {
+		cl_ = std::make_shared<CamerasList>();
+		cl_->prepare(cl_);
+	}
+	return cl_->start();
+}
+
+CamerasList::CamerasList()
 	: initialized_(false)
 {
 }
 
-int CameraManager::Private::start()
+CamerasList::~CamerasList()
+{
+	stop();
+}
+
+int CamerasList::start()
 {
 	int status;
 
@@ -103,8 +98,9 @@  int CameraManager::Private::start()
 	return 0;
 }
 
-void CameraManager::Private::run()
+void CamerasList::run()
 {
+	ASSERT(singleton_.use_count() > 0);
 	LOG(Camera, Debug) << "Starting camera manager";
 
 	int ret = init();
@@ -124,21 +120,28 @@  void CameraManager::Private::run()
 	cleanup();
 }
 
-int CameraManager::Private::init()
+void CamerasList::prepare(std::shared_ptr<CamerasList> self)
+{
+	singleton_ = self;
+}
+
+int CamerasList::init()
 {
 	enumerator_ = DeviceEnumerator::create();
 	if (!enumerator_ || enumerator_->enumerate())
 		return -ENODEV;
 
 	createPipelineHandlers();
-	enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers);
+	// It's safe to add signals directly through a pointer
+	// because enumerator (and its signals) can not outlive CamerasList.
+	enumerator_->devicesAdded.connect(this, &CamerasList::createPipelineHandlers);
 
 	return 0;
 }
 
-void CameraManager::Private::createPipelineHandlers()
+void CamerasList::createPipelineHandlers()
 {
-	CameraManager *const o = LIBCAMERA_O_PTR();
+	std::shared_ptr<CamerasList> const o = singleton_.lock();
 
 	/*
 	 * \todo Try to read handlers and order from configuration
@@ -168,10 +171,9 @@  void CameraManager::Private::createPipelineHandlers()
 	}
 }
 
-void CameraManager::Private::cleanup()
+void CamerasList::cleanup()
 {
 	enumerator_->devicesAdded.disconnect(this);
-
 	/*
 	 * Release all references to cameras to ensure they all get destroyed
 	 * before the device enumerator deletes the media devices. Cameras are
@@ -189,7 +191,7 @@  void CameraManager::Private::cleanup()
 	enumerator_.reset(nullptr);
 }
 
-void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
+void CamerasList::addCamera(std::shared_ptr<Camera> camera,
 				       const std::vector<dev_t> &devnums)
 {
 	MutexLocker locker(mutex_);
@@ -210,7 +212,7 @@  void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
 		camerasByDevnum_[devnum] = cameras_[index];
 }
 
-void CameraManager::Private::removeCamera(Camera *camera)
+void CamerasList::removeCamera(Camera *camera)
 {
 	MutexLocker locker(mutex_);
 
@@ -234,6 +236,13 @@  void CameraManager::Private::removeCamera(Camera *camera)
 	cameras_.erase(iter);
 }
 
+void CamerasList::stop()
+{
+	exit();
+	wait();
+}
+
+
 /**
  * \class CameraManager
  * \brief Provide access and manage all cameras in the system
@@ -244,9 +253,7 @@  void CameraManager::Private::removeCamera(Camera *camera)
  * handles hot-plugging and hot-unplugging to manage the lifetime of cameras.
  *
  * To interact with libcamera, an application starts by creating a camera
- * manager instance. Only a single instance of the camera manager may exist at
- * a time. Attempting to create a second instance without first deleting the
- * existing instance results in undefined behaviour.
+ * manager instance.
  *
  * The manager is initially stopped, and shall be started with start(). This
  * will enumerate all the cameras present in the system, which can then be
@@ -258,29 +265,19 @@  void CameraManager::Private::removeCamera(Camera *camera)
  * references it held to cameras, the camera manager can be stopped with
  * stop().
  */
-
-CameraManager *CameraManager::self_ = nullptr;
-
 CameraManager::CameraManager()
 	: Extensible(std::make_unique<CameraManager::Private>())
 {
-	if (self_)
-		LOG(Camera, Fatal)
-			<< "Multiple CameraManager objects are not allowed";
-
-	self_ = this;
 }
 
 /**
  * \brief Destroy the camera manager
  *
- * Destroying the camera manager stops it if it is currently running.
+ * Destroying the camera manager stops it if it is currently running
+ * and none of its cameras exist.
  */
 CameraManager::~CameraManager()
 {
-	stop();
-
-	self_ = nullptr;
 }
 
 /**
@@ -317,9 +314,8 @@  int CameraManager::start()
  */
 void CameraManager::stop()
 {
-	Private *const d = _d();
-	d->exit();
-	d->wait();
+	auto *const d = _d()->cl_.get();
+	d->stop();
 }
 
 /**
@@ -335,7 +331,7 @@  void CameraManager::stop()
  */
 std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
 {
-	const Private *const d = _d();
+	const auto *const d = _d()->cl_.get();
 
 	MutexLocker locker(d->mutex_);
 
@@ -355,7 +351,7 @@  std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
  */
 std::shared_ptr<Camera> CameraManager::get(const std::string &id)
 {
-	Private *const d = _d();
+	auto *const d = _d()->cl_.get();
 
 	MutexLocker locker(d->mutex_);
 
@@ -385,7 +381,7 @@  std::shared_ptr<Camera> CameraManager::get(const std::string &id)
  */
 std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
 {
-	Private *const d = _d();
+	auto *const d = _d()->cl_.get();
 
 	MutexLocker locker(d->mutex_);
 
@@ -441,7 +437,7 @@  std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
 void CameraManager::addCamera(std::shared_ptr<Camera> camera,
 			      const std::vector<dev_t> &devnums)
 {
-	Private *const d = _d();
+	auto *const d = _d()->cl_.get();
 
 	ASSERT(Thread::current() == d);
 
@@ -461,7 +457,7 @@  void CameraManager::addCamera(std::shared_ptr<Camera> camera,
  */
 void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
 {
-	Private *const d = _d();
+	auto *const d = _d()->cl_.get();
 
 	ASSERT(Thread::current() == d);
 
diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 0c67e35d..f57316bf 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -107,7 +107,7 @@  private:
 class PipelineHandlerISI : public PipelineHandler
 {
 public:
-	PipelineHandlerISI(CameraManager *manager);
+	PipelineHandlerISI(std::shared_ptr<CamerasList> manager);
 
 	bool match(DeviceEnumerator *enumerator) override;
 
@@ -564,7 +564,7 @@  CameraConfiguration::Status ISICameraConfiguration::validate()
  * Pipeline Handler
  */
 
-PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
+PipelineHandlerISI::PipelineHandlerISI(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager)
 {
 }
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 355cb0cb..b789ccc0 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -132,7 +132,7 @@  public:
 		IPU3PipeModeStillCapture = 1,
 	};
 
-	PipelineHandlerIPU3(CameraManager *manager);
+	PipelineHandlerIPU3(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -384,7 +384,7 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	return status;
 }
 
-PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
+PipelineHandlerIPU3::PipelineHandlerIPU3(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
 {
 }
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 00600441..4ddea305 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -364,7 +364,7 @@  private:
 class PipelineHandlerRPi : public PipelineHandler
 {
 public:
-	PipelineHandlerRPi(CameraManager *manager);
+	PipelineHandlerRPi(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -661,7 +661,7 @@  CameraConfiguration::Status RPiCameraConfiguration::validate()
 	return status;
 }
 
-PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
+PipelineHandlerRPi::PipelineHandlerRPi(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager)
 {
 }
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 8a30fe06..ef6d92b2 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -145,7 +145,7 @@  private:
 class PipelineHandlerRkISP1 : public PipelineHandler
 {
 public:
-	PipelineHandlerRkISP1(CameraManager *manager);
+	PipelineHandlerRkISP1(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -602,7 +602,7 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
  * Pipeline Operations
  */
 
-PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
+PipelineHandlerRkISP1::PipelineHandlerRkISP1(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager), hasSelfPath_(true)
 {
 }
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 2423ec10..a6e4cfca 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -309,7 +309,7 @@  private:
 class SimplePipelineHandler : public PipelineHandler
 {
 public:
-	SimplePipelineHandler(CameraManager *manager);
+	SimplePipelineHandler(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -1032,7 +1032,7 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
  * Pipeline Handler
  */
 
-SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
+SimplePipelineHandler::SimplePipelineHandler(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager), converter_(nullptr)
 {
 }
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 277465b7..1dad59d7 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -72,7 +72,7 @@  private:
 class PipelineHandlerUVC : public PipelineHandler
 {
 public:
-	PipelineHandlerUVC(CameraManager *manager);
+	PipelineHandlerUVC(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -173,7 +173,7 @@  CameraConfiguration::Status UVCCameraConfiguration::validate()
 	return status;
 }
 
-PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
+PipelineHandlerUVC::PipelineHandlerUVC(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager)
 {
 }
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 204f5ad7..e4c34586 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -82,7 +82,7 @@  private:
 class PipelineHandlerVimc : public PipelineHandler
 {
 public:
-	PipelineHandlerVimc(CameraManager *manager);
+	PipelineHandlerVimc(std::shared_ptr<CamerasList> manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -184,7 +184,7 @@  CameraConfiguration::Status VimcCameraConfiguration::validate()
 	return status;
 }
 
-PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
+PipelineHandlerVimc::PipelineHandlerVimc(std::shared_ptr<CamerasList> manager)
 	: PipelineHandler(manager)
 {
 }
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index f72613b8..07ab284a 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -20,6 +20,7 @@ 
 #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"
@@ -67,7 +68,7 @@  LOG_DEFINE_CATEGORY(Pipeline)
  * PipelineHandler instances shall never be constructed manually, but always
  * through the PipelineHandlerFactoryBase::create() function.
  */
-PipelineHandler::PipelineHandler(CameraManager *manager)
+PipelineHandler::PipelineHandler(std::shared_ptr<CamerasList> manager)
 	: manager_(manager), useCount_(0)
 {
 }
@@ -691,7 +692,7 @@  void PipelineHandler::disconnect()
 			continue;
 
 		camera->disconnect();
-		manager_->removeCamera(camera);
+		manager_->removeCamera(camera.get());
 	}
 }
 
@@ -743,7 +744,7 @@  PipelineHandlerFactoryBase::PipelineHandlerFactoryBase(const char *name)
  * \return A shared pointer to a new instance of the PipelineHandler subclass
  * corresponding to the factory
  */
-std::shared_ptr<PipelineHandler> PipelineHandlerFactoryBase::create(CameraManager *manager) const
+std::shared_ptr<PipelineHandler> PipelineHandlerFactoryBase::create(std::shared_ptr<CamerasList> manager) const
 {
 	std::unique_ptr<PipelineHandler> handler = createInstance(manager);
 	handler->name_ = name_.c_str();