[libcamera-devel] [RFC PATCH v2 3/7] libcamera: add IPA proxy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 3 23:26:43 CEST 2019
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 at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list