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

1593 diff mbox series

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@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(-)

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@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();