[libcamera-devel] [RFC PATCH v2 5/7] libcamera: ipa_manager: use proxy

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jul 03, 2019 at 05:00:05PM +0900, Paul Elder wrote:
> Make IPAManager isolate an IPA in a Proxy if the IPA's license is not
> open source, before returning the IPA to the caller. For now, only use
> the default Linux proxy, and only GPL and LGPL are considered open
> source.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> New in v2
> - replaces adding shims
> - since Proxies are not external shared objects like the shims in v1
>   were, there is no longer a list of shims that is treated like
>   IPAModules
> - instead the matching is done by searching the list of proxy factories
> 
>  src/libcamera/ipa_manager.cpp | 48 +++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 532b77d..60cd84d 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -14,6 +14,7 @@
>  #include "ipa_module.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
> +#include "ipa_proxy.h"

Alphabetical order please.

>  #include "utils.h"
>  
>  /**
> @@ -25,6 +26,20 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAManager)
>  
> +namespace {
> +
> +bool isOpenSource(const char *license)
> +{
> +	if (!strncmp(license, "GPL", 3))
> +		return true;
> +	if (!strncmp(license, "LGPL", 4))
> +		return true;
> +
> +	return false;
> +}

You could add this method to the IPAModule class. It would then have to
be documented, which would ensure the documentation is explicit about
what licenses are considered open-source.

As you only accept GPL licenses for now, should the method be named
isGPL() ?

> +
> +} /* namespace */
> +
>  /**
>   * \class IPAManager
>   * \brief Manager for IPA modules
> @@ -129,7 +144,7 @@ int IPAManager::addDir(const char *libDir)
>   * \param[in] maxVersion Maximum acceptable version of IPA module
>   *
>   * \return A newly created IPA interface, or nullptr if no matching
> - * IPA module is found
> + * IPA module is found or if the IPA interface fails to initialize

"initialize" seems to imply the init() method, which isn't called here.
How about "if no matching IPA module is found or if the module interface
can't be accessed" ? I'm still not very happy with that sentence, so if
you have a better idea please propose it.

>   */
>  std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  						    uint32_t maxVersion,
> @@ -144,7 +159,36 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  		}
>  	}
>  
> -	if (!m || !m->load())
> +	if (!m)
> +		return nullptr;
> +
> +	if (!isOpenSource(m->info().license)) {
> +		ProxyFactory *pf = nullptr;
> +		std::vector<ProxyFactory *> &factories = ProxyFactory::factories();
> +
> +		for (ProxyFactory *factory : factories) {

You can write

		for (ProxyFactor *factory : ProxyFactory::factories()) {

> +			/* TODO: Better matching */

			/**
			 * \todo Match proxies with modules through a
			 * configuration file.
			 */

> +			if (!strcmp(factory->name().c_str(), "ProxyLinuxDefault")) {

			if (factory->name() == "ProxyLinuxDefault") {

> +				pf = factory;
> +				break;
> +			}
> +		}
> +
> +		if (!pf) {
> +			LOG(IPAManager, Error) << "Failed to get proxy factory";

"Failed to find IPA proxy factory for module " << m->libPath();

Which requires exposing a libPath() method to the IPAModule class, but I
think you will need that anyway as the configuration file will likely
associate a library path with a proxy (we can't trust the name reported
by the module itself, or any information it contains for that matter, to
decide which proxy to use).)

> +			return nullptr;
> +		}
> +
> +		std::unique_ptr<Proxy> proxy = pf->create(m);
> +		if (!proxy->isValid()) {
> +			LOG(IPAManager, Error) << "Failed to load proxy";

"Failed to create IPA proxy for module " << m->libPath();

> +			return nullptr;
> +		}
> +
> +		return proxy;
> +	}
> +
> +	if (!m->load())
>  		return nullptr;
>  
>  	return m->createInstance();

-- 
Regards,

Laurent Pinchart