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

1591 diff mbox series

Add a Proxy class whose implementations will act as a proxy between a
pipeline handler and an isolated IPA interface. Also add a ProxyFactory
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>
---
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 |  68 ++++++++++
 src/libcamera/ipa_proxy.cpp       | 219 ++++++++++++++++++++++++++++++
 src/libcamera/meson.build         |   6 +
 test/libtest/test.cpp             |   4 +
 5 files changed, 299 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 03, 2019 at 05:00:03PM +0900, Paul Elder wrote:
> Add a Proxy class whose implementations will act as a proxy between a

I think the class should be called IPAProxy.

> pipeline handler and an isolated IPA interface. Also add a ProxyFactory

And this one 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>
> ---
> 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 |  68 ++++++++++
>  src/libcamera/ipa_proxy.cpp       | 219 ++++++++++++++++++++++++++++++
>  src/libcamera/meson.build         |   6 +
>  test/libtest/test.cpp             |   4 +
>  5 files changed, 299 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 9ca3224..8c24f94 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..b493c1a
> --- /dev/null
> +++ b/src/libcamera/include/ipa_proxy.h
> @@ -0,0 +1,68 @@
> +/* 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 <vector>

You 

> +
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "ipa_module.h"
> +#include "utils.h"
> +
> +namespace libcamera {
> +
> +class Proxy : public IPAInterface
> +{
> +public:
> +	Proxy(IPAModule *ipam);
> +	virtual ~Proxy();

No need to declare the destructor as virtual, as it is already virtual
in the base class.

> +
> +	virtual int init() = 0;

You don't need this either for the same reason.

> +
> +	bool isValid() const;
> +
> +protected:
> +	const std::string resolvePath(const std::string &file) const;

No need to make the returned string const as you return it by value.

> +
> +	std::unique_ptr<IPAInterface> ipa_;

This is never used. And I would be surprised if it was, as the whole
point of the proxy is to access the IPA interface of the module through
IPC, so it shouldn't have a direct pointer to the IPAInterface.

> +
> +	bool valid_;
> +};
> +
> +class ProxyFactory
> +{
> +public:
> +	ProxyFactory(const char *name);
> +	virtual ~ProxyFactory() { };
> +
> +	virtual std::unique_ptr<Proxy> create(IPAModule *ipam) = 0;
> +
> +	const std::string &name() const { return name_; }
> +
> +	static void registerType(ProxyFactory *factory);
> +	static std::vector<ProxyFactory *> &factories();
> +
> +private:
> +	std::string name_;
> +};
> +
> +#define REGISTER_IPA_PROXY(proxy)			\
> +class proxy##Factory final : public ProxyFactory	\
> +{							\
> +public:							\
> +	proxy##Factory() : ProxyFactory(#proxy) {}	\
> +	std::unique_ptr<Proxy> 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..8bb3622
> --- /dev/null
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -0,0 +1,219 @@
> +/* 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>

iostream can be moved just above string.h. And do you actually need it ?

> +
> +/**
> + * \file ipa_proxy.h
> + * \brief IPA Proxy
> + *
> + * TODO write this

You know what I will say...

> + *
> + * Isolate IPA into separate process.
> + *
> + * Every subclass of proxy shall be registered with libcamera using
> + * the REGISTER_IPA_PROXY() macro.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Proxy)
> +
> +/**
> + * \class Proxy
> + * \brief IPA Proxy
> + *
> + * TODO write this

... so I won't repeat it :-)

> + *
> + * Isolate IPA into separate process.
> + */
> +
> +/**
> + * \brief Construct a Proxy instance
> + * \param[in] ipam The IPA module to wrap around
> + *
> + * Proxy instances shall be constructed through the ProxyFactory::create()
> + * method implemented by the respective factories.
> + */
> +Proxy::Proxy(IPAModule *ipam)

Why does the base constructor take a parameter if it doesn't use it ? If
you expect all IPA proxies to store the module pointer inside you could
add a protected ipam_ member to the base proxy class and initialise it
here. Otherwise I would just remove the parameter.

> +	: valid_(false)
> +{
> +}
> +
> +Proxy::~Proxy()
> +{
> +}
> +
> +/**
> + * \brief Check if the Proxy instance is valid
> + *
> + * A Proxy instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * \return true if the the Proxy is valid, false otherwise

https://lists.libcamera.org/pipermail/libcamera-devel/2019-July/003769.html

> + */
> +bool Proxy::isValid() const
> +{
> +	return valid_;
> +}

You could make this an inline function in the class definition as it
just returns a member variable.

> +
> +/**
> + * \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 must be have exec permission.

s/must be/shall/

> + *
> + * \return full path to proxy worker executable, or empty string if no valid

s/full path to proxy/The full path to the proxy/
s/or empty string/or an empty string/

> + * executable path
> + */
> +const std::string Proxy::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*/

s/variable/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);

Should you verify that proxyPath starts with a / ?

> +			proxyPath += proxyFile;
> +			if (!access(proxyPath.c_str(), X_OK))
> +				return proxyPath;
> +		}
> +
> +		if (*delim == '\0')
> +			break;
> +
> +		execPaths += count + 1;
> +	}
> +
> +	return std::string();
> +}
> +
> +/**
> + * \var Proxy::ipa_
> + * \brief The IPA interface that the Proxy is isolating
> + */
> +
> +/**
> + * \var Proxy::valid_
> + * \brief Flag to indicate if the Proxy instance is valid
> + *
> + * A Proxy instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * This flag can be read via Proxy::isValid()
> + *
> + * Implementations of the Proxy class should set this flag upon successful
> + * construction.
> + */
> +
> +/**
> + * \class ProxyFactory
> + * \brief Registration of Proxy classes and creation of instances
> + *
> + * To facilitate discovery and instantiation of Proxy classes, the
> + * ProxyFactory class maintains a registry of Proxy classes. Each
> + * Proxy subclass shall register itself using the REGISTER_IPA_PROXY()
> + * macro, which will create a corresponding instance of a ProxyFactory
> + * subclass and register it with the static list of factories.
> + */
> +
> +/**
> + * \brief Construct a Proxy factory
> + * \param[in] name Name of the Proxy 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 Proxy matching purposes
> + * and shall be unique.
> + */
> +ProxyFactory::ProxyFactory(const char *name)
> +	: name_(name)
> +{
> +	registerType(this);
> +}
> +
> +/**
> + * \fn ProxyFactory::create()
> + * \brief Create an instance of the Proxy corresponding to the factory
> + * \param[in] ipam The IPA module
> + *
> + * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.
> + * It creates a Proxy instance that isolates an IPA interface designated
> + * by the IPA module \a ipam.
> + *
> + * \return a pointer to a newly constructed instance of the Proxy subclass
> + * corresponding to the factory
> + */
> +
> +/**
> + * \fn ProxyFactory::name()
> + * \brief Retrieve the factory name
> + * \return The factory name
> + */
> +
> +/**
> + * \brief Add a Proxy class to the registry
> + * \param[in] factory Factory to use to construct the Proxy
> + *
> + * The caller is responsible to guarantee the uniqueness of the Proxy name.
> + */
> +void ProxyFactory::registerType(ProxyFactory *factory)
> +{
> +	std::vector<ProxyFactory *> &factories = ProxyFactory::factories();
> +
> +	factories.push_back(factory);
> +
> +	LOG(Proxy, Debug)
> +		<< "Registered proxy \"" << factory->name() << "\"";
> +}
> +
> +/**
> + * \brief Retrieve the list of all Proxy factories
> + *
> + * The static factories map is defined inside the function to ensures it gets

s/ensures/ensure/

(feel free to fix the typo in pipeline_handler.cpp)

> + * initialized on first use, without any dependency on link order.
> + *
> + * \return the list of pipeline handler factories
> + */
> +std::vector<ProxyFactory *> &ProxyFactory::factories()
> +{
> +	static std::vector<ProxyFactory *> factories;
> +	return factories;
> +}
> +
> +/**
> + * \def REGISTER_IPA_PROXY
> + * \brief Register a Proxy with the Proxy factory
> + * \param[in] proxy Class name of Proxy 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 087b578..412564f 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',
> @@ -64,6 +66,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) + '"')
> +

How about moving this to src/libcamera/ipa/ to group it with the related
IPA_MODULE_DIR ?

>  libudev = dependency('libudev', required : false)
>  
>  if libudev.found()
> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> index b119cf1..35aa021 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;