[{"id":128,"web_url":"https://patchwork.libcamera.org/comment/128/","msgid":"<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>","date":"2018-12-30T10:33:21","subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n  a few things you might want to address before or after pushing this\n\nOn Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:\n> Provide a PipelineHandler which represents a handler for one or more\n> media devices and provider of one or more cameras.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/include/pipeline_handler.h |  62 ++++++++++\n>  src/libcamera/meson.build                |   2 +\n>  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++\n>  3 files changed, 202 insertions(+)\n>  create mode 100644 src/libcamera/include/pipeline_handler.h\n>  create mode 100644 src/libcamera/pipeline_handler.cpp\n>\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> new file mode 100644\n> index 0000000000000000..a7805a01e1c779bd\n> --- /dev/null\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * pipeline_handler.h - Pipeline handler infrastructure\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n> +#define __LIBCAMERA_PIPELINE_HANDLER_H__\n> +\n> +#include <map>\n> +#include <string>\n> +#include <vector>\n> +\n> +#include <libcamera/camera.h>\n> +\n> +namespace libcamera {\n> +\n> +class DeviceEnumerator;\n> +class PipelineHandlerFactory;\n> +\n> +class PipelineHandler\n> +{\n> +public:\n> +\tvirtual ~PipelineHandler() { };\n> +\n> +\tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> +\n> +\tvirtual unsigned int count() = 0;\n> +\tvirtual Camera *camera(unsigned int id) = 0;\n> +};\n> +\n> +class PipelineHandlerFactory\n> +{\n> +public:\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 void handlers(std::vector<std::string> &handlers);\n> +\n> +private:\n> +\tstatic std::map<std::string, PipelineHandlerFactory *> &registry();\n> +};\n> +\n> +#define REGISTER_PIPELINE_HANDLER(handler) \\\n> +\tclass handler##Factory : public PipelineHandlerFactory { \\\n> +\tpublic: \\\n> +\t\thandler##Factory() \\\n> +\t\t{ \\\n> +\t\t\tPipelineHandlerFactory::registerType(#handler, this); \\\n> +\t\t} \\\n> +\t\tvirtual PipelineHandler *create() { \\\n> +\t\t\treturn new handler(); \\\n> +\t\t} \\\n> +\t}; \\\n> +\tstatic handler##Factory global_##handler##Factory;\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 581da1aa78ebb3ba..0db648dd3e37156e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -3,11 +3,13 @@ libcamera_sources = files([\n>      'device_enumerator.cpp',\n>      'log.cpp',\n>      'main.cpp',\n> +    'pipeline_handler.cpp',\n>  ])\n>\n>  libcamera_headers = files([\n>      'include/device_enumerator.h',\n>      'include/log.h',\n> +    'include/pipeline_handler.h',\n>      'include/utils.h',\n>  ])\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> new file mode 100644\n> index 0000000000000000..b6e28216a6636faf\n> --- /dev/null\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -0,0 +1,138 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\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> +/**\n> + * \\file pipeline_handler.h\n> + * \\brief Create pipelines and cameras from one or more media device\n\ndevices\n\n> + *\n> + * Each pipeline supported by libcamera needs to be backed by a pipeline\n> + * handler implementation which describes the one or many media devices\n> + * needed for a pipeline to function properly.\n> + *\n> + * The pipeline handler is responsible to find all media devices it requires\n\ns/to find/of finding/ ?\n\n> + * to operate and once it retrieves them create all the camera devices\n> + * it is able to support with the that set of devices.\n\ns/the//\n\nI would:\n\nThe pipeline handler is responsible of providing a description of the\nmedia devices it requires to operates on, and once it gets provided wit\nthem by the device enumerator it creates all camera devices it is able to\nsupport with that set of devices.\n\n> + *\n> + * To make it a bit less bit complicated to write pipe line handlers a\n> + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a pipeline\n> + * handler implementation to register itself with the library with ease.\n> + */\n> +\n> +namespace libcamera {\n> +\n\nnit: two empty lines\n> +\n> +/**\n> + * \\class PipelineHandler\n> + * \\brief Find a set of media devices and provide cameras\n> + *\n> + * The responsibility of a PipelineHandler is to describe all media\n> + * devices it would need in order to provide cameras to the system.\n> + */\n> +\n> +/**\n> + * \\class PipelineHandlerFactory\n> + * \\brief Keep a registry and create instances of available pipeline handlers\n> + *\n> + * The responsibility of the PipelineHandlerFactory is to keep a list\n> + * of all pipelines in the system. Each pipeline handler should register\n> + * it self with the factory using the REGISTER_PIPELINE_HANDLER() macro.\n> + */\n> +\n> +/**\n> + * \\brief Add a pipeline handler to the global list\n> + *\n> + * \\param[in] name Name of the pipeline handler to add\n> + * \\param[in] factory Factory to use to construct the pipeline\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the pipeline name.\n> + */\n> +void PipelineHandlerFactory::registerType(const std::string &name, PipelineHandlerFactory *factory)\n\nnit: You could break this...\n\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> +\tfactories[name] = factory;\n> +}\n> +\n> +/**\n> + * \\brief Create a new pipeline handler and try to match it\n\nand try to match the media devices it requires\n\n> + *\n> + * \\param[in] name Name of the pipeline handler to try\n> + * \\param[in] enumerator  Numerator to to search for a match for the handler\n\ns/Numerator/Device enumerator/\n\n> + *\n> + * Search \\a enumerator for a match for a pipeline handler named \\a name.\n\nTry to match the media devices this pipeline requires against the ones\nregistered in \\a enumerator.\n\n> + *\n> + * \\return Pipeline handler if a match was found else nullptr\ns/was found else/was found, nullptr otherwise/\n\n> + */\n> +PipelineHandler *PipelineHandlerFactory::create(const std::string &name, DeviceEnumerator *enumerator)\n\nnit: you could break this\n\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 \" << name;\n\nno value in breaking 80 cols here\n\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tPipelineHandler *pipe;\n> +\n> +\tpipe = it->second->create();\n\ndeclare and initialize on the same line.\n> +\n> +\tif (pipe->match(enumerator))\n> +\t\treturn pipe;\n> +\n> +\tdelete pipe;\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief List all names of handlers from the global list\n> + *\n> + * \\param[out] handlers Names of all handlers registered with the global list\n> + */\n> +void PipelineHandlerFactory::handlers(std::vector<std::string> &handlers)\n> +{\n> +\tstd::map<std::string, PipelineHandlerFactory *> &factories = registry();\n> +\n> +\tfor (auto const &handler : factories)\n> +\t\thandlers.push_back(handler.first);\n> +}\n> +\n> +/**\n> + * \\brief Static global list of pipeline handlers\n> + *\n> + * It might seem odd to hide the static map inside a function.\n> + * This is needed to make sure the list is created before anyone\n> + * tries to access it and creating problems at link time.\n> + *\n> + * \\return Global list of pipeline handlers\n> + */\n> +std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()\n> +{\n> +\tstatic std::map<std::string, PipelineHandlerFactory *> factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\def REGISTER_PIPELINE_HANDLER\n> + * \\brief Register a pipeline handler with the global list\n> + *\n> + * \\param[in] handler Class name of PipelineHandler subclass to register\n\nnit: derived class\n\n> + *\n> + * Register a specifc pipline handler with the global list and make it\n> + * avaiable to try and match devices for libcamera.\n\ns/for libcamera//\n\nFeel free to address this later, this is all minor stuff and I'm fine\nmerging code as it is.\n\nThanks\n  j\n\n\n> + */\n> +\n> +} /* namespace libcamera */\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87A4A60B31\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 11:33:51 +0100 (CET)","from uno.localdomain (unknown [37.176.180.32])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id C22741BF206;\n\tSun, 30 Dec 2018 10:33:19 +0000 (UTC)"],"X-Originating-IP":"37.176.180.32","Date":"Sun, 30 Dec 2018 11:33:21 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-10-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"2wmu7zw62jfhzjzs\"","Content-Disposition":"inline","In-Reply-To":"<20181229032855.26249-10-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","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":"Sun, 30 Dec 2018 10:33:51 -0000"}},{"id":133,"web_url":"https://patchwork.libcamera.org/comment/133/","msgid":"<20181230202353.GB31866@bigcity.dyn.berto.se>","date":"2018-12-30T20:23:53","subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2018-12-30 11:33:21 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n>   a few things you might want to address before or after pushing this\n> \n> On Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:\n> > Provide a PipelineHandler which represents a handler for one or more\n> > media devices and provider of one or more cameras.\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/libcamera/include/pipeline_handler.h |  62 ++++++++++\n> >  src/libcamera/meson.build                |   2 +\n> >  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++\n> >  3 files changed, 202 insertions(+)\n> >  create mode 100644 src/libcamera/include/pipeline_handler.h\n> >  create mode 100644 src/libcamera/pipeline_handler.cpp\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > new file mode 100644\n> > index 0000000000000000..a7805a01e1c779bd\n> > --- /dev/null\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -0,0 +1,62 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * pipeline_handler.h - Pipeline handler infrastructure\n> > + */\n> > +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n> > +#define __LIBCAMERA_PIPELINE_HANDLER_H__\n> > +\n> > +#include <map>\n> > +#include <string>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/camera.h>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class DeviceEnumerator;\n> > +class PipelineHandlerFactory;\n> > +\n> > +class PipelineHandler\n> > +{\n> > +public:\n> > +\tvirtual ~PipelineHandler() { };\n> > +\n> > +\tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > +\n> > +\tvirtual unsigned int count() = 0;\n> > +\tvirtual Camera *camera(unsigned int id) = 0;\n> > +};\n> > +\n> > +class PipelineHandlerFactory\n> > +{\n> > +public:\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 void handlers(std::vector<std::string> &handlers);\n> > +\n> > +private:\n> > +\tstatic std::map<std::string, PipelineHandlerFactory *> &registry();\n> > +};\n> > +\n> > +#define REGISTER_PIPELINE_HANDLER(handler) \\\n> > +\tclass handler##Factory : public PipelineHandlerFactory { \\\n> > +\tpublic: \\\n> > +\t\thandler##Factory() \\\n> > +\t\t{ \\\n> > +\t\t\tPipelineHandlerFactory::registerType(#handler, this); \\\n> > +\t\t} \\\n> > +\t\tvirtual PipelineHandler *create() { \\\n> > +\t\t\treturn new handler(); \\\n> > +\t\t} \\\n> > +\t}; \\\n> > +\tstatic handler##Factory global_##handler##Factory;\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 581da1aa78ebb3ba..0db648dd3e37156e 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -3,11 +3,13 @@ libcamera_sources = files([\n> >      'device_enumerator.cpp',\n> >      'log.cpp',\n> >      'main.cpp',\n> > +    'pipeline_handler.cpp',\n> >  ])\n> >\n> >  libcamera_headers = files([\n> >      'include/device_enumerator.h',\n> >      'include/log.h',\n> > +    'include/pipeline_handler.h',\n> >      'include/utils.h',\n> >  ])\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > new file mode 100644\n> > index 0000000000000000..b6e28216a6636faf\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -0,0 +1,138 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\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> > +/**\n> > + * \\file pipeline_handler.h\n> > + * \\brief Create pipelines and cameras from one or more media device\n> \n> devices\n> \n\nThanks.\n\n> > + *\n> > + * Each pipeline supported by libcamera needs to be backed by a pipeline\n> > + * handler implementation which describes the one or many media devices\n> > + * needed for a pipeline to function properly.\n> > + *\n> > + * The pipeline handler is responsible to find all media devices it requires\n> \n> s/to find/of finding/ ?\n> \n> > + * to operate and once it retrieves them create all the camera devices\n> > + * it is able to support with the that set of devices.\n> \n> s/the//\n> \n> I would:\n> \n> The pipeline handler is responsible of providing a description of the\n> media devices it requires to operates on, and once it gets provided wit\n> them by the device enumerator it creates all camera devices it is able to\n> support with that set of devices.\n\nSome parts of your text is better then mine, I have merged to in my view \nbest of both and ended up with:\n\n The pipeline handler is responsible of providing a description of the\n media devices it requires to operate. Once all media devices can be\n provided the pipeline handler can acquire them and create camera\n devices which utilize the acquired media devices.\n\n> \n> > + *\n> > + * To make it a bit less bit complicated to write pipe line handlers a\n> > + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a pipeline\n> > + * handler implementation to register itself with the library with ease.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> \n> nit: two empty lines\n\nThanks.\n\n> > +\n> > +/**\n> > + * \\class PipelineHandler\n> > + * \\brief Find a set of media devices and provide cameras\n> > + *\n> > + * The responsibility of a PipelineHandler is to describe all media\n> > + * devices it would need in order to provide cameras to the system.\n> > + */\n> > +\n> > +/**\n> > + * \\class PipelineHandlerFactory\n> > + * \\brief Keep a registry and create instances of available pipeline handlers\n> > + *\n> > + * The responsibility of the PipelineHandlerFactory is to keep a list\n> > + * of all pipelines in the system. Each pipeline handler should register\n> > + * it self with the factory using the REGISTER_PIPELINE_HANDLER() macro.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Add a pipeline handler to the global list\n> > + *\n> > + * \\param[in] name Name of the pipeline handler to add\n> > + * \\param[in] factory Factory to use to construct the pipeline\n> > + *\n> > + * The caller is responsible to guarantee the uniqueness of the pipeline name.\n> > + */\n> > +void PipelineHandlerFactory::registerType(const std::string &name, PipelineHandlerFactory *factory)\n> \n> nit: You could break this...\n> \n\nDone.\n\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> > +\tfactories[name] = factory;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a new pipeline handler and try to match it\n> \n> and try to match the media devices it requires\n> \n\nMuch better, thanks.\n\n> > + *\n> > + * \\param[in] name Name of the pipeline handler to try\n> > + * \\param[in] enumerator  Numerator to to search for a match for the handler\n> \n> s/Numerator/Device enumerator/\n\nWops :-)\n\n> \n> > + *\n> > + * Search \\a enumerator for a match for a pipeline handler named \\a name.\n> \n> Try to match the media devices this pipeline requires against the ones\n> registered in \\a enumerator.\n\nThanks.\n\n> \n> > + *\n> > + * \\return Pipeline handler if a match was found else nullptr\n> s/was found else/was found, nullptr otherwise/\n\nThanks.\n\n> \n> > + */\n> > +PipelineHandler *PipelineHandlerFactory::create(const std::string &name, DeviceEnumerator *enumerator)\n> \n> nit: you could break this\n\nDone.\n\n> \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 \" << name;\n> \n> no value in breaking 80 cols here\n\nDone.\n\n> \n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tPipelineHandler *pipe;\n> > +\n> > +\tpipe = it->second->create();\n> \n> declare and initialize on the same line.\n\nDone.\n\n> > +\n> > +\tif (pipe->match(enumerator))\n> > +\t\treturn pipe;\n> > +\n> > +\tdelete pipe;\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief List all names of handlers from the global list\n> > + *\n> > + * \\param[out] handlers Names of all handlers registered with the global list\n> > + */\n> > +void PipelineHandlerFactory::handlers(std::vector<std::string> &handlers)\n> > +{\n> > +\tstd::map<std::string, PipelineHandlerFactory *> &factories = registry();\n> > +\n> > +\tfor (auto const &handler : factories)\n> > +\t\thandlers.push_back(handler.first);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Static global list of pipeline handlers\n> > + *\n> > + * It might seem odd to hide the static map inside a function.\n> > + * This is needed to make sure the list is created before anyone\n> > + * tries to access it and creating problems at link time.\n> > + *\n> > + * \\return Global list of pipeline handlers\n> > + */\n> > +std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()\n> > +{\n> > +\tstatic std::map<std::string, PipelineHandlerFactory *> factories;\n> > +\treturn factories;\n> > +}\n> > +\n> > +/**\n> > + * \\def REGISTER_PIPELINE_HANDLER\n> > + * \\brief Register a pipeline handler with the global list\n> > + *\n> > + * \\param[in] handler Class name of PipelineHandler subclass to register\n> \n> nit: derived class\n> \n> > + *\n> > + * Register a specifc pipline handler with the global list and make it\n> > + * avaiable to try and match devices for libcamera.\n> \n> s/for libcamera//\n\nThanks. I also fixed s/specifc/specific/ s/pipline/pipeline/ and \ns/avaiable/available/. Seems my spellchecker was not enabled at the \ntime... :-)\n\n> \n> Feel free to address this later, this is all minor stuff and I'm fine\n> merging code as it is.\n\nDoes this mean I can add your review tag with all of the above fixed?\n\n> \n> Thanks\n>   j\n> \n> \n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.20.1\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-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 805C3600CC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 21:24:02 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id v15-v6so22501985ljh.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 12:24: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\td82sm9099475lfd.82.2018.12.30.12.23.53\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 12:23:53 -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=ZhNPV16A6vi6TNthUvPzPUj8yjIOPKV4PCD2P3Nqt3U=;\n\tb=CwVrk7QPQXG24+TTnn9/CYpbbOELKCKldGSuKxBKfyvNztsDMvlWvWQTpD8PQWcag9\n\tPMCZCFIVNgxAMTUSw+8IV1HwgCCDW9RX81W8VKc2ornL981A6bF3BCwzyWRcB2mdFuED\n\tdECMUfilQqoXawGT4l/PYKEudQXakbSXCTOUlZwQEB97TPU4lXxs5lbzQd7ZWxvFW00P\n\tQ6AmGPMg85k+mwfpkcN9OJnwkjJrmh+g6/lpB2xqvO3ozhn2y8reOt7TShqq+d5u/lLH\n\t8wzF4FhTs/4b4hV/UmZH0n/jMP5gj6zu4h+VdTCU8d23WdRC+S2PBoMuvVlXqcDXwHhZ\n\t82kA==","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=ZhNPV16A6vi6TNthUvPzPUj8yjIOPKV4PCD2P3Nqt3U=;\n\tb=AnCYMPUtUiBv0orXdKDj84WVT7AMhslZPvrTY2nrs6+IK1sp1kuXbAz6nFFXOYgKuk\n\tYTLeJKS7W3wMglYjSJsH3J0JG77yHjiBIdgMvSQsoZSU00gtSbXlrSE/Pzjn24hK68xy\n\tTJr3NwoJdvIjsIVw80/50Uoyr8fxMglt2Xlfx9NNpXWGiVdOXkUUm4JDfoMdCozzIx4J\n\t756zoeTcomE2zjVJA0QanKVPqm6OaFF8taszdSihF8PBFKs5IERxznhcJKD2kWbKzD2w\n\tSl+Low59QHkySbHs06SXYSr3wwPBr+TtmIS8fsgpZKSz7yeeMu6O2JA6rpV+oFzPNWZp\n\tX5Ew==","X-Gm-Message-State":"AA+aEWYApEUml+Ry5e9HBWmrku4c93TgNn3j/kw0046dkbzdtXs/LaEC\n\tNkXtxDffekkPxfncrHgoMqxkPpQNONc=","X-Google-Smtp-Source":"ALg8bN5lM6teeQtTg/naoemz6ydnezwf6hV+L/ifCpW6SUaunyEBbdGeATjCp2y1H301yaXV5WdaQg==","X-Received":"by 2002:a2e:9f0b:: with SMTP id\n\tu11-v6mr17171426ljk.99.1546201434661; \n\tSun, 30 Dec 2018 12:23:54 -0800 (PST)","Date":"Sun, 30 Dec 2018 21:23:53 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230202353.GB31866@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-10-niklas.soderlund@ragnatech.se>\n\t<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","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":"Sun, 30 Dec 2018 20:24:02 -0000"}},{"id":139,"web_url":"https://patchwork.libcamera.org/comment/139/","msgid":"<6530004.8rO4h8kE09@avalon>","date":"2018-12-30T21:15:43","subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sunday, 30 December 2018 22:23:53 EET Niklas Söderlund wrote:\n> On 2018-12-30 11:33:21 +0100, Jacopo Mondi wrote:\n> > On Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:\n> > > Provide a PipelineHandler which represents a handler for one or more\n> > > media devices and provider of one or more cameras.\n\ns/provider/provides/\n\n> > > \n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > > \n> > >  src/libcamera/include/pipeline_handler.h |  62 ++++++++++\n> > >  src/libcamera/meson.build                |   2 +\n> > >  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++\n> > >  3 files changed, 202 insertions(+)\n> > >  create mode 100644 src/libcamera/include/pipeline_handler.h\n> > >  create mode 100644 src/libcamera/pipeline_handler.cpp\n> > > \n> > > diff --git a/src/libcamera/include/pipeline_handler.h\n> > > b/src/libcamera/include/pipeline_handler.h new file mode 100644\n> > > index 0000000000000000..a7805a01e1c779bd\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/pipeline_handler.h\n> > > @@ -0,0 +1,62 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * pipeline_handler.h - Pipeline handler infrastructure\n> > > + */\n> > > +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n> > > +#define __LIBCAMERA_PIPELINE_HANDLER_H__\n> > > +\n> > > +#include <map>\n> > > +#include <string>\n> > > +#include <vector>\n> > > +\n> > > +#include <libcamera/camera.h>\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class DeviceEnumerator;\n> > > +class PipelineHandlerFactory;\n> > > +\n> > > +class PipelineHandler\n> > > +{\n> > > +public:\n> > > +\tvirtual ~PipelineHandler() { };\n> > > +\n> > > +\tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > > +\n> > > +\tvirtual unsigned int count() = 0;\n> > > +\tvirtual Camera *camera(unsigned int id) = 0;\n> > > +};\n> > > +\n> > > +class PipelineHandlerFactory\n> > > +{\n> > > +public:\n> > > +\tvirtual ~PipelineHandlerFactory() { };\n> > > +\n> > > +\tvirtual PipelineHandler *create() = 0;\n> > > +\n> > > +\tstatic void registerType(const std::string &name,\n> > > PipelineHandlerFactory *factory);\n> > > +\tstatic PipelineHandler *create(const std::string &name,\n> > > DeviceEnumerator *enumerator);\n> > > +\tstatic void handlers(std::vector<std::string> &handlers);\n> > > +\n> > > +private:\n> > > +\tstatic std::map<std::string, PipelineHandlerFactory *> &registry();\n> > > +};\n> > > +\n> > > +#define REGISTER_PIPELINE_HANDLER(handler) \\\n> > > +\tclass handler##Factory : public PipelineHandlerFactory { \\\n> > > +\tpublic: \\\n> > > +\t\thandler##Factory() \\\n> > > +\t\t{ \\\n> > > +\t\t\tPipelineHandlerFactory::registerType(#handler, this); \\\n> > > +\t\t} \\\n> > > +\t\tvirtual PipelineHandler *create() { \\\n> > > +\t\t\treturn new handler(); \\\n> > > +\t\t} \\\n> > > +\t}; \\\n> > > +\tstatic handler##Factory global_##handler##Factory;\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 581da1aa78ebb3ba..0db648dd3e37156e 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -3,11 +3,13 @@ libcamera_sources = files([\n> > >      'device_enumerator.cpp',\n> > >      'log.cpp',\n> > >      'main.cpp',\n> > > +    'pipeline_handler.cpp',\n> > >  ])\n> > >  \n> > >  libcamera_headers = files([\n> > >      'include/device_enumerator.h',\n> > >      'include/log.h',\n> > > +    'include/pipeline_handler.h',\n> > >      'include/utils.h',\n> > >  ])\n> > > \n> > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > b/src/libcamera/pipeline_handler.cpp new file mode 100644\n> > > index 0000000000000000..b6e28216a6636faf\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -0,0 +1,138 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\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> > > +/**\n> > > + * \\file pipeline_handler.h\n> > > + * \\brief Create pipelines and cameras from one or more media device\n> > \n> > devices\n> \n> Thanks.\n> \n> > > + *\n> > > + * Each pipeline supported by libcamera needs to be backed by a\n> > > pipeline\n> > > + * handler implementation which describes the one or many media devices\n> > > + * needed for a pipeline to function properly.\n> > > + *\n> > > + * The pipeline handler is responsible to find all media devices it\n> > > requires\n> > \n> > s/to find/of finding/ ?\n> > \n> > > + * to operate and once it retrieves them create all the camera devices\n> > > + * it is able to support with the that set of devices.\n> > \n> > s/the//\n> > \n> > I would:\n> > \n> > The pipeline handler is responsible of providing a description of the\n> > media devices it requires to operates on, and once it gets provided wit\n> > them by the device enumerator it creates all camera devices it is able to\n> > support with that set of devices.\n> \n> Some parts of your text is better then mine, I have merged to in my view\n> best of both and ended up with:\n> \n>  The pipeline handler is responsible of providing a description of the\n\ns/of providing/for providing/\n\n>  media devices it requires to operate. Once all media devices can be\n>  provided the pipeline handler can acquire them and create camera\n>  devices which utilize the acquired media devices.\n\ns/which/that/\n\n> > > + *\n> > > + * To make it a bit less bit complicated to write pipe line handlers a\n> > > + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a\n> > > pipeline\n> > > + * handler implementation to register itself with the library with\n> > > ease.\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > \n> > nit: two empty lines\n> \n> Thanks.\n> \n> > > +\n> > > +/**\n> > > + * \\class PipelineHandler\n> > > + * \\brief Find a set of media devices and provide cameras\n> > > + *\n> > > + * The responsibility of a PipelineHandler is to describe all media\n> > > + * devices it would need in order to provide cameras to the system.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class PipelineHandlerFactory\n> > > + * \\brief Keep a registry and create instances of available pipeline\n> > > handlers + *\n> > > + * The responsibility of the PipelineHandlerFactory is to keep a list\n> > > + * of all pipelines in the system. Each pipeline handler should\n> > > register\n> > > + * it self with the factory using the REGISTER_PIPELINE_HANDLER()\n> > > macro.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Add a pipeline handler to the global list\n> > > + *\n> > > + * \\param[in] name Name of the pipeline handler to add\n> > > + * \\param[in] factory Factory to use to construct the pipeline\n> > > + *\n> > > + * The caller is responsible to guarantee the uniqueness of the\n> > > pipeline name.\n> > > + */\n> > > +void PipelineHandlerFactory::registerType(const std::string &name,\n> > > PipelineHandlerFactory *factory)\n> > \n> > nit: You could break this...\n> \n> Done.\n> \n> > > +{\n> > > +\tstd::map<std::string, PipelineHandlerFactory *> &factories =\n> > > 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 a new pipeline handler and try to match it\n> > \n> > and try to match the media devices it requires\n> \n> Much better, thanks.\n> \n> > > + *\n> > > + * \\param[in] name Name of the pipeline handler to try\n> > > + * \\param[in] enumerator  Numerator to to search for a match for the\n> > > handler\n> > \n> > s/Numerator/Device enumerator/\n> \n> Wops :-)\n\nAnd s/to to/to/\n\n> > > + *\n> > > + * Search \\a enumerator for a match for a pipeline handler named \\a\n> > > name.\n> > \n> > Try to match the media devices this pipeline requires against the ones\n> > registered in \\a enumerator.\n> \n> Thanks.\n> \n> > > + *\n> > > + * \\return Pipeline handler if a match was found else nullptr\n> > \n> > s/was found else/was found, nullptr otherwise/\n> \n> Thanks.\n> \n> > > + */\n> > > +PipelineHandler *PipelineHandlerFactory::create(const std::string\n> > > &name, DeviceEnumerator *enumerator)\n> > \n> > nit: you could break this\n> \n> Done.\n> \n> > > +{\n> > > +\tstd::map<std::string, PipelineHandlerFactory *> &factories =\n> > > 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<<\n> > > name;\n> > \n> > no value in breaking 80 cols here\n> \n> Done.\n> \n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tPipelineHandler *pipe;\n> > > +\n> > > +\tpipe = it->second->create();\n> > \n> > declare and initialize on the same line.\n> \n> Done.\n> \n> > > +\n> > > +\tif (pipe->match(enumerator))\n> > > +\t\treturn pipe;\n> > > +\n> > > +\tdelete pipe;\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief List all names of handlers from the global list\n> > > + *\n> > > + * \\param[out] handlers Names of all handlers registered with the\n> > > global list\n> > > + */\n> > > +void PipelineHandlerFactory::handlers(std::vector<std::string>\n> > > &handlers)\n> > > +{\n> > > +\tstd::map<std::string, PipelineHandlerFactory *> &factories =\n> > > registry();\n> > > +\n> > > +\tfor (auto const &handler : factories)\n> > > +\t\thandlers.push_back(handler.first);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Static global list of pipeline handlers\n> > > + *\n> > > + * It might seem odd to hide the static map inside a function.\n> > > + * This is needed to make sure the list is created before anyone\n> > > + * tries to access it and creating problems at link time.\n\nNo need to apologize about the oddity :-)\n\n\"The static factories map is defined inside the function to ensures it gets \ninitialized on first use, without any dependency on link order.\"\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > > + * \\return Global list of pipeline handlers\n> > > + */\n> > > +std::map<std::string, PipelineHandlerFactory *>\n> > > &PipelineHandlerFactory::registry()\n> > > +{\n> > > +\tstatic std::map<std::string, PipelineHandlerFactory *> factories;\n> > > +\treturn factories;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\def REGISTER_PIPELINE_HANDLER\n> > > + * \\brief Register a pipeline handler with the global list\n> > > + *\n> > > + * \\param[in] handler Class name of PipelineHandler subclass to\n> > > register\n> > \n> > nit: derived class\n> > \n> > > + *\n> > > + * Register a specifc pipline handler with the global list and make it\n> > > + * avaiable to try and match devices for libcamera.\n> > \n> > s/for libcamera//\n> \n> Thanks. I also fixed s/specifc/specific/ s/pipline/pipeline/ and\n> s/avaiable/available/. Seems my spellchecker was not enabled at the\n> time... :-)\n> \n> > Feel free to address this later, this is all minor stuff and I'm fine\n> > merging code as it is.\n> \n> Does this mean I can add your review tag with all of the above fixed?\n> \n> > > + */\n> > > +\n> > > +} /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","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 3A655600CC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 22:14:48 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:a03f:3ad5:900:7d1d:858b:75d5:534d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64EC550A;\n\tSun, 30 Dec 2018 22:14:46 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546204486;\n\tbh=50Aty/uLYoqn9VoDr6t/iiPy2zhrS9f5eGX9o8W3uh8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=iMwyuucGfa2Vdo9H4QoaGWNMNwBPzvF1eDH+LautrOp46P3cZytvWVp3ewyI05OqU\n\t5OCZ9ezuBdwgiZGCVMvhoe82qJmDhgiCh8rTuFrFfdCwuaXSPHjnBrsNb4gU5BDI2d\n\tvvhL1F6GwNCGY0Snf4jk/KXdlX6gU54dJj7mC0Eg=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Sun, 30 Dec 2018 23:15:43 +0200","Message-ID":"<6530004.8rO4h8kE09@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181230202353.GB31866@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>\n\t<20181230202353.GB31866@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","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":"Sun, 30 Dec 2018 21:14:48 -0000"}},{"id":146,"web_url":"https://patchwork.libcamera.org/comment/146/","msgid":"<20181230225821.GB10929@bigcity.dyn.berto.se>","date":"2018-12-30T22:58:22","subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nAll grammar comments addressed, thanks!\n\nOn 2018-12-30 23:15:43 +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > > > +/**\n> > > > + * \\brief Static global list of pipeline handlers\n> > > > + *\n> > > > + * It might seem odd to hide the static map inside a function.\n> > > > + * This is needed to make sure the list is created before anyone\n> > > > + * tries to access it and creating problems at link time.\n> \n> No need to apologize about the oddity :-)\n> \n> \"The static factories map is defined inside the function to ensures it gets \n> initialized on first use, without any dependency on link order.\"\n\nMuch better! Thanks for providing this and your tag!\n\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08FD8600CC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 23:58:24 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id p86so17536431lfg.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Dec 2018 14:58:23 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te97-v6sm9945062lji.51.2018.12.30.14.58.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 14:58:22 -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=fJc9zThBgXc2/j4LrMk8nkKNURcQI7dyv8+rpvAmhnM=;\n\tb=zvb4+WWqt/PeojdFQqWngWESF9L70PCNaa9ROu/H7TyJxR7fiaJam09H8++9yoNoYB\n\tt/iXKZ4xtksy1/U0Johz8nTWp6VhgALrjfUdPxNFWjZ6eTyquNfv3cUmZEBbh/mNrFpI\n\tcc9fkfAyTRDG4XkQxRwEbwA0olDD+oKvMKw0ceEFKVdQuwR2ci8BOe0Z5S+r3a+F1gIi\n\trH4rWuHymrKRA4BZEl8064QDQHAaJsj1jez3ldUUNN3g1KF0w8v5pkEdHCD4KiViN2Hn\n\t04ec7s55J9AWOr56Lo8kVtQ43RkG6IuO5kGlcTOyn/JbsxfZuVgy+6wGShwBmN8vad8l\n\tgYKA==","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=fJc9zThBgXc2/j4LrMk8nkKNURcQI7dyv8+rpvAmhnM=;\n\tb=a7ep5Tqmj2EeItEPZ7u5GJYh9yIY6HWz5UTihPpOx5ENG7bpS5BajEnIhPPuZccWsv\n\t8GKriRVBVdw0WtQtdlCmUpcYEmfQNuMd52Js/2du7yOx49i8Ur6RZcC1BCbCmPc2iRXE\n\trt9R411EeXc9aL5hlRNgSu9BqNN2XxKbGMBugfis7jmDFv3Hwvtttu3JqhqNuNw+tGgU\n\tziDjuD9CqHyLVpbAn3wv+Zong96YGl57hHxyACrsQrbEIS55XlpsHRLcjGzo34xCdNIP\n\txt4IdpeNuBs+ZeWD6mYTcdVckPuPYBFDw98wWh9Wd99xjXSsrp1eIqbBzPUlcDLFhZ0c\n\tPSMA==","X-Gm-Message-State":"AA+aEWZqAnIpyKYDTwPTM1eAO6y920Lq5wU4ZOnXTKI+feNBZy6u1ztj\n\td0J+FQp4ydC7pi95ActHK2o6CB7JzkM=","X-Google-Smtp-Source":"AFSGD/Xkxp+I0+LSX7EOOmIaz1jFLAQUuPrtMl1I/EDhM9rtoysk4OvA0dYHrYHVE+esQ9HNoSQyGQ==","X-Received":"by 2002:a19:1f54:: with SMTP id\n\tf81mr16895175lff.153.1546210703104; \n\tSun, 30 Dec 2018 14:58:23 -0800 (PST)","Date":"Sun, 30 Dec 2018 23:58:22 +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, Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20181230225821.GB10929@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181230103321.lxhdr5lxy5kw5vxz@uno.localdomain>\n\t<20181230202353.GB31866@bigcity.dyn.berto.se>\n\t<6530004.8rO4h8kE09@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6530004.8rO4h8kE09@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler:\n\tadd PipelineHandler class","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":"Sun, 30 Dec 2018 22:58:24 -0000"}}]