[libcamera-devel] [PATCH 1/3] libcamera: pipeline_handler: Don't index factories by name

[Laurent Pinchart]

Pipeline handler factories are register in a map indexed by their name,
and the list of names is used to expose the factories and look them up.
This is unnecessary cumbersome, we can instead store factories in a
vector and expose it directly. The pipeline factory users will still
have access to the factory names through the factory name() function.

The PipelineHandlerFactory::create() method becomes so simple that it
can be inlined in its single caller, removing the unneeded usage of the
DeviceEnumerator in the factory.

Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
 src/libcamera/camera_manager.cpp         | 20 +++---
 src/libcamera/include/pipeline_handler.h | 29 ++++----
 src/libcamera/pipeline_handler.cpp       | 91 +++++++-----------------
 3 files changed, 52 insertions(+), 88 deletions(-)

diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index be327f5d5638..e4072529fdc4 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -74,20 +74,22 @@ int CameraManager::start()
 	 * file and only fallback on all handlers if there is no
 	 * configuration file.
 	 */
-	std::vector<std::string> handlers = PipelineHandlerFactory::handlers();
-
-	for (std::string const &handler : handlers) {
-		PipelineHandler *pipe;
+	std::vector<PipelineHandlerFactory *> &handlers = PipelineHandlerFactory::handlers();
 
+	for (PipelineHandlerFactory *factory : handlers) {
 		/*
 		 * Try each pipeline handler until it exhaust
 		 * all pipelines it can provide.
 		 */
-		do {
-			pipe = PipelineHandlerFactory::create(handler, enumerator_);
-			if (pipe)
-				pipes_.push_back(pipe);
-		} while (pipe);
+		while (1) {
+			PipelineHandler *pipe = factory->create();
+			if (!pipe->match(enumerator_)) {
+				delete pipe;
+				break;
+			}
+
+			pipes_.push_back(pipe);
+		}
 	}
 
 	/* TODO: register hot-plug callback here */
diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
index fdf8b8db2e0a..764dde9ccc65 100644
--- a/src/libcamera/include/pipeline_handler.h
+++ b/src/libcamera/include/pipeline_handler.h
@@ -31,29 +31,28 @@ public:
 class PipelineHandlerFactory
 {
 public:
+	PipelineHandlerFactory(const char *name);
 	virtual ~PipelineHandlerFactory() { };
 
 	virtual PipelineHandler *create() = 0;
 
-	static void registerType(const std::string &name, PipelineHandlerFactory *factory);
-	static PipelineHandler *create(const std::string &name, DeviceEnumerator *enumerator);
-	static std::vector<std::string> handlers();
+	const std::string &name() const { return name_; }
+
+	static void registerType(PipelineHandlerFactory *factory);
+	static std::vector<PipelineHandlerFactory *> &handlers();
 
 private:
-	static std::map<std::string, PipelineHandlerFactory *> &registry();
+	std::string name_;
 };
 
-#define REGISTER_PIPELINE_HANDLER(handler) \
-class handler##Factory : public PipelineHandlerFactory { \
-public: \
-	handler##Factory() \
-	{ \
-		PipelineHandlerFactory::registerType(#handler, this); \
-	} \
-	virtual PipelineHandler *create() { \
-		return new handler(); \
-	} \
-}; \
+#define REGISTER_PIPELINE_HANDLER(handler)				\
+class handler##Factory : public PipelineHandlerFactory {		\
+public:									\
+	handler##Factory() : PipelineHandlerFactory(#handler) { }	\
+	PipelineHandler *create() final {				\
+		return new handler();					\
+	}								\
+};									\
 static handler##Factory global_##handler##Factory;
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index ee7694879848..005a7619927f 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -5,7 +5,6 @@
  * pipeline_handler.cpp - Pipeline handler infrastructure
  */
 
-#include "device_enumerator.h"
 #include "log.h"
 #include "pipeline_handler.h"
 
@@ -40,7 +39,7 @@ namespace libcamera {
  * system
  *
  * This function is the main entry point of the pipeline handler. It is called
- * by the device enumerator with the \a enumerator passed as an argument. It
+ * by the camera manager with the \a enumerator passed as an argument. It
  * shall acquire from the \a enumerator all the media devices it needs for a
  * single pipeline and create one or multiple Camera instances.
  *
@@ -88,6 +87,21 @@ namespace libcamera {
  * static list of factories.
  */
 
+/**
+ * \brief Construct a pipeline handler factory
+ * \param[in] name Name of the pipeline handler class
+ *
+ * Creating an instance of the factory registers is with the global list of
+ * factories, accessible through the handlers() function.
+ *
+ * The factory \a name is used for debug purpose and shall be unique.
+ */
+PipelineHandlerFactory::PipelineHandlerFactory(const char *name)
+	: name_(name)
+{
+	registerType(this);
+}
+
 /**
  * \fn PipelineHandlerFactory::create()
  * \brief Create an instance of the PipelineHandler corresponding to the factory
@@ -98,75 +112,24 @@ namespace libcamera {
  * subclass corresponding to the factory
  */
 
+/**
+ * \fn PipelineHandlerFactory::name()
+ * \brief Retrieve the factory name
+ * \return The factory name
+ */
+
 /**
  * \brief Add a pipeline handler class to the registry
- * \param[in] name Name of the pipeline handler class
  * \param[in] factory Factory to use to construct the pipeline handler
  *
  * The caller is responsible to guarantee the uniqueness of the pipeline handler
  * name.
  */
-void PipelineHandlerFactory::registerType(const std::string &name,
-					  PipelineHandlerFactory *factory)
-{
-	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
-
-	if (factories.count(name)) {
-		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
-		return;
-	}
-
-	factories[name] = factory;
-}
-
-/**
- * \brief Create an instance of a pipeline handler if it matches media devices
- * present in the system
- * \param[in] name Name of the pipeline handler to instantiate
- * \param[in] enumerator Device enumerator to search for a match for the handler
- *
- * This function matches the media devices required by pipeline \a name against
- * the devices enumerated by \a enumerator.
- *
- * \return the newly created pipeline handler instance if a match was found, or
- * nullptr otherwise
- */
-PipelineHandler *PipelineHandlerFactory::create(const std::string &name,
-						DeviceEnumerator *enumerator)
-{
-	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
-
-	auto it = factories.find(name);
-	if (it == factories.end()) {
-		LOG(Error) << "Trying to create non-existing pipeline handler "
-			   << name;
-		return nullptr;
-	}
-
-	PipelineHandler *pipe = it->second->create();
-
-	if (pipe->match(enumerator))
-		return pipe;
-
-	delete pipe;
-	return nullptr;
-}
-
-/**
- * \brief Retrieve the names of all pipeline handlers registered with the
- * factory
- *
- * \return a list of all registered pipeline handler names
- */
-std::vector<std::string> PipelineHandlerFactory::handlers()
+void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
 {
-	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
-	std::vector<std::string> handlers;
-
-	for (auto const &handler : factories)
-		handlers.push_back(handler.first);
+	std::vector<PipelineHandlerFactory *> &factories = handlers();
 
-	return handlers;
+	factories.push_back(factory);
 }
 
 /**
@@ -177,9 +140,9 @@ std::vector<std::string> PipelineHandlerFactory::handlers()
  *
  * \return the list of pipeline handler factories
  */
-std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()
+std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::handlers()
 {
-	static std::map<std::string, PipelineHandlerFactory *> factories;
+	static std::vector<PipelineHandlerFactory *> factories;
 	return factories;
 }
 
-- 
Regards,

Laurent Pinchart