{"id":233,"url":"https://patchwork.libcamera.org/api/1.1/patches/233/?format=json","web_url":"https://patchwork.libcamera.org/patch/233/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>","date":"2019-01-15T15:18:42","name":"[libcamera-devel,v2,1/8] libcamera: pipeline_handler: Don't index factories by name","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"bc32d28e84c4f9cb8176393d436860bc9baa65bb","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/1.1/people/2/?format=json","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/233/mbox/","series":[{"id":81,"url":"https://patchwork.libcamera.org/api/1.1/series/81/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=81","date":"2019-01-15T15:18:41","name":"Pipeline handler refactoring and assorted improvements","version":2,"mbox":"https://patchwork.libcamera.org/series/81/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/233/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/233/checks/","tags":{},"headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19E2660C78\n\tfor ;\n\tTue, 15 Jan 2019 16:18:53 +0100 (CET)","from pendragon.bb.dnainternet.fi\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84BDA55C\n\tfor ;\n\tTue, 15 Jan 2019 16:18:52 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547565532;\n\tbh=kdznshboMzFNLLFQJ+OaNhbXtZBIq03ilNd5xxWeZUk=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=AiX1vnmpEwKCvJthZ+TTHUC7/g8dQD0bzW2FHynqAAPDSXsROIkuSK58blox3hgul\n\t1eYgBNSwcQJ/corHu6A4mJ1RkWX8vv+J1CP6SXiTCSbwh/zI3E54dzicHBF/xqshpY\n\tW3nyIkM6nhYP/Cpr+ZZQCmotFfO3C/6xtdz/0MJc=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Tue, 15 Jan 2019 17:18:42 +0200","Message-Id":"<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>","X-Mailer":"git-send-email 2.19.2","In-Reply-To":"<20190115151849.1547-1-laurent.pinchart@ideasonboard.com>","References":"<20190115151849.1547-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH v2 1/8] libcamera: pipeline_handler: Don't\n\tindex factories by name","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 15:18:53 -0000"},"content":"Pipeline handler factories are register in a map indexed by their name,\nand the list of names is used to expose the factories and look them up.\nThis is unnecessary cumbersome, we can instead store factories in a\nvector and expose it directly. The pipeline factory users will still\nhave access to the factory names through the factory name() function.\n\nThe PipelineHandlerFactory::create() method becomes so simple that it\ncan be inlined in its single caller, removing the unneeded usage of the\nDeviceEnumerator in the factory.\n\nSigned-off-by: Laurent Pinchart \nReviewed-by: Jacopo Mondi \n---\nChanges since v1:\n\n- ASSERT() factory name uniqueness\n---\n src/libcamera/camera_manager.cpp | 22 +++---\n src/libcamera/include/pipeline_handler.h | 29 ++++----\n src/libcamera/pipeline_handler.cpp | 95 ++++++++----------------\n 3 files changed, 57 insertions(+), 89 deletions(-)","diff":"diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex be327f5d5638..91ef6753f405 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -74,20 +74,24 @@ int CameraManager::start()\n \t * file and only fallback on all handlers if there is no\n \t * configuration file.\n \t */\n-\tstd::vector handlers = PipelineHandlerFactory::handlers();\n-\n-\tfor (std::string const &handler : handlers) {\n-\t\tPipelineHandler *pipe;\n+\tstd::vector &handlers = PipelineHandlerFactory::handlers();\n \n+\tfor (PipelineHandlerFactory *factory : handlers) {\n \t\t/*\n \t\t * Try each pipeline handler until it exhaust\n \t\t * all pipelines it can provide.\n \t\t */\n-\t\tdo {\n-\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_);\n-\t\t\tif (pipe)\n-\t\t\t\tpipes_.push_back(pipe);\n-\t\t} while (pipe);\n+\t\twhile (1) {\n+\t\t\tPipelineHandler *pipe = factory->create();\n+\t\t\tif (!pipe->match(enumerator_)) {\n+\t\t\t\tdelete pipe;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tLOG(Debug) << \"Pipeline handler \\\"\" << factory->name()\n+\t\t\t\t << \"\\\" matched\";\n+\t\t\tpipes_.push_back(pipe);\n+\t\t}\n \t}\n \n \t/* TODO: register hot-plug callback here */\ndiff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\nindex fdf8b8db2e0a..764dde9ccc65 100644\n--- a/src/libcamera/include/pipeline_handler.h\n+++ b/src/libcamera/include/pipeline_handler.h\n@@ -31,29 +31,28 @@ public:\n class PipelineHandlerFactory\n {\n public:\n+\tPipelineHandlerFactory(const char *name);\n \tvirtual ~PipelineHandlerFactory() { };\n \n \tvirtual PipelineHandler *create() = 0;\n \n-\tstatic void registerType(const std::string &name, PipelineHandlerFactory *factory);\n-\tstatic PipelineHandler *create(const std::string &name, DeviceEnumerator *enumerator);\n-\tstatic std::vector handlers();\n+\tconst std::string &name() const { return name_; }\n+\n+\tstatic void registerType(PipelineHandlerFactory *factory);\n+\tstatic std::vector &handlers();\n \n private:\n-\tstatic std::map ®istry();\n+\tstd::string name_;\n };\n \n-#define REGISTER_PIPELINE_HANDLER(handler) \\\n-class handler##Factory : public PipelineHandlerFactory { \\\n-public: \\\n-\thandler##Factory() \\\n-\t{ \\\n-\t\tPipelineHandlerFactory::registerType(#handler, this); \\\n-\t} \\\n-\tvirtual PipelineHandler *create() { \\\n-\t\treturn new handler(); \\\n-\t} \\\n-}; \\\n+#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n+class handler##Factory : public PipelineHandlerFactory {\t\t\\\n+public:\t\t\t\t\t\t\t\t\t\\\n+\thandler##Factory() : PipelineHandlerFactory(#handler) { }\t\\\n+\tPipelineHandler *create() final {\t\t\t\t\\\n+\t\treturn new handler();\t\t\t\t\t\\\n+\t}\t\t\t\t\t\t\t\t\\\n+};\t\t\t\t\t\t\t\t\t\\\n static handler##Factory global_##handler##Factory;\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex 1daada8653e2..08e3291e741a 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -5,7 +5,6 @@\n * pipeline_handler.cpp - Pipeline handler infrastructure\n */\n \n-#include \"device_enumerator.h\"\n #include \"log.h\"\n #include \"pipeline_handler.h\"\n \n@@ -40,7 +39,7 @@ namespace libcamera {\n * system\n *\n * This function is the main entry point of the pipeline handler. It is called\n- * by the device enumerator with the \\a enumerator passed as an argument. It\n+ * by the camera manager with the \\a enumerator passed as an argument. It\n * shall acquire from the \\a enumerator all the media devices it needs for a\n * single pipeline and create one or multiple Camera instances.\n *\n@@ -88,6 +87,21 @@ namespace libcamera {\n * static list of factories.\n */\n \n+/**\n+ * \\brief Construct a pipeline handler factory\n+ * \\param[in] name Name of the pipeline handler class\n+ *\n+ * Creating an instance of the factory registers is with the global list of\n+ * factories, accessible through the handlers() function.\n+ *\n+ * The factory \\a name is used for debug purpose and shall be unique.\n+ */\n+PipelineHandlerFactory::PipelineHandlerFactory(const char *name)\n+\t: name_(name)\n+{\n+\tregisterType(this);\n+}\n+\n /**\n * \\fn PipelineHandlerFactory::create()\n * \\brief Create an instance of the PipelineHandler corresponding to the factory\n@@ -98,78 +112,29 @@ namespace libcamera {\n * subclass corresponding to the factory\n */\n \n+/**\n+ * \\fn PipelineHandlerFactory::name()\n+ * \\brief Retrieve the factory name\n+ * \\return The factory name\n+ */\n+\n /**\n * \\brief Add a pipeline handler class to the registry\n- * \\param[in] name Name of the pipeline handler class\n * \\param[in] factory Factory to use to construct the pipeline handler\n *\n * The caller is responsible to guarantee the uniqueness of the pipeline handler\n * name.\n */\n-void PipelineHandlerFactory::registerType(const std::string &name,\n-\t\t\t\t\t PipelineHandlerFactory *factory)\n-{\n-\tstd::map &factories = registry();\n-\n-\tif (factories.count(name)) {\n-\t\tLOG(Error) << \"Registering '\" << name << \"' pipeline twice\";\n-\t\treturn;\n-\t}\n-\n-\tLOG(Debug) << \"Registered pipeline handler \\\"\" << name << \"\\\"\";\n-\tfactories[name] = factory;\n-}\n-\n-/**\n- * \\brief Create an instance of a pipeline handler if it matches media devices\n- * present in the system\n- * \\param[in] name Name of the pipeline handler to instantiate\n- * \\param[in] enumerator Device enumerator to search for a match for the handler\n- *\n- * This function matches the media devices required by pipeline \\a name against\n- * the devices enumerated by \\a enumerator.\n- *\n- * \\return the newly created pipeline handler instance if a match was found, or\n- * nullptr otherwise\n- */\n-PipelineHandler *PipelineHandlerFactory::create(const std::string &name,\n-\t\t\t\t\t\tDeviceEnumerator *enumerator)\n+void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)\n {\n-\tstd::map &factories = registry();\n-\n-\tauto it = factories.find(name);\n-\tif (it == factories.end()) {\n-\t\tLOG(Error) << \"Trying to create non-existing pipeline handler \"\n-\t\t\t << name;\n-\t\treturn nullptr;\n-\t}\n-\n-\tPipelineHandler *pipe = it->second->create();\n+\tstd::vector &factories = handlers();\n \n-\tif (pipe->match(enumerator)) {\n-\t\tLOG(Debug) << \"Pipeline handler \\\"\" << name << \"\\\" matched\";\n-\t\treturn pipe;\n-\t}\n-\n-\tdelete pipe;\n-\treturn nullptr;\n-}\n-\n-/**\n- * \\brief Retrieve the names of all pipeline handlers registered with the\n- * factory\n- *\n- * \\return a list of all registered pipeline handler names\n- */\n-std::vector PipelineHandlerFactory::handlers()\n-{\n-\tstd::map &factories = registry();\n-\tstd::vector handlers;\n+\tfor (PipelineHandlerFactory *f : factories)\n+\t\tASSERT(factory->name() != f->name());\n \n-\tfor (auto const &handler : factories)\n-\t\thandlers.push_back(handler.first);\n+\tfactories.push_back(factory);\n \n-\treturn handlers;\n+\tLOG(Debug) << \"Registered pipeline handler \\\"\" << factory->name() << \"\\\"\";\n }\n \n /**\n@@ -180,9 +145,9 @@ std::vector PipelineHandlerFactory::handlers()\n *\n * \\return the list of pipeline handler factories\n */\n-std::map &PipelineHandlerFactory::registry()\n+std::vector &PipelineHandlerFactory::handlers()\n {\n-\tstatic std::map factories;\n+\tstatic std::vector factories;\n \treturn factories;\n }\n \n","prefixes":["libcamera-devel","v2","1/8"]}