[libcamera-devel] [PATCH 21/23] libcamera: IPAManager: Make createIPA return proxy directly

[Niklas Söderlund]

Hi Paul,

Thanks for your work.

On 2020-09-15 23:20:36 +0900, Paul Elder wrote:
> Since every pipeline knows the type of the proxy that it needs, and
> since all IPAs are to be wrapped in a proxy, IPAManager no longer needs
> to search in the factory list to fetch the proxy factory to construct a
> factory. Instead, we define createIPA as a template function, and the
> pipeline handler can declare the proxy type when it calls createIPA.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  include/libcamera/internal/ipa_manager.h      | 31 ++++++++++--
>  src/libcamera/ipa_manager.cpp                 | 48 +------------------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  3 +-
>  3 files changed, 30 insertions(+), 52 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 4a143b6a..297a8a58 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -14,20 +14,45 @@
>  #include <libcamera/ipa/ipa_module_info.h>
>  
>  #include "libcamera/internal/ipa_module.h"
> +#include "libcamera/internal/log.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/pub_key.h"
>  
>  namespace libcamera {
>  
> +LOG_DECLARE_CATEGORY(IPAManager)
> +
>  class IPAManager
>  {
>  public:
>  	IPAManager();
>  	~IPAManager();
>  
> -	static std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> -						   uint32_t maxVersion,
> -						   uint32_t minVersion);
> +	template<class P>
> +	static std::unique_ptr<P> createIPA(PipelineHandler *pipe,
> +					    uint32_t maxVersion,
> +					    uint32_t minVersion)
> +	{
> +		IPAModule *m = nullptr;
> +
> +		for (IPAModule *module : self_->modules_) {
> +			if (module->match(pipe, minVersion, maxVersion)) {
> +				m = module;
> +				break;
> +			}
> +		}
> +
> +		if (!m)
> +			return nullptr;
> +
> +		std::unique_ptr<P> proxy = std::make_unique<P>(m, !self_->isSignatureValid(m));
> +		if (!proxy->isValid()) {
> +			LOG(IPAManager, Error) << "Failed to load proxy";
> +			return nullptr;
> +		}
> +
> +		return proxy;
> +	}
>  
>  private:
>  	static IPAManager *self_;
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 26458153..0d518443 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -245,6 +245,7 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>  }
>  
>  /**
> + * \fn IPAManager::createIPA()
>   * \brief Create an IPA proxy that matches a given pipeline handler
>   * \param[in] pipe The pipeline handler that wants a matching IPA proxy
>   * \param[in] minVersion Minimum acceptable version of IPA module
> @@ -253,53 +254,6 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>   * \return A newly created IPA proxy, or nullptr if no matching IPA module is
>   * found or if the IPA proxy fails to initialize
>   */
> -std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
> -						uint32_t maxVersion,
> -						uint32_t minVersion)
> -{
> -	IPAModule *m = nullptr;
> -
> -	for (IPAModule *module : self_->modules_) {
> -		if (module->match(pipe, minVersion, maxVersion)) {
> -			m = module;
> -			break;
> -		}
> -	}
> -
> -	if (!m)
> -		return nullptr;
> -
> -	/*
> -	 * Load and run the IPA module in a thread if it has a valid signature,
> -	 * or isolate it in a separate process otherwise.
> -	 *
> -	 * \todo Implement a better proxy selection
> -	 */
> -	std::string pipeName(pipe->name());
> -	const char *proxyName = pipeName.replace(0, 15, "IPAProxy").c_str();
> -	IPAProxyFactory *pf = nullptr;
> -
> -	for (IPAProxyFactory *factory : IPAProxyFactory::factories()) {
> -		if (!strcmp(factory->name().c_str(), proxyName)) {
> -			pf = factory;
> -			break;
> -		}
> -	}
> -
> -	if (!pf) {
> -		LOG(IPAManager, Error) << "Failed to get proxy factory";
> -		return nullptr;
> -	}
> -
> -	std::unique_ptr<IPAProxy> proxy =
> -		pf->create(m, !self_->isSignatureValid(m));
> -	if (!proxy->isValid()) {
> -		LOG(IPAManager, Error) << "Failed to load proxy";
> -		return nullptr;
> -	}
> -
> -	return proxy;
> -}
>  
>  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>  {
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 70bb6fcc..19755e8c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1108,8 +1108,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)
>  
>  int RPiCameraData::loadIPA()
>  {
> -	std::unique_ptr<IPAProxy> ptr = IPAManager::createIPA(pipe_, 1, 1);
> -	ipa_ = std::unique_ptr<IPAProxyRPi>{static_cast<IPAProxyRPi*>(std::move(ptr).release())};
> +	ipa_ = IPAManager::createIPA<IPAProxyRPi>(pipe_, 1, 1);
>  
>  	if (!ipa_)
>  		return -ENOENT;
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund