Message ID | 20200915142038.28757-20-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, Thanks for your work. On 2020-09-15 23:20:34 +0900, Paul Elder wrote: > Implementations of IPA proxies use a registration mechanism to register > themselves with the main IPA proxy factory. This registration declares > static objects, causing a risk of things being constructed before the > proper libcamera facilities are ready. Since each pipeline handler has > its own IPA proxy and knows the type, it isn't necessary to have a proxy > factory. Remove it to alleviate the risk of early construction. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/internal/ipa_proxy.h | 29 --------- > src/libcamera/ipa_proxy.cpp | 85 -------------------------- > 2 files changed, 114 deletions(-) > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > index 1903150e..f651a3ae 100644 > --- a/include/libcamera/internal/ipa_proxy.h > +++ b/include/libcamera/internal/ipa_proxy.h > @@ -36,35 +36,6 @@ private: > IPAModule *ipam_; > }; > > -class IPAProxyFactory > -{ > -public: > - IPAProxyFactory(const char *name); > - virtual ~IPAProxyFactory() {} > - > - virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0; > - > - const std::string &name() const { return name_; } > - > - static void registerType(IPAProxyFactory *factory); > - static std::vector<IPAProxyFactory *> &factories(); > - > -private: > - std::string name_; > -}; > - > -#define REGISTER_IPA_PROXY(proxy) \ > -class proxy##Factory final : public IPAProxyFactory \ > -{ \ > -public: \ > - proxy##Factory() : IPAProxyFactory(#proxy) {} \ > - std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) \ > - { \ > - return std::make_unique<proxy>(ipam, isolate); \ > - } \ > -}; \ > -static proxy##Factory global_##proxy##Factory; > - > } /* namespace libcamera */ > > #endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_H__ */ > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index ff4d7fd1..e7d4db06 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -229,89 +229,4 @@ std::string IPAProxy::resolvePath(const std::string &file) const > * construction. > */ > > -/** > - * \class IPAProxyFactory > - * \brief Registration of IPAProxy classes and creation of instances > - * > - * To facilitate discovery and instantiation of IPAProxy classes, the > - * IPAProxyFactory class maintains a registry of IPAProxy classes. Each > - * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY() > - * macro, which will create a corresponding instance of a IPAProxyFactory > - * subclass and register it with the static list of factories. > - */ > - > -/** > - * \brief Construct a IPAProxy factory > - * \param[in] name Name of the IPAProxy class > - * > - * Creating an instance of the factory registers is with the global list of > - * factories, accessible through the factories() function. > - * > - * The factory \a name is used for debugging and IPAProxy matching purposes > - * and shall be unique. > - */ > -IPAProxyFactory::IPAProxyFactory(const char *name) > - : name_(name) > -{ > - registerType(this); > -} > - > -/** > - * \fn IPAProxyFactory::create() > - * \brief Create an instance of the IPAProxy corresponding to the factory > - * \param[in] ipam The IPA module > - * > - * This virtual function is implemented by the REGISTER_IPA_PROXY() macro. > - * It creates a IPAProxy instance that isolates an IPA interface designated > - * by the IPA module \a ipam. > - * > - * \return A pointer to a newly constructed instance of the IPAProxy subclass > - * corresponding to the factory > - */ > - > -/** > - * \fn IPAProxyFactory::name() > - * \brief Retrieve the factory name > - * \return The factory name > - */ > - > -/** > - * \brief Add a IPAProxy class to the registry > - * \param[in] factory Factory to use to construct the IPAProxy > - * > - * The caller is responsible to guarantee the uniqueness of the IPAProxy name. > - */ > -void IPAProxyFactory::registerType(IPAProxyFactory *factory) > -{ > - std::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories(); > - > - factories.push_back(factory); > - > - LOG(IPAProxy, Debug) > - << "Registered proxy \"" << factory->name() << "\""; > -} > - > -/** > - * \brief Retrieve the list of all IPAProxy factories > - * > - * The static factories map is defined inside the function to ensure it gets > - * initialized on first use, without any dependency on link order. > - * > - * \return The list of pipeline handler factories > - */ > -std::vector<IPAProxyFactory *> &IPAProxyFactory::factories() > -{ > - static std::vector<IPAProxyFactory *> factories; > - return factories; > -} > - > -/** > - * \def REGISTER_IPA_PROXY > - * \brief Register a IPAProxy with the IPAProxy factory > - * \param[in] proxy Class name of IPAProxy derived class to register > - * > - * Register a proxy subclass with the factory and make it available to > - * isolate IPA modules. > - */ > - > } /* namespace libcamera */ > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h index 1903150e..f651a3ae 100644 --- a/include/libcamera/internal/ipa_proxy.h +++ b/include/libcamera/internal/ipa_proxy.h @@ -36,35 +36,6 @@ private: IPAModule *ipam_; }; -class IPAProxyFactory -{ -public: - IPAProxyFactory(const char *name); - virtual ~IPAProxyFactory() {} - - virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0; - - const std::string &name() const { return name_; } - - static void registerType(IPAProxyFactory *factory); - static std::vector<IPAProxyFactory *> &factories(); - -private: - std::string name_; -}; - -#define REGISTER_IPA_PROXY(proxy) \ -class proxy##Factory final : public IPAProxyFactory \ -{ \ -public: \ - proxy##Factory() : IPAProxyFactory(#proxy) {} \ - std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) \ - { \ - return std::make_unique<proxy>(ipam, isolate); \ - } \ -}; \ -static proxy##Factory global_##proxy##Factory; - } /* namespace libcamera */ #endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_H__ */ diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index ff4d7fd1..e7d4db06 100644 --- a/src/libcamera/ipa_proxy.cpp +++ b/src/libcamera/ipa_proxy.cpp @@ -229,89 +229,4 @@ std::string IPAProxy::resolvePath(const std::string &file) const * construction. */ -/** - * \class IPAProxyFactory - * \brief Registration of IPAProxy classes and creation of instances - * - * To facilitate discovery and instantiation of IPAProxy classes, the - * IPAProxyFactory class maintains a registry of IPAProxy classes. Each - * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY() - * macro, which will create a corresponding instance of a IPAProxyFactory - * subclass and register it with the static list of factories. - */ - -/** - * \brief Construct a IPAProxy factory - * \param[in] name Name of the IPAProxy class - * - * Creating an instance of the factory registers is with the global list of - * factories, accessible through the factories() function. - * - * The factory \a name is used for debugging and IPAProxy matching purposes - * and shall be unique. - */ -IPAProxyFactory::IPAProxyFactory(const char *name) - : name_(name) -{ - registerType(this); -} - -/** - * \fn IPAProxyFactory::create() - * \brief Create an instance of the IPAProxy corresponding to the factory - * \param[in] ipam The IPA module - * - * This virtual function is implemented by the REGISTER_IPA_PROXY() macro. - * It creates a IPAProxy instance that isolates an IPA interface designated - * by the IPA module \a ipam. - * - * \return A pointer to a newly constructed instance of the IPAProxy subclass - * corresponding to the factory - */ - -/** - * \fn IPAProxyFactory::name() - * \brief Retrieve the factory name - * \return The factory name - */ - -/** - * \brief Add a IPAProxy class to the registry - * \param[in] factory Factory to use to construct the IPAProxy - * - * The caller is responsible to guarantee the uniqueness of the IPAProxy name. - */ -void IPAProxyFactory::registerType(IPAProxyFactory *factory) -{ - std::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories(); - - factories.push_back(factory); - - LOG(IPAProxy, Debug) - << "Registered proxy \"" << factory->name() << "\""; -} - -/** - * \brief Retrieve the list of all IPAProxy factories - * - * The static factories map is defined inside the function to ensure it gets - * initialized on first use, without any dependency on link order. - * - * \return The list of pipeline handler factories - */ -std::vector<IPAProxyFactory *> &IPAProxyFactory::factories() -{ - static std::vector<IPAProxyFactory *> factories; - return factories; -} - -/** - * \def REGISTER_IPA_PROXY - * \brief Register a IPAProxy with the IPAProxy factory - * \param[in] proxy Class name of IPAProxy derived class to register - * - * Register a proxy subclass with the factory and make it available to - * isolate IPA modules. - */ - } /* namespace libcamera */
Implementations of IPA proxies use a registration mechanism to register themselves with the main IPA proxy factory. This registration declares static objects, causing a risk of things being constructed before the proper libcamera facilities are ready. Since each pipeline handler has its own IPA proxy and knows the type, it isn't necessary to have a proxy factory. Remove it to alleviate the risk of early construction. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- include/libcamera/internal/ipa_proxy.h | 29 --------- src/libcamera/ipa_proxy.cpp | 85 -------------------------- 2 files changed, 114 deletions(-)