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

1636 diff mbox series

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.

Update Doxygen to ignore the directory where Proxy implementations will
reside.

Signed-off-by: Paul Elder <paul.elder@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

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