[{"id":13779,"web_url":"https://patchwork.libcamera.org/comment/13779/","msgid":"<20201118132059.vuyxwrh7nezr7bzs@uno.localdomain>","date":"2020-11-18T13:20:59","subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Nov 06, 2020 at 07:36:59PM +0900, Paul Elder wrote:\n> Implementations of IPA proxies use a registration mechanism to register\n> themselves with the main IPA proxy factory. This registration declares\n> static objects, causing a risk of things being constructed before the\n> proper libcamera facilities are ready. Since each pipeline handler has\n> its own IPA proxy and knows the type, it isn't necessary to have a proxy\n> factory. Remove it to alleviate the risk of early construction.\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n>\n\nVery minor, but since the factory is now gone, I wonder if the typedef\nin ipa_module.h\n\n        typedef IPAInterface *(*IPAIntfFactory)(void);\n\nShould not be changed.\n\nApart from this\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n> ---\n> No change in v4\n>\n> No change in v3\n>\n> Changes in v2:\n> - remove documentation\n> ---\n>  include/libcamera/internal/ipa_proxy.h | 29 --------\n>  src/libcamera/ipa_proxy.cpp            | 93 --------------------------\n>  2 files changed, 122 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> index 195d2cb4..f651a3ae 100644\n> --- a/include/libcamera/internal/ipa_proxy.h\n> +++ b/include/libcamera/internal/ipa_proxy.h\n> @@ -36,35 +36,6 @@ private:\n>  \tIPAModule *ipam_;\n>  };\n>\n> -class IPAProxyFactory\n> -{\n> -public:\n> -\tIPAProxyFactory(const char *name);\n> -\tvirtual ~IPAProxyFactory() = default;\n> -\n> -\tvirtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;\n> -\n> -\tconst std::string &name() const { return name_; }\n> -\n> -\tstatic void registerType(IPAProxyFactory *factory);\n> -\tstatic std::vector<IPAProxyFactory *> &factories();\n> -\n> -private:\n> -\tstd::string name_;\n> -};\n> -\n> -#define REGISTER_IPA_PROXY(proxy)\t\t\t\t\t\\\n> -class proxy##Factory final : public IPAProxyFactory\t\t\t\\\n> -{\t\t\t\t\t\t\t\t\t\\\n> -public:\t\t\t\t\t\t\t\t\t\\\n> -\tproxy##Factory() : IPAProxyFactory(#proxy) {}\t\t\t\\\n> -\tstd::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)\t\\\n> -\t{\t\t\t\t\t\t\t\t\\\n> -\t\treturn std::make_unique<proxy>(ipam, isolate);\t\t\\\n> -\t}\t\t\t\t\t\t\t\t\\\n> -};\t\t\t\t\t\t\t\t\t\\\n> -static proxy##Factory global_##proxy##Factory;\n> -\n>  } /* namespace libcamera */\n>\n>  #endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_H__ */\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index ee5e6f3e..29c0e9e0 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)\n>   * \\brief IPA Proxy\n>   *\n>   * Isolate IPA into separate process.\n> - *\n> - * Every subclass of proxy shall be registered with libcamera using\n> - * the REGISTER_IPA_PROXY() macro.\n>   */\n>\n>  /**\n>   * \\brief Construct an IPAProxy instance\n>   * \\param[in] ipam The IPA module\n> - *\n> - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()\n> - * method implemented by the respective factories.\n>   */\n>  IPAProxy::IPAProxy(IPAModule *ipam)\n>  \t: valid_(false), ipam_(ipam)\n> @@ -219,91 +213,4 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n>   * construction.\n>   */\n>\n> -/**\n> - * \\class IPAProxyFactory\n> - * \\brief Registration of IPAProxy classes and creation of instances\n> - *\n> - * To facilitate discovery and instantiation of IPAProxy classes, the\n> - * IPAProxyFactory class maintains a registry of IPAProxy classes. Each\n> - * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY()\n> - * macro, which will create a corresponding instance of a IPAProxyFactory\n> - * subclass and register it with the static list of factories.\n> - */\n> -\n> -/**\n> - * \\brief Construct a IPAProxy factory\n> - * \\param[in] name Name of the IPAProxy class\n> - *\n> - * Creating an instance of the factory registers is with the global list of\n> - * factories, accessible through the factories() function.\n> - *\n> - * The factory \\a name is used for debugging and IPAProxy matching purposes\n> - * and shall be unique.\n> - */\n> -IPAProxyFactory::IPAProxyFactory(const char *name)\n> -\t: name_(name)\n> -{\n> -\tregisterType(this);\n> -}\n> -\n> -/**\n> - * \\fn IPAProxyFactory::create()\n> - * \\brief Create an instance of the IPAProxy corresponding to the factory\n> - * \\param[in] ipam The IPA module\n> - * \\param[in] isolate Flag to isolate the IPA module\n> - *\n> - * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.\n> - * It creates a IPAProxy instance that isolates an IPA interface designated\n> - * by the IPA module \\a ipam. If \\a isolate is true, then the IPA module will\n> - * be isolated.\n> - *\n> - * \\return A pointer to a newly constructed instance of the IPAProxy subclass\n> - * corresponding to the factory\n> - */\n> -\n> -/**\n> - * \\fn IPAProxyFactory::name()\n> - * \\brief Retrieve the factory name\n> - * \\return The factory name\n> - */\n> -\n> -/**\n> - * \\brief Add a IPAProxy class to the registry\n> - * \\param[in] factory Factory to use to construct the IPAProxy\n> - *\n> - * The caller is responsible to guarantee the uniqueness of the IPAProxy name.\n> - */\n> -void IPAProxyFactory::registerType(IPAProxyFactory *factory)\n> -{\n> -\tstd::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();\n> -\n> -\tfactories.push_back(factory);\n> -\n> -\tLOG(IPAProxy, Debug)\n> -\t\t<< \"Registered proxy \\\"\" << factory->name() << \"\\\"\";\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the list of all IPAProxy factories\n> - *\n> - * The static factories map is defined inside the function to ensure it gets\n> - * initialized on first use, without any dependency on link order.\n> - *\n> - * \\return The list of pipeline handler factories\n> - */\n> -std::vector<IPAProxyFactory *> &IPAProxyFactory::factories()\n> -{\n> -\tstatic std::vector<IPAProxyFactory *> factories;\n> -\treturn factories;\n> -}\n> -\n> -/**\n> - * \\def REGISTER_IPA_PROXY\n> - * \\brief Register a IPAProxy with the IPAProxy factory\n> - * \\param[in] proxy Class name of IPAProxy derived class to register\n> - *\n> - * Register a proxy subclass with the factory and make it available to\n> - * isolate IPA modules.\n> - */\n> -\n>  } /* namespace libcamera */\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C2C6EBE08A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Nov 2020 13:20:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45FA76156F;\n\tWed, 18 Nov 2020 14:20:57 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 445A661565\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Nov 2020 14:20:56 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id C402A40007;\n\tWed, 18 Nov 2020 13:20:55 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 18 Nov 2020 14:20:59 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20201118132059.vuyxwrh7nezr7bzs@uno.localdomain>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-30-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-30-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13795,"web_url":"https://patchwork.libcamera.org/comment/13795/","msgid":"<20201119031655.GJ1856@pyrite.rasen.tech>","date":"2020-11-19T03:16:55","subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Nov 18, 2020 at 02:20:59PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Nov 06, 2020 at 07:36:59PM +0900, Paul Elder wrote:\n> > Implementations of IPA proxies use a registration mechanism to register\n> > themselves with the main IPA proxy factory. This registration declares\n> > static objects, causing a risk of things being constructed before the\n> > proper libcamera facilities are ready. Since each pipeline handler has\n> > its own IPA proxy and knows the type, it isn't necessary to have a proxy\n> > factory. Remove it to alleviate the risk of early construction.\n> >\n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> >\n> \n> Very minor, but since the factory is now gone, I wonder if the typedef\n> in ipa_module.h\n> \n>         typedef IPAInterface *(*IPAIntfFactory)(void);\n\nThis is different. This refers to the function that's loaded from the\nIPA .so that constructs the IPAInterface:\n\nIPAInterface *ipaCreate()\n{\n\treturn new IPARPi();\n}\n\nI think it's fair to call it an IPAIntfFactory. At most it could be\nrenamed to IPAIntfConstructor, which I don't think is any better.\n\n> Should not be changed.\n> \n> Apart from this\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\n\nThanks,\n\nPaul\n\n> > ---\n> > No change in v4\n> >\n> > No change in v3\n> >\n> > Changes in v2:\n> > - remove documentation\n> > ---\n> >  include/libcamera/internal/ipa_proxy.h | 29 --------\n> >  src/libcamera/ipa_proxy.cpp            | 93 --------------------------\n> >  2 files changed, 122 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> > index 195d2cb4..f651a3ae 100644\n> > --- a/include/libcamera/internal/ipa_proxy.h\n> > +++ b/include/libcamera/internal/ipa_proxy.h\n> > @@ -36,35 +36,6 @@ private:\n> >  \tIPAModule *ipam_;\n> >  };\n> >\n> > -class IPAProxyFactory\n> > -{\n> > -public:\n> > -\tIPAProxyFactory(const char *name);\n> > -\tvirtual ~IPAProxyFactory() = default;\n> > -\n> > -\tvirtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;\n> > -\n> > -\tconst std::string &name() const { return name_; }\n> > -\n> > -\tstatic void registerType(IPAProxyFactory *factory);\n> > -\tstatic std::vector<IPAProxyFactory *> &factories();\n> > -\n> > -private:\n> > -\tstd::string name_;\n> > -};\n> > -\n> > -#define REGISTER_IPA_PROXY(proxy)\t\t\t\t\t\\\n> > -class proxy##Factory final : public IPAProxyFactory\t\t\t\\\n> > -{\t\t\t\t\t\t\t\t\t\\\n> > -public:\t\t\t\t\t\t\t\t\t\\\n> > -\tproxy##Factory() : IPAProxyFactory(#proxy) {}\t\t\t\\\n> > -\tstd::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)\t\\\n> > -\t{\t\t\t\t\t\t\t\t\\\n> > -\t\treturn std::make_unique<proxy>(ipam, isolate);\t\t\\\n> > -\t}\t\t\t\t\t\t\t\t\\\n> > -};\t\t\t\t\t\t\t\t\t\\\n> > -static proxy##Factory global_##proxy##Factory;\n> > -\n> >  } /* namespace libcamera */\n> >\n> >  #endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_H__ */\n> > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> > index ee5e6f3e..29c0e9e0 100644\n> > --- a/src/libcamera/ipa_proxy.cpp\n> > +++ b/src/libcamera/ipa_proxy.cpp\n> > @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)\n> >   * \\brief IPA Proxy\n> >   *\n> >   * Isolate IPA into separate process.\n> > - *\n> > - * Every subclass of proxy shall be registered with libcamera using\n> > - * the REGISTER_IPA_PROXY() macro.\n> >   */\n> >\n> >  /**\n> >   * \\brief Construct an IPAProxy instance\n> >   * \\param[in] ipam The IPA module\n> > - *\n> > - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()\n> > - * method implemented by the respective factories.\n> >   */\n> >  IPAProxy::IPAProxy(IPAModule *ipam)\n> >  \t: valid_(false), ipam_(ipam)\n> > @@ -219,91 +213,4 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n> >   * construction.\n> >   */\n> >\n> > -/**\n> > - * \\class IPAProxyFactory\n> > - * \\brief Registration of IPAProxy classes and creation of instances\n> > - *\n> > - * To facilitate discovery and instantiation of IPAProxy classes, the\n> > - * IPAProxyFactory class maintains a registry of IPAProxy classes. Each\n> > - * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY()\n> > - * macro, which will create a corresponding instance of a IPAProxyFactory\n> > - * subclass and register it with the static list of factories.\n> > - */\n> > -\n> > -/**\n> > - * \\brief Construct a IPAProxy factory\n> > - * \\param[in] name Name of the IPAProxy class\n> > - *\n> > - * Creating an instance of the factory registers is with the global list of\n> > - * factories, accessible through the factories() function.\n> > - *\n> > - * The factory \\a name is used for debugging and IPAProxy matching purposes\n> > - * and shall be unique.\n> > - */\n> > -IPAProxyFactory::IPAProxyFactory(const char *name)\n> > -\t: name_(name)\n> > -{\n> > -\tregisterType(this);\n> > -}\n> > -\n> > -/**\n> > - * \\fn IPAProxyFactory::create()\n> > - * \\brief Create an instance of the IPAProxy corresponding to the factory\n> > - * \\param[in] ipam The IPA module\n> > - * \\param[in] isolate Flag to isolate the IPA module\n> > - *\n> > - * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.\n> > - * It creates a IPAProxy instance that isolates an IPA interface designated\n> > - * by the IPA module \\a ipam. If \\a isolate is true, then the IPA module will\n> > - * be isolated.\n> > - *\n> > - * \\return A pointer to a newly constructed instance of the IPAProxy subclass\n> > - * corresponding to the factory\n> > - */\n> > -\n> > -/**\n> > - * \\fn IPAProxyFactory::name()\n> > - * \\brief Retrieve the factory name\n> > - * \\return The factory name\n> > - */\n> > -\n> > -/**\n> > - * \\brief Add a IPAProxy class to the registry\n> > - * \\param[in] factory Factory to use to construct the IPAProxy\n> > - *\n> > - * The caller is responsible to guarantee the uniqueness of the IPAProxy name.\n> > - */\n> > -void IPAProxyFactory::registerType(IPAProxyFactory *factory)\n> > -{\n> > -\tstd::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();\n> > -\n> > -\tfactories.push_back(factory);\n> > -\n> > -\tLOG(IPAProxy, Debug)\n> > -\t\t<< \"Registered proxy \\\"\" << factory->name() << \"\\\"\";\n> > -}\n> > -\n> > -/**\n> > - * \\brief Retrieve the list of all IPAProxy factories\n> > - *\n> > - * The static factories map is defined inside the function to ensure it gets\n> > - * initialized on first use, without any dependency on link order.\n> > - *\n> > - * \\return The list of pipeline handler factories\n> > - */\n> > -std::vector<IPAProxyFactory *> &IPAProxyFactory::factories()\n> > -{\n> > -\tstatic std::vector<IPAProxyFactory *> factories;\n> > -\treturn factories;\n> > -}\n> > -\n> > -/**\n> > - * \\def REGISTER_IPA_PROXY\n> > - * \\brief Register a IPAProxy with the IPAProxy factory\n> > - * \\param[in] proxy Class name of IPAProxy derived class to register\n> > - *\n> > - * Register a proxy subclass with the factory and make it available to\n> > - * isolate IPA modules.\n> > - */\n> > -\n> >  } /* namespace libcamera */\n> > --\n> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D175EBE08A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 19 Nov 2020 03:17:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FB936159D;\n\tThu, 19 Nov 2020 04:17:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 254D060336\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 19 Nov 2020 04:17:03 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6A8193;\n\tThu, 19 Nov 2020 04:17:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EwQnelzZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605755822;\n\tbh=2q1yGKhZkpQBqpZ0/X+ql31F47hitjKBhkSegCA3P2o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EwQnelzZP28Wd85rzlzd2cwZT0u/vZrXLJ5hAj54DpTynGcuCbRqTsAFHjmIN9x0O\n\tMwGImWBHiiOlLG+ANGOohhBzPo6B00AsQGChsNjqgHawj2SBgHY2LR7lWCm/Ji0TnB\n\tpFSA5gZD0scryyiahoLmElwtx4JHxdqgeKiUogx4=","Date":"Thu, 19 Nov 2020 12:16:55 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20201119031655.GJ1856@pyrite.rasen.tech>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-30-paul.elder@ideasonboard.com>\n\t<20201118132059.vuyxwrh7nezr7bzs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201118132059.vuyxwrh7nezr7bzs@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13927,"web_url":"https://patchwork.libcamera.org/comment/13927/","msgid":"<20201126154612.GR3905@pendragon.ideasonboard.com>","date":"2020-11-26T15:46:12","subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Nov 06, 2020 at 07:36:59PM +0900, Paul Elder wrote:\n> Implementations of IPA proxies use a registration mechanism to register\n> themselves with the main IPA proxy factory. This registration declares\n> static objects, causing a risk of things being constructed before the\n> proper libcamera facilities are ready. Since each pipeline handler has\n> its own IPA proxy and knows the type, it isn't necessary to have a proxy\n> factory. Remove it to alleviate the risk of early construction.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n> No change in v4\n> \n> No change in v3\n> \n> Changes in v2:\n> - remove documentation\n> ---\n>  include/libcamera/internal/ipa_proxy.h | 29 --------\n>  src/libcamera/ipa_proxy.cpp            | 93 --------------------------\n>  2 files changed, 122 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> index 195d2cb4..f651a3ae 100644\n> --- a/include/libcamera/internal/ipa_proxy.h\n> +++ b/include/libcamera/internal/ipa_proxy.h\n> @@ -36,35 +36,6 @@ private:\n>  \tIPAModule *ipam_;\n>  };\n>  \n> -class IPAProxyFactory\n> -{\n> -public:\n> -\tIPAProxyFactory(const char *name);\n> -\tvirtual ~IPAProxyFactory() = default;\n> -\n> -\tvirtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;\n> -\n> -\tconst std::string &name() const { return name_; }\n> -\n> -\tstatic void registerType(IPAProxyFactory *factory);\n> -\tstatic std::vector<IPAProxyFactory *> &factories();\n> -\n> -private:\n> -\tstd::string name_;\n> -};\n> -\n> -#define REGISTER_IPA_PROXY(proxy)\t\t\t\t\t\\\n> -class proxy##Factory final : public IPAProxyFactory\t\t\t\\\n> -{\t\t\t\t\t\t\t\t\t\\\n> -public:\t\t\t\t\t\t\t\t\t\\\n> -\tproxy##Factory() : IPAProxyFactory(#proxy) {}\t\t\t\\\n> -\tstd::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)\t\\\n> -\t{\t\t\t\t\t\t\t\t\\\n> -\t\treturn std::make_unique<proxy>(ipam, isolate);\t\t\\\n> -\t}\t\t\t\t\t\t\t\t\\\n> -};\t\t\t\t\t\t\t\t\t\\\n> -static proxy##Factory global_##proxy##Factory;\n> -\n>  } /* namespace libcamera */\n>  \n>  #endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_H__ */\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index ee5e6f3e..29c0e9e0 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)\n>   * \\brief IPA Proxy\n>   *\n>   * Isolate IPA into separate process.\n> - *\n> - * Every subclass of proxy shall be registered with libcamera using\n> - * the REGISTER_IPA_PROXY() macro.\n>   */\n>  \n>  /**\n>   * \\brief Construct an IPAProxy instance\n>   * \\param[in] ipam The IPA module\n> - *\n> - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()\n> - * method implemented by the respective factories.\n>   */\n>  IPAProxy::IPAProxy(IPAModule *ipam)\n>  \t: valid_(false), ipam_(ipam)\n> @@ -219,91 +213,4 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n>   * construction.\n>   */\n>  \n> -/**\n> - * \\class IPAProxyFactory\n> - * \\brief Registration of IPAProxy classes and creation of instances\n> - *\n> - * To facilitate discovery and instantiation of IPAProxy classes, the\n> - * IPAProxyFactory class maintains a registry of IPAProxy classes. Each\n> - * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY()\n> - * macro, which will create a corresponding instance of a IPAProxyFactory\n> - * subclass and register it with the static list of factories.\n> - */\n> -\n> -/**\n> - * \\brief Construct a IPAProxy factory\n> - * \\param[in] name Name of the IPAProxy class\n> - *\n> - * Creating an instance of the factory registers is with the global list of\n> - * factories, accessible through the factories() function.\n> - *\n> - * The factory \\a name is used for debugging and IPAProxy matching purposes\n> - * and shall be unique.\n> - */\n> -IPAProxyFactory::IPAProxyFactory(const char *name)\n> -\t: name_(name)\n> -{\n> -\tregisterType(this);\n> -}\n> -\n> -/**\n> - * \\fn IPAProxyFactory::create()\n> - * \\brief Create an instance of the IPAProxy corresponding to the factory\n> - * \\param[in] ipam The IPA module\n> - * \\param[in] isolate Flag to isolate the IPA module\n> - *\n> - * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.\n> - * It creates a IPAProxy instance that isolates an IPA interface designated\n> - * by the IPA module \\a ipam. If \\a isolate is true, then the IPA module will\n> - * be isolated.\n> - *\n> - * \\return A pointer to a newly constructed instance of the IPAProxy subclass\n> - * corresponding to the factory\n> - */\n> -\n> -/**\n> - * \\fn IPAProxyFactory::name()\n> - * \\brief Retrieve the factory name\n> - * \\return The factory name\n> - */\n> -\n> -/**\n> - * \\brief Add a IPAProxy class to the registry\n> - * \\param[in] factory Factory to use to construct the IPAProxy\n> - *\n> - * The caller is responsible to guarantee the uniqueness of the IPAProxy name.\n> - */\n> -void IPAProxyFactory::registerType(IPAProxyFactory *factory)\n> -{\n> -\tstd::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();\n> -\n> -\tfactories.push_back(factory);\n> -\n> -\tLOG(IPAProxy, Debug)\n> -\t\t<< \"Registered proxy \\\"\" << factory->name() << \"\\\"\";\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the list of all IPAProxy factories\n> - *\n> - * The static factories map is defined inside the function to ensure it gets\n> - * initialized on first use, without any dependency on link order.\n> - *\n> - * \\return The list of pipeline handler factories\n> - */\n> -std::vector<IPAProxyFactory *> &IPAProxyFactory::factories()\n> -{\n> -\tstatic std::vector<IPAProxyFactory *> factories;\n> -\treturn factories;\n> -}\n> -\n> -/**\n> - * \\def REGISTER_IPA_PROXY\n> - * \\brief Register a IPAProxy with the IPAProxy factory\n> - * \\param[in] proxy Class name of IPAProxy derived class to register\n> - *\n> - * Register a proxy subclass with the factory and make it available to\n> - * isolate IPA modules.\n> - */\n> -\n>  } /* namespace libcamera */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BB786BE176\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 26 Nov 2020 15:46:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 368FF6346B;\n\tThu, 26 Nov 2020 16:46:22 +0100 (CET)","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 7C6426344B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 26 Nov 2020 16:46:21 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DEEEFA1B;\n\tThu, 26 Nov 2020 16:46:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rqyvu8nH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606405581;\n\tbh=jsow027kdEpqdUaIbkTZAFh6tRTQzlrvx/a4C1Z7a7w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rqyvu8nHxwBDspt4L0tO/n+n5Ax0Z8xDWJftKShr3z4GfO2xbaUzqT8CKFeiXQ0eK\n\tCJAjsj+cyzSqJB96cV0gfN1jSqhBq0n/7DO2oiRwcQJ7cVgvW6WA2tKBf426m2AH8e\n\t6BBHAUHRP6htrUi2AsCxITWt3clk0UbxQrZd30VE=","Date":"Thu, 26 Nov 2020 17:46:12 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20201126154612.GR3905@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-30-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-30-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 29/37] libcamera: IPAProxy: Remove\n\tregistration mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]