[libcamera-devel,v4,29/37] libcamera: IPAProxy: Remove registration mechanism

10377 diff mbox series

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>

---
No change in v4

No change in v3

Changes in v2:
- remove documentation
---
 include/libcamera/internal/ipa_proxy.h | 29 --------
 src/libcamera/ipa_proxy.cpp            | 93 --------------------------
 2 files changed, 122 deletions(-)

Hi Paul,

On Fri, Nov 06, 2020 at 07:36:59PM +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>
>

Very minor, but since the factory is now gone, I wonder if the typedef
in ipa_module.h

        typedef IPAInterface *(*IPAIntfFactory)(void);

Should not be changed.

Apart from this
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> No change in v4
>
> No change in v3
>
> Changes in v2:
> - remove documentation
> ---
>  include/libcamera/internal/ipa_proxy.h | 29 --------
>  src/libcamera/ipa_proxy.cpp            | 93 --------------------------
>  2 files changed, 122 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 195d2cb4..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() = default;
> -
> -	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 ee5e6f3e..29c0e9e0 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)
>   * \brief IPA Proxy
>   *
>   * Isolate IPA into separate process.
> - *
> - * Every subclass of proxy shall be registered with libcamera using
> - * the REGISTER_IPA_PROXY() macro.
>   */
>
>  /**
>   * \brief Construct an IPAProxy instance
>   * \param[in] ipam The IPA module
> - *
> - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()
> - * method implemented by the respective factories.
>   */
>  IPAProxy::IPAProxy(IPAModule *ipam)
>  	: valid_(false), ipam_(ipam)
> @@ -219,91 +213,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
> - * \param[in] isolate Flag to isolate 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. If \a isolate is true, then the IPA module will
> - * be isolated.
> - *
> - * \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

Hi Jacopo,

On Wed, Nov 18, 2020 at 02:20:59PM +0100, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Nov 06, 2020 at 07:36:59PM +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>
> >
> 
> Very minor, but since the factory is now gone, I wonder if the typedef
> in ipa_module.h
> 
>         typedef IPAInterface *(*IPAIntfFactory)(void);

This is different. This refers to the function that's loaded from the
IPA .so that constructs the IPAInterface:

IPAInterface *ipaCreate()
{
	return new IPARPi();
}

I think it's fair to call it an IPAIntfFactory. At most it could be
renamed to IPAIntfConstructor, which I don't think is any better.

> Should not be changed.
> 
> Apart from this
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Thanks,

Paul

> > ---
> > No change in v4
> >
> > No change in v3
> >
> > Changes in v2:
> > - remove documentation
> > ---
> >  include/libcamera/internal/ipa_proxy.h | 29 --------
> >  src/libcamera/ipa_proxy.cpp            | 93 --------------------------
> >  2 files changed, 122 deletions(-)
> >
> > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> > index 195d2cb4..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() = default;
> > -
> > -	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 ee5e6f3e..29c0e9e0 100644
> > --- a/src/libcamera/ipa_proxy.cpp
> > +++ b/src/libcamera/ipa_proxy.cpp
> > @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)
> >   * \brief IPA Proxy
> >   *
> >   * Isolate IPA into separate process.
> > - *
> > - * Every subclass of proxy shall be registered with libcamera using
> > - * the REGISTER_IPA_PROXY() macro.
> >   */
> >
> >  /**
> >   * \brief Construct an IPAProxy instance
> >   * \param[in] ipam The IPA module
> > - *
> > - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()
> > - * method implemented by the respective factories.
> >   */
> >  IPAProxy::IPAProxy(IPAModule *ipam)
> >  	: valid_(false), ipam_(ipam)
> > @@ -219,91 +213,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
> > - * \param[in] isolate Flag to isolate 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. If \a isolate is true, then the IPA module will
> > - * be isolated.
> > - *
> > - * \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

Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:59PM +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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> No change in v4
> 
> No change in v3
> 
> Changes in v2:
> - remove documentation
> ---
>  include/libcamera/internal/ipa_proxy.h | 29 --------
>  src/libcamera/ipa_proxy.cpp            | 93 --------------------------
>  2 files changed, 122 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 195d2cb4..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() = default;
> -
> -	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 ee5e6f3e..29c0e9e0 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -30,17 +30,11 @@ LOG_DEFINE_CATEGORY(IPAProxy)
>   * \brief IPA Proxy
>   *
>   * Isolate IPA into separate process.
> - *
> - * Every subclass of proxy shall be registered with libcamera using
> - * the REGISTER_IPA_PROXY() macro.
>   */
>  
>  /**
>   * \brief Construct an IPAProxy instance
>   * \param[in] ipam The IPA module
> - *
> - * IPAProxy instances shall be constructed through the IPAProxyFactory::create()
> - * method implemented by the respective factories.
>   */
>  IPAProxy::IPAProxy(IPAModule *ipam)
>  	: valid_(false), ipam_(ipam)
> @@ -219,91 +213,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
> - * \param[in] isolate Flag to isolate 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. If \a isolate is true, then the IPA module will
> - * be isolated.
> - *
> - * \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 */