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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:53PM +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>
> 
> ---
> No change in v4
> 
> No change in v3
> 
> No change in v2
> ---
>  include/libcamera/internal/ipa_manager.h | 31 +++++++++++++--
>  src/libcamera/ipa_manager.cpp            | 48 +-----------------------
>  2 files changed, 29 insertions(+), 50 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>

s/class P/typename T/ is more customary.

> +	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;
> +			}
> +		}

Would it make sense to move this code to a private function
(findModule() ?), to avoid duplicating it for each pipeline ?

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

> +
> +		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 3a32573f..93d02d94 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
>  {

-- 
Regards,

Laurent Pinchart