[libcamera-devel] [PATCH v3 3/7] libcamera: add IPA proxy

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jul 10, 2019 at 03:44:46AM +0900, Paul Elder wrote:
> Add an IPAProxy class whose implementations will act as a proxy between a
> pipeline handler and an isolated IPA interface. Also add an IPAProxyFactory
> that will construct the Proxy implementations as necessary.

s/Proxy/IPAProxy/

> 
> Update Doxygen to ignore the directory where Proxy implementations will

Ditto.

> reside.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v3:
> - renamed Proxy and ProxyFactory to IPAProxy and IPAProxyFactory
> - moved proxy workers to proxy/worker/ (from proxy_worker/)
> 
> New in v2
> - replaces shims from v1
> - build into libcamera, hence the register macro (similar to what
>   PipelineHandler uses)
> - no longer builds separate .so
> 
>  Documentation/Doxyfile.in         |   3 +-
>  src/libcamera/include/ipa_proxy.h |  66 ++++++++++
>  src/libcamera/ipa_proxy.cpp       | 204 ++++++++++++++++++++++++++++++
>  src/libcamera/meson.build         |   6 +
>  test/libtest/test.cpp             |   4 +
>  5 files changed, 282 insertions(+), 1 deletion(-)
>  create mode 100644 src/libcamera/include/ipa_proxy.h
>  create mode 100644 src/libcamera/ipa_proxy.cpp
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index cad85ff..3d94623 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -837,7 +837,8 @@ EXCLUDE                = @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp
>  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>  			 @TOP_SRCDIR@/src/libcamera/include/device_enumerator_sysfs.h \
>  			 @TOP_SRCDIR@/src/libcamera/include/device_enumerator_udev.h \
> -			 @TOP_SRCDIR@/src/libcamera/pipeline/
> +			 @TOP_SRCDIR@/src/libcamera/pipeline/ \
> +			 @TOP_SRCDIR@/src/libcamera/proxy/
>  
>  # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
>  # directories that are symbolic links (a Unix file system feature) are excluded
> diff --git a/src/libcamera/include/ipa_proxy.h b/src/libcamera/include/ipa_proxy.h
> new file mode 100644
> index 0000000..c76d917
> --- /dev/null
> +++ b/src/libcamera/include/ipa_proxy.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_proxy.h - Image Processing Algorithm proxy
> + */
> +#ifndef __LIBCAMERA_IPA_PROXY_H__
> +#define __LIBCAMERA_IPA_PROXY_H__
> +
> +#include <memory>
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "ipa_module.h"
> +#include "utils.h"
> +
> +namespace libcamera {
> +
> +class IPAProxy : public IPAInterface
> +{
> +public:
> +	IPAProxy();
> +	~IPAProxy();
> +
> +	bool isValid() const { return valid_; }
> +
> +protected:
> +	std::string resolvePath(const std::string &file) const;
> +
> +	bool valid_;
> +};
> +
> +class IPAProxyFactory
> +{
> +public:
> +	IPAProxyFactory(const char *name);
> +	virtual ~IPAProxyFactory() { };
> +
> +	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam) = 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)	\
> +	{						\
> +		return utils::make_unique<proxy>(ipam);	\
> +	}						\
> +};							\
> +static proxy##Factory global_##proxy##Factory;
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_PROXY_H__ */
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> new file mode 100644
> index 0000000..1f6a8a0
> --- /dev/null
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -0,0 +1,204 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_proxy.cpp - Image Processing Algorithm proxy
> + */
> +
> +#include "ipa_proxy.h"
> +
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +#include <iostream>

Do you need iostream ? If so it an be moved just above string.h.

> +
> +/**
> + * \file ipa_proxy.h
> + * \brief IPA Proxy
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPAProxy)
> +
> +/**
> + * \class 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
> + *
> + * IPAProxy instances shall be constructed through the IPAProxyFactory::create()
> + * method implemented by the respective factories.
> + */
> +IPAProxy::IPAProxy()
> +	: valid_(false)
> +{
> +}
> +
> +IPAProxy::~IPAProxy()
> +{
> +}
> +
> +/**
> + * \fn IPAProxy::isValid()
> + * \brief Check if the IPAProxy instance is valid
> + *
> + * An IPAProxy instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * \return True if the IPAProxy is valid, false otherwise
> + */
> +
> +/**
> + * \brief Find a valid full path for a proxy worker for a given executable name
> + * \param[in] file File name of proxy worker executable
> + *
> + * A proxy worker's executable could be found in either the global installation
> + * directory, or in the paths specified by the environment variable
> + * LIBCAMERA_IPA_PROXY_PATH. This method checks the global install directory
> + * first, then LIBCAMERA_IPA_PROXY_PATH in order, and returns the full path to
> + * the proxy worker executable that is specified by file. The proxy worker
> + * executable shall have exec permission.
> + *
> + * \return The full path to the proxy worker executable, or empty string if

s/or/or an/

> + * no valid executable path
> + */
> +std::string IPAProxy::resolvePath(const std::string &file) const
> +{
> +	/* Try finding the exec target from the install directory first */
> +	std::string proxyFile = "/" + file;
> +	std::string proxyPath = std::string(IPA_PROXY_DIR) + proxyFile;
> +	if (!access(proxyPath.c_str(), X_OK))
> +		return proxyPath;
> +
> +	/* No exec target in install directory; check env variable. */
> +	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
> +	while (execPaths) {
> +		const char *delim = strchrnul(execPaths, ':');
> +		size_t count = delim - execPaths;
> +
> +		if (count) {
> +			std::string proxyPath(execPaths, count);
> +			proxyPath += proxyFile;
> +			if (!access(proxyPath.c_str(), X_OK))
> +				return proxyPath;
> +		}
> +
> +		if (*delim == '\0')
> +			break;
> +
> +		execPaths += count + 1;
> +	}
> +
> +	return std::string();
> +}
> +
> +/**
> + * \var IPAProxy::valid_
> + * \brief Flag to indicate if the IPAProxy instance is valid
> + *
> + * A IPAProxy instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * This flag can be read via IPAProxy::isValid()

s/$/./

With these small issues fixed,

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

> + *
> + * Implementations of the IPAProxy class should set this flag upon successful
> + * 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
> + *
> + * 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.
> + *
> + * \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 */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index a364f68..73a6fc7 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -14,6 +14,7 @@ libcamera_sources = files([
>      'ipa_interface.cpp',
>      'ipa_manager.cpp',
>      'ipa_module.cpp',
> +    'ipa_proxy.cpp',
>      'ipc_unixsocket.cpp',
>      'log.cpp',
>      'media_device.cpp',
> @@ -42,6 +43,7 @@ libcamera_headers = files([
>      'include/formats.h',
>      'include/ipa_manager.h',
>      'include/ipa_module.h',
> +    'include/ipa_proxy.h',
>      'include/ipc_unixsocket.h',
>      'include/log.h',
>      'include/media_device.h',
> @@ -63,6 +65,10 @@ includes = [
>  
>  subdir('pipeline')
>  
> +proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')
> +config_h.set('IPA_PROXY_DIR',
> +             '"' + join_paths(get_option('prefix'), proxy_install_dir) + '"')
> +
>  libudev = dependency('libudev', required : false)
>  
>  if libudev.found()
> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> index b119cf1..333d216 100644
> --- a/test/libtest/test.cpp
> +++ b/test/libtest/test.cpp
> @@ -25,6 +25,10 @@ int Test::execute()
>  	if (ret)
>  		return errno;
>  
> +	ret = setenv("LIBCAMERA_IPA_PROXY_PATH", "src/libcamera/proxy/worker", 1);
> +	if (ret)
> +		return errno;
> +
>  	ret = init();
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart