[{"id":2214,"web_url":"https://patchwork.libcamera.org/comment/2214/","msgid":"<20190711054524.GE5247@pendragon.ideasonboard.com>","date":"2019-07-11T05:45:24","subject":"Re: [libcamera-devel] [PATCH v3 3/7] libcamera: add IPA proxy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jul 10, 2019 at 03:44:46AM +0900, Paul Elder wrote:\n> Add an IPAProxy class whose implementations will act as a proxy between a\n> pipeline handler and an isolated IPA interface. Also add an IPAProxyFactory\n> that will construct the Proxy implementations as necessary.\n\ns/Proxy/IPAProxy/\n\n> \n> Update Doxygen to ignore the directory where Proxy implementations will\n\nDitto.\n\n> reside.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> Changes in v3:\n> - renamed Proxy and ProxyFactory to IPAProxy and IPAProxyFactory\n> - moved proxy workers to proxy/worker/ (from proxy_worker/)\n> \n> New in v2\n> - replaces shims from v1\n> - build into libcamera, hence the register macro (similar to what\n>   PipelineHandler uses)\n> - no longer builds separate .so\n> \n>  Documentation/Doxyfile.in         |   3 +-\n>  src/libcamera/include/ipa_proxy.h |  66 ++++++++++\n>  src/libcamera/ipa_proxy.cpp       | 204 ++++++++++++++++++++++++++++++\n>  src/libcamera/meson.build         |   6 +\n>  test/libtest/test.cpp             |   4 +\n>  5 files changed, 282 insertions(+), 1 deletion(-)\n>  create mode 100644 src/libcamera/include/ipa_proxy.h\n>  create mode 100644 src/libcamera/ipa_proxy.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index cad85ff..3d94623 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -837,7 +837,8 @@ EXCLUDE                = @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/include/device_enumerator_sysfs.h \\\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/include/device_enumerator_udev.h \\\n> -\t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/\n> +\t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> +\t\t\t @TOP_SRCDIR@/src/libcamera/proxy/\n>  \n>  # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>  # directories that are symbolic links (a Unix file system feature) are excluded\n> diff --git a/src/libcamera/include/ipa_proxy.h b/src/libcamera/include/ipa_proxy.h\n> new file mode 100644\n> index 0000000..c76d917\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_proxy.h\n> @@ -0,0 +1,66 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_proxy.h - Image Processing Algorithm proxy\n> + */\n> +#ifndef __LIBCAMERA_IPA_PROXY_H__\n> +#define __LIBCAMERA_IPA_PROXY_H__\n> +\n> +#include <memory>\n> +#include <string>\n> +#include <vector>\n> +\n> +#include <libcamera/ipa/ipa_interface.h>\n> +\n> +#include \"ipa_module.h\"\n> +#include \"utils.h\"\n> +\n> +namespace libcamera {\n> +\n> +class IPAProxy : public IPAInterface\n> +{\n> +public:\n> +\tIPAProxy();\n> +\t~IPAProxy();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +protected:\n> +\tstd::string resolvePath(const std::string &file) const;\n> +\n> +\tbool valid_;\n> +};\n> +\n> +class IPAProxyFactory\n> +{\n> +public:\n> +\tIPAProxyFactory(const char *name);\n> +\tvirtual ~IPAProxyFactory() { };\n> +\n> +\tvirtual std::unique_ptr<IPAProxy> create(IPAModule *ipam) = 0;\n> +\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tstatic void registerType(IPAProxyFactory *factory);\n> +\tstatic std::vector<IPAProxyFactory *> &factories();\n> +\n> +private:\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_IPA_PROXY(proxy)\t\t\t\\\n> +class proxy##Factory final : public IPAProxyFactory\t\\\n> +{\t\t\t\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\\\n> +\tproxy##Factory() : IPAProxyFactory(#proxy) {}\t\\\n> +\tstd::unique_ptr<IPAProxy> create(IPAModule *ipam)\t\\\n> +\t{\t\t\t\t\t\t\\\n> +\t\treturn utils::make_unique<proxy>(ipam);\t\\\n> +\t}\t\t\t\t\t\t\\\n> +};\t\t\t\t\t\t\t\\\n> +static proxy##Factory global_##proxy##Factory;\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_PROXY_H__ */\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> new file mode 100644\n> index 0000000..1f6a8a0\n> --- /dev/null\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -0,0 +1,204 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_proxy.cpp - Image Processing Algorithm proxy\n> + */\n> +\n> +#include \"ipa_proxy.h\"\n> +\n> +#include <string.h>\n> +#include <unistd.h>\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +#include <iostream>\n\nDo you need iostream ? If so it an be moved just above string.h.\n\n> +\n> +/**\n> + * \\file ipa_proxy.h\n> + * \\brief IPA Proxy\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPAProxy)\n> +\n> +/**\n> + * \\class IPAProxy\n> + * \\brief IPA Proxy\n> + *\n> + * Isolate IPA into separate process.\n> + *\n> + * Every subclass of proxy shall be registered with libcamera using\n> + * the REGISTER_IPA_PROXY() macro.\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAProxy instance\n> + *\n> + * IPAProxy instances shall be constructed through the IPAProxyFactory::create()\n> + * method implemented by the respective factories.\n> + */\n> +IPAProxy::IPAProxy()\n> +\t: valid_(false)\n> +{\n> +}\n> +\n> +IPAProxy::~IPAProxy()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn IPAProxy::isValid()\n> + * \\brief Check if the IPAProxy instance is valid\n> + *\n> + * An IPAProxy instance is valid if the IPA interface is successfully created in\n> + * isolation, and IPC is successfully set up.\n> + *\n> + * \\return True if the IPAProxy is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Find a valid full path for a proxy worker for a given executable name\n> + * \\param[in] file File name of proxy worker executable\n> + *\n> + * A proxy worker's executable could be found in either the global installation\n> + * directory, or in the paths specified by the environment variable\n> + * LIBCAMERA_IPA_PROXY_PATH. This method checks the global install directory\n> + * first, then LIBCAMERA_IPA_PROXY_PATH in order, and returns the full path to\n> + * the proxy worker executable that is specified by file. The proxy worker\n> + * executable shall have exec permission.\n> + *\n> + * \\return The full path to the proxy worker executable, or empty string if\n\ns/or/or an/\n\n> + * no valid executable path\n> + */\n> +std::string IPAProxy::resolvePath(const std::string &file) const\n> +{\n> +\t/* Try finding the exec target from the install directory first */\n> +\tstd::string proxyFile = \"/\" + file;\n> +\tstd::string proxyPath = std::string(IPA_PROXY_DIR) + proxyFile;\n> +\tif (!access(proxyPath.c_str(), X_OK))\n> +\t\treturn proxyPath;\n> +\n> +\t/* No exec target in install directory; check env variable. */\n> +\tconst char *execPaths = utils::secure_getenv(\"LIBCAMERA_IPA_PROXY_PATH\");\n> +\twhile (execPaths) {\n> +\t\tconst char *delim = strchrnul(execPaths, ':');\n> +\t\tsize_t count = delim - execPaths;\n> +\n> +\t\tif (count) {\n> +\t\t\tstd::string proxyPath(execPaths, count);\n> +\t\t\tproxyPath += proxyFile;\n> +\t\t\tif (!access(proxyPath.c_str(), X_OK))\n> +\t\t\t\treturn proxyPath;\n> +\t\t}\n> +\n> +\t\tif (*delim == '\\0')\n> +\t\t\tbreak;\n> +\n> +\t\texecPaths += count + 1;\n> +\t}\n> +\n> +\treturn std::string();\n> +}\n> +\n> +/**\n> + * \\var IPAProxy::valid_\n> + * \\brief Flag to indicate if the IPAProxy instance is valid\n> + *\n> + * A IPAProxy instance is valid if the IPA interface is successfully created in\n> + * isolation, and IPC is successfully set up.\n> + *\n> + * This flag can be read via IPAProxy::isValid()\n\ns/$/./\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> + *\n> + * Implementations of the IPAProxy class should set this flag upon successful\n> + * construction.\n> + */\n> +\n> +/**\n> + * \\class IPAProxyFactory\n> + * \\brief Registration of IPAProxy classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of IPAProxy classes, the\n> + * IPAProxyFactory class maintains a registry of IPAProxy classes. Each\n> + * IPAProxy subclass shall register itself using the REGISTER_IPA_PROXY()\n> + * macro, which will create a corresponding instance of a IPAProxyFactory\n> + * subclass and register it with the static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a IPAProxy factory\n> + * \\param[in] name Name of the IPAProxy class\n> + *\n> + * Creating an instance of the factory registers is with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debugging and IPAProxy matching purposes\n> + * and shall be unique.\n> + */\n> +IPAProxyFactory::IPAProxyFactory(const char *name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\fn IPAProxyFactory::create()\n> + * \\brief Create an instance of the IPAProxy corresponding to the factory\n> + * \\param[in] ipam The IPA module\n> + *\n> + * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.\n> + * It creates a IPAProxy instance that isolates an IPA interface designated\n> + * by the IPA module \\a ipam.\n> + *\n> + * \\return a pointer to a newly constructed instance of the IPAProxy subclass\n> + * corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\fn IPAProxyFactory::name()\n> + * \\brief Retrieve the factory name\n> + * \\return The factory name\n> + */\n> +\n> +/**\n> + * \\brief Add a IPAProxy class to the registry\n> + * \\param[in] factory Factory to use to construct the IPAProxy\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the IPAProxy name.\n> + */\n> +void IPAProxyFactory::registerType(IPAProxyFactory *factory)\n> +{\n> +\tstd::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +\n> +\tLOG(IPAProxy, Debug)\n> +\t\t<< \"Registered proxy \\\"\" << factory->name() << \"\\\"\";\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all IPAProxy factories\n> + *\n> + * The static factories map is defined inside the function to ensure it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return the list of pipeline handler factories\n> + */\n> +std::vector<IPAProxyFactory *> &IPAProxyFactory::factories()\n> +{\n> +\tstatic std::vector<IPAProxyFactory *> factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\def REGISTER_IPA_PROXY\n> + * \\brief Register a IPAProxy with the IPAProxy factory\n> + * \\param[in] proxy Class name of IPAProxy derived class to register\n> + *\n> + * Register a proxy subclass with the factory and make it available to\n> + * isolate IPA modules.\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index a364f68..73a6fc7 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n>      'ipa_interface.cpp',\n>      'ipa_manager.cpp',\n>      'ipa_module.cpp',\n> +    'ipa_proxy.cpp',\n>      'ipc_unixsocket.cpp',\n>      'log.cpp',\n>      'media_device.cpp',\n> @@ -42,6 +43,7 @@ libcamera_headers = files([\n>      'include/formats.h',\n>      'include/ipa_manager.h',\n>      'include/ipa_module.h',\n> +    'include/ipa_proxy.h',\n>      'include/ipc_unixsocket.h',\n>      'include/log.h',\n>      'include/media_device.h',\n> @@ -63,6 +65,10 @@ includes = [\n>  \n>  subdir('pipeline')\n>  \n> +proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')\n> +config_h.set('IPA_PROXY_DIR',\n> +             '\"' + join_paths(get_option('prefix'), proxy_install_dir) + '\"')\n> +\n>  libudev = dependency('libudev', required : false)\n>  \n>  if libudev.found()\n> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp\n> index b119cf1..333d216 100644\n> --- a/test/libtest/test.cpp\n> +++ b/test/libtest/test.cpp\n> @@ -25,6 +25,10 @@ int Test::execute()\n>  \tif (ret)\n>  \t\treturn errno;\n>  \n> +\tret = setenv(\"LIBCAMERA_IPA_PROXY_PATH\", \"src/libcamera/proxy/worker\", 1);\n> +\tif (ret)\n> +\t\treturn errno;\n> +\n>  \tret = init();\n>  \tif (ret)\n>  \t\treturn ret;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E561260C23\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 11 Jul 2019 07:45:52 +0200 (CEST)","from pendragon.ideasonboard.com (softbank126163157105.bbtec.net\n\t[126.163.157.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5363531C;\n\tThu, 11 Jul 2019 07:45:51 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562823952;\n\tbh=812Y5oXvWpAmoBXU+mbfA0VUuy34KcTvYS6B4v6NbdI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=StAD/lBKDli9vwL9My4RabxXT2H11Cxi1TEfsuSy24jvcdVFb6E5wcSha4CBbPGmO\n\tOdQlaBxApq8HaxedHlV273j4ykXcM61d2DvcteIaXcaRqzeUkwxs71f5qBaK+buBS8\n\t0Ki5IPrVBSHSqO6HdJPPUQ9n2ohGObHXasr3qhE8=","Date":"Thu, 11 Jul 2019 08:45:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190711054524.GE5247@pendragon.ideasonboard.com>","References":"<20190709184450.32023-1-paul.elder@ideasonboard.com>\n\t<20190709184450.32023-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190709184450.32023-4-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 3/7] libcamera: add IPA proxy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 11 Jul 2019 05:45:53 -0000"}}]