[{"id":343,"web_url":"https://patchwork.libcamera.org/comment/343/","msgid":"<20190115221400.GE7393@bigcity.dyn.berto.se>","date":"2019-01-15T22:14:00","subject":"Re: [libcamera-devel] [PATCH v2 1/8] libcamera: pipeline_handler:\n\tDon't index factories by name","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-01-15 17:18:42 +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 <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThis patch could have benefited from '--diff-algorithm=patience' :-)\n\nApart from that really nice work!\n\n> ---\n> Changes 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(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 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<std::string> handlers = PipelineHandlerFactory::handlers();\n> -\n> -\tfor (std::string const &handler : handlers) {\n> -\t\tPipelineHandler *pipe;\n> +\tstd::vector<PipelineHandlerFactory *> &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 */\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<std::string> handlers();\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tstatic void registerType(PipelineHandlerFactory *factory);\n> +\tstatic std::vector<PipelineHandlerFactory *> &handlers();\n>  \n>  private:\n> -\tstatic std::map<std::string, PipelineHandlerFactory *> &registry();\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 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<std::string, PipelineHandlerFactory *> &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<std::string, PipelineHandlerFactory *> &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<PipelineHandlerFactory *> &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<std::string> PipelineHandlerFactory::handlers()\n> -{\n> -\tstd::map<std::string, PipelineHandlerFactory *> &factories = registry();\n> -\tstd::vector<std::string> handlers;\n> +\tfor (PipelineHandlerFactory *f : factories)\n> +\t\tASSERT(factory->name() != f->name());\n\nIs this check needed? The name come from the class name passed to \nREGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name \nwould we not get a symbol collision at compile time?\n\nWith or without this fixed\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\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<std::string> PipelineHandlerFactory::handlers()\n>   *\n>   * \\return the list of pipeline handler factories\n>   */\n> -std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()\n> +std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::handlers()\n>  {\n> -\tstatic std::map<std::string, PipelineHandlerFactory *> factories;\n> +\tstatic std::vector<PipelineHandlerFactory *> 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":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C731060C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jan 2019 23:14:02 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id g11-v6so3756740ljk.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jan 2019 14:14:02 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tm63-v6sm836840lje.81.2019.01.15.14.14.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 15 Jan 2019 14:14:01 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=HmbB41vVjy74w23eUa2C5CMtN9Rhcp7MpMmsUDVH2Yo=;\n\tb=wBxnj2YsCQl6cjL1Svf0+DgNquZdCPhSuKC7X78m2Qul5TqhsN9Y1UOrHZsC1DRqrQ\n\tz7+q8lpdVCMBily23V7EMiPsvfuuqkWzUENQxtGLonYpWvVqwD1ZWpHhCVJJ85QDv9SB\n\tlMCD8Mo2kFhQge/mrNumLLELBK8NC9XvjqNAHcWKyw2ex54wor6jbFXk5Ylf2yId9AVU\n\tHzN05AhufxWv2KYtRfgPK4+paOtwxMPxA/z86s6CZQ9WdtLoLepcHzrKfgf7g7SNbp69\n\tTIFeQL2i2xa+DhkpG/VjKCxx/78+zt6TOvbScnRMl88QTTVdLT12bWDRupGkWR3SKvrQ\n\tVj9w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=HmbB41vVjy74w23eUa2C5CMtN9Rhcp7MpMmsUDVH2Yo=;\n\tb=SiAElg/hIs3RXe4FP0sfaQF/k0XLLQI1ERlKqR9NjNdMg50MwgIAqgyc9RGDI9W1l+\n\t+DBOk7qbVdI8/c6GCXibMoRCwP1h4yFnfy7leAVolxXMOtYYr2xN+dNig7kAsrYE9fwf\n\tusR0w2APgBhivlOMIMx/+MseBn8595rXsU+0zyRgKAULUyyMCM/wHcUZV8AVcHv6PJ1M\n\t8dOjhXpk803L8u2DXR8N3XBvp/5rzlSU+w6pi8pIDzOIqlxN96XE5YWMg3y83N8g12TF\n\tIuZjKoR5t7xwFuZGdHzEPIqKsefqmNvjYxwypKfzigjgLFbMFVNAopf0ZOo26kpcPi8r\n\tqw7A==","X-Gm-Message-State":"AJcUukeyq0gLoPv9EwaTGtc2PEOikVNTGCPfbqGho4lci/5MolpWYrLT\n\t+Kp/kIt/mAPUMXI/ELCsCYkMUziTZnQ=","X-Google-Smtp-Source":"ALg8bN6woIaRGY+tpukQ71N02SOqZ9Y2INXJua2apE/FAhHFZtJSKbdLRqxDWDwzMSnhwWGwD8DskA==","X-Received":"by 2002:a2e:81a:: with SMTP id\n\t26-v6mr4621475lji.14.1547590441931; \n\tTue, 15 Jan 2019 14:14:01 -0800 (PST)","Date":"Tue, 15 Jan 2019 23:14:00 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190115221400.GE7393@bigcity.dyn.berto.se>","References":"<20190115151849.1547-1-laurent.pinchart@ideasonboard.com>\n\t<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/8] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 15 Jan 2019 22:14:03 -0000"}},{"id":351,"web_url":"https://patchwork.libcamera.org/comment/351/","msgid":"<20190116000508.GA32066@pendragon.ideasonboard.com>","date":"2019-01-16T00:05:08","subject":"Re: [libcamera-devel] [PATCH v2 1/8] libcamera: pipeline_handler:\n\tDon't index factories by name","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Jan 15, 2019 at 11:14:00PM +0100, Niklas Söderlund wrote:\n> On 2019-01-15 17:18:42 +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 <laurent.pinchart@ideasonboard.com>\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> This patch could have benefited from '--diff-algorithm=patience' :-)\n> \n> Apart from that really nice work!\n> \n> > ---\n> > Changes 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(-)\n\n[snip]\n\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 1daada8653e2..08e3291e741a 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n\n[snip]\n\n> > +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)\n> >  {\n> > -\tstd::map<std::string, PipelineHandlerFactory *> &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<PipelineHandlerFactory *> &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<std::string> PipelineHandlerFactory::handlers()\n> > -{\n> > -\tstd::map<std::string, PipelineHandlerFactory *> &factories = registry();\n> > -\tstd::vector<std::string> handlers;\n> > +\tfor (PipelineHandlerFactory *f : factories)\n> > +\t\tASSERT(factory->name() != f->name());\n> \n> Is this check needed? The name come from the class name passed to \n> REGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name \n> would we not get a symbol collision at compile time?\n\nWe would, unless the classes are put in an anonymous namespace. Do you\nthink keeping the check is worth it ?\n\n> With or without this fixed\n> \n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \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[snip]","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53ACC60C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 01:05:08 +0100 (CET)","from pendragon.ideasonboard.com\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 C1E89530;\n\tWed, 16 Jan 2019 01:05:07 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547597107;\n\tbh=z2D7a+MTFIQV+vYOnqMUVNDNfZeDQTvhD5rhpWHv9Fc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MRYFzF7a0ALXKOXabYIo6RQXNS2G1PrwJ9tOVl/NvrFWwbpJqdL7MDKqSdUmyUJZu\n\tGNQ+07X30vHta/emWwezDZLgN9eTLuXyhOQF7pcSDXnOMitreQo8z+YrBN+OD57fVx\n\tfHA71v948UdggF5pgS3oBxw5fHBjMFffEKVo2fSk=","Date":"Wed, 16 Jan 2019 02:05:08 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190116000508.GA32066@pendragon.ideasonboard.com>","References":"<20190115151849.1547-1-laurent.pinchart@ideasonboard.com>\n\t<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>\n\t<20190115221400.GE7393@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190115221400.GE7393@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/8] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 16 Jan 2019 00:05:08 -0000"}},{"id":359,"web_url":"https://patchwork.libcamera.org/comment/359/","msgid":"<20190116122314.GM7393@bigcity.dyn.berto.se>","date":"2019-01-16T12:23:14","subject":"Re: [libcamera-devel] [PATCH v2 1/8] libcamera: pipeline_handler:\n\tDon't index factories by name","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2019-01-16 02:05:08 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Tue, Jan 15, 2019 at 11:14:00PM +0100, Niklas Söderlund wrote:\n> > On 2019-01-15 17:18:42 +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 <laurent.pinchart@ideasonboard.com>\n> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > \n> > This patch could have benefited from '--diff-algorithm=patience' :-)\n> > \n> > Apart from that really nice work!\n> > \n> > > ---\n> > > Changes 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(-)\n> \n> [snip]\n> \n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 1daada8653e2..08e3291e741a 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> \n> [snip]\n> \n> > > +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)\n> > >  {\n> > > -\tstd::map<std::string, PipelineHandlerFactory *> &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<PipelineHandlerFactory *> &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<std::string> PipelineHandlerFactory::handlers()\n> > > -{\n> > > -\tstd::map<std::string, PipelineHandlerFactory *> &factories = registry();\n> > > -\tstd::vector<std::string> handlers;\n> > > +\tfor (PipelineHandlerFactory *f : factories)\n> > > +\t\tASSERT(factory->name() != f->name());\n> > \n> > Is this check needed? The name come from the class name passed to \n> > REGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name \n> > would we not get a symbol collision at compile time?\n> \n> We would, unless the classes are put in an anonymous namespace. Do you\n> think keeping the check is worth it ?\n\nI think we can drop it. As I think it is possible if one really tries to \ncreate two handlers with the same name anyhow :-P\n\n> \n> > With or without this fixed\n> > \n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > \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> [snip]\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1687560B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 13:23:17 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id c19-v6so5271204lja.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 04:23:17 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tf8sm1165012lfb.2.2019.01.16.04.23.14\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 16 Jan 2019 04:23:15 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=Ddy2AOX1G+C6VQqyGApG1sRGRH3om5sqboVU01mvtNw=;\n\tb=tefzDtXuWafoPIz2l3CJVU8TRxYRX5z1fwUdvMY6CYiAgC45ofBBMVooTZEpzRyReB\n\t12378ksfjopWPTSQZy0218+KiU+g3NG0fMY8CDn7ioAR+gEoVD8PCjyqGRkiIyEBHAul\n\tI16EZW6W2YlbJ8VSXrIPWNPUiRwbfVe3iulzr5WmYceHseWa4Ihjj3KZLveTma3TWLno\n\tDeaRVAZ7DkEaob693/FDBnOeWg7wWREj7OlnUutAdMyVPgUXIaeXaga17YHbjUechIDm\n\td8CJJdszu4xMT/EhwyuRp05ivG5h5oYLqc8yKUUX036WqIBf+U+0yexPzDuQN7hLf7RB\n\tMnfg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=Ddy2AOX1G+C6VQqyGApG1sRGRH3om5sqboVU01mvtNw=;\n\tb=k6C0pKajBkaqLmtKMuw4a8FNwB39g9rap9EcpgiXgQRuk0kJ1mv/EafLJ+Ua3D2LAt\n\tmTntPMYNFrW2K7PLooAl0ww44NTvvxeHBP/rbAXthKxJbaF0kIOpxvyfTlkP4XAO+3gS\n\tkY55ZQjOatxFyFpVNOyxY/FEIUtzXsFnuhPu428pjqIfaGIdTjrK/iYl/L1WN9N5ySME\n\tu9y6sZfvd1vbZ2zNMwEy/3/DMH1AFtOmGC3HjBBkjXU3flBPxcFZusbk1u24ZC4aTVw8\n\tdSkm0zd4GE2csP6TI6SynSZoP2GuPWsSN+pFmDasS1milw7/GL7ib8Lx5mWhSwPugE1W\n\t01kg==","X-Gm-Message-State":"AJcUuke8ha78eA4kRnf1Slw9a0tdQORg3+MMvYDn03JwVuEgvgyF9qmA\n\tUFlSjH7pOiUuSvsGoxKErfaMoA==","X-Google-Smtp-Source":"ALg8bN5yV7u+JMZSgeKheUHK089SaJN7c/XisBgyXN4iyCX2s6PDn+ut6V+x9YUZjG6tPCS1ABN4Cg==","X-Received":"by 2002:a2e:6c04:: with SMTP id\n\th4-v6mr6258268ljc.92.1547641396145; \n\tWed, 16 Jan 2019 04:23:16 -0800 (PST)","Date":"Wed, 16 Jan 2019 13:23:14 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190116122314.GM7393@bigcity.dyn.berto.se>","References":"<20190115151849.1547-1-laurent.pinchart@ideasonboard.com>\n\t<20190115151849.1547-2-laurent.pinchart@ideasonboard.com>\n\t<20190115221400.GE7393@bigcity.dyn.berto.se>\n\t<20190116000508.GA32066@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190116000508.GA32066@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/8] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 16 Jan 2019 12:23:17 -0000"}}]