[{"id":294,"web_url":"https://patchwork.libcamera.org/comment/294/","msgid":"<20190114092432.zd5zyol7i2bcyrs7@uno.localdomain>","date":"2019-01-14T09:24:32","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: pipeline_handler:\n\tDon't index factories by name","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for the patches\n\nOn Sat, Jan 12, 2019 at 04:46:17PM +0200, Laurent Pinchart wrote:\n> Pipeline handler factories are register in a map indexed by their name,\n> and the list of names is used to expose the factories and look them up.\n> This is unnecessary cumbersome, we can instead store factories in a\n> vector and expose it directly. The pipeline factory users will still\n> have access to the factory names through the factory name() function.\n>\n> The PipelineHandlerFactory::create() method becomes so simple that it\n> can be inlined in its single caller, removing the unneeded usage of the\n> DeviceEnumerator in the factory.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/camera_manager.cpp | 20 +++---\n> src/libcamera/include/pipeline_handler.h | 29 ++++----\n> src/libcamera/pipeline_handler.cpp | 91 +++++++-----------------\n> 3 files changed, 52 insertions(+), 88 deletions(-)\n>\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index be327f5d5638..e4072529fdc4 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -74,20 +74,22 @@ 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\nI never really got why this method is called handlers, it return\n'factories', its return value is assigned to variables called\n'factories' where it is used... I would name this 'factories'\naccordingly while at there.\n\nAnd while at there, have a look at the \\return directive for this\nfunction, which is possibly outdated\n\n* \\return true on successful start false otherwise\n\nApart from this, this patch makes the code simpler, please add my\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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\tpipes_.push_back(pipe);\n> +\t\t}\n> \t}\n>\n> \t/* TODO: register hot-plug callback here */\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 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 */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index ee7694879848..005a7619927f 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,75 +112,24 @@ 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> -\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> -{\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> -\n> -\tif (pipe->match(enumerator))\n> -\t\treturn pipe;\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> +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)\n> {\n> -\tstd::map &factories = registry();\n> -\tstd::vector handlers;\n> -\n> -\tfor (auto const &handler : factories)\n> -\t\thandlers.push_back(handler.first);\n> +\tstd::vector &factories = handlers();\n>\n> -\treturn handlers;\n> +\tfactories.push_back(factory);\n> }\n>\n> /**\n> @@ -177,9 +140,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> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6277C60C68\n\tfor ;\n\tMon, 14 Jan 2019 10:24:28 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id D717E6000C;\n\tMon, 14 Jan 2019 09:24:27 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 14 Jan 2019 10:24:32 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190114092432.zd5zyol7i2bcyrs7@uno.localdomain>","References":"<20190112144619.5816-1-laurent.pinchart@ideasonboard.com>\n\t<20190112144619.5816-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"isgq4oz7zbqzltvv\"","Content-Disposition":"inline","In-Reply-To":"<20190112144619.5816-2-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: pipeline_handler:\n\tDon't index 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":"Mon, 14 Jan 2019 09:24:28 -0000"}}]