[{"id":2136,"web_url":"https://patchwork.libcamera.org/comment/2136/","msgid":"<20190703212643.GJ5007@pendragon.ideasonboard.com>","date":"2019-07-03T21:26:43","subject":"Re: [libcamera-devel] [RFC PATCH v2 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 03, 2019 at 05:00:03PM +0900, Paul Elder wrote:\n> Add a Proxy class whose implementations will act as a proxy between a\n\nI think the class should be called IPAProxy.\n\n> pipeline handler and an isolated IPA interface. Also add a ProxyFactory\n\nAnd this one IPAProxyFactory.\n\n> that will construct the Proxy implementations as necessary.\n> \n> Update Doxygen to ignore the directory where Proxy implementations will\n> reside.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\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 |  68 ++++++++++\n>  src/libcamera/ipa_proxy.cpp       | 219 ++++++++++++++++++++++++++++++\n>  src/libcamera/meson.build         |   6 +\n>  test/libtest/test.cpp             |   4 +\n>  5 files changed, 299 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 9ca3224..8c24f94 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..b493c1a\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_proxy.h\n> @@ -0,0 +1,68 @@\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 <vector>\n\nYou \n\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 Proxy : public IPAInterface\n> +{\n> +public:\n> +\tProxy(IPAModule *ipam);\n> +\tvirtual ~Proxy();\n\nNo need to declare the destructor as virtual, as it is already virtual\nin the base class.\n\n> +\n> +\tvirtual int init() = 0;\n\nYou don't need this either for the same reason.\n\n> +\n> +\tbool isValid() const;\n> +\n> +protected:\n> +\tconst std::string resolvePath(const std::string &file) const;\n\nNo need to make the returned string const as you return it by value.\n\n> +\n> +\tstd::unique_ptr<IPAInterface> ipa_;\n\nThis is never used. And I would be surprised if it was, as the whole\npoint of the proxy is to access the IPA interface of the module through\nIPC, so it shouldn't have a direct pointer to the IPAInterface.\n\n> +\n> +\tbool valid_;\n> +};\n> +\n> +class ProxyFactory\n> +{\n> +public:\n> +\tProxyFactory(const char *name);\n> +\tvirtual ~ProxyFactory() { };\n> +\n> +\tvirtual std::unique_ptr<Proxy> create(IPAModule *ipam) = 0;\n> +\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tstatic void registerType(ProxyFactory *factory);\n> +\tstatic std::vector<ProxyFactory *> &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 ProxyFactory\t\\\n> +{\t\t\t\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\\\n> +\tproxy##Factory() : ProxyFactory(#proxy) {}\t\\\n> +\tstd::unique_ptr<Proxy> 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..8bb3622\n> --- /dev/null\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -0,0 +1,219 @@\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\niostream can be moved just above string.h. And do you actually need it ?\n\n> +\n> +/**\n> + * \\file ipa_proxy.h\n> + * \\brief IPA Proxy\n> + *\n> + * TODO write this\n\nYou know what I will say...\n\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> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Proxy)\n> +\n> +/**\n> + * \\class Proxy\n> + * \\brief IPA Proxy\n> + *\n> + * TODO write this\n\n... so I won't repeat it :-)\n\n> + *\n> + * Isolate IPA into separate process.\n> + */\n> +\n> +/**\n> + * \\brief Construct a Proxy instance\n> + * \\param[in] ipam The IPA module to wrap around\n> + *\n> + * Proxy instances shall be constructed through the ProxyFactory::create()\n> + * method implemented by the respective factories.\n> + */\n> +Proxy::Proxy(IPAModule *ipam)\n\nWhy does the base constructor take a parameter if it doesn't use it ? If\nyou expect all IPA proxies to store the module pointer inside you could\nadd a protected ipam_ member to the base proxy class and initialise it\nhere. Otherwise I would just remove the parameter.\n\n> +\t: valid_(false)\n> +{\n> +}\n> +\n> +Proxy::~Proxy()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Check if the Proxy instance is valid\n> + *\n> + * A Proxy 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 the Proxy is valid, false otherwise\n\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2019-July/003769.html\n\n> + */\n> +bool Proxy::isValid() const\n> +{\n> +\treturn valid_;\n> +}\n\nYou could make this an inline function in the class definition as it\njust returns a member variable.\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 must be have exec permission.\n\ns/must be/shall/\n\n> + *\n> + * \\return full path to proxy worker executable, or empty string if no valid\n\ns/full path to proxy/The full path to the proxy/\ns/or empty string/or an empty string/\n\n> + * executable path\n> + */\n> +const std::string Proxy::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\ns/variable/variable. /\n\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\nShould you verify that proxyPath starts with a / ?\n\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 Proxy::ipa_\n> + * \\brief The IPA interface that the Proxy is isolating\n> + */\n> +\n> +/**\n> + * \\var Proxy::valid_\n> + * \\brief Flag to indicate if the Proxy instance is valid\n> + *\n> + * A Proxy 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 Proxy::isValid()\n> + *\n> + * Implementations of the Proxy class should set this flag upon successful\n> + * construction.\n> + */\n> +\n> +/**\n> + * \\class ProxyFactory\n> + * \\brief Registration of Proxy classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of Proxy classes, the\n> + * ProxyFactory class maintains a registry of Proxy classes. Each\n> + * Proxy subclass shall register itself using the REGISTER_IPA_PROXY()\n> + * macro, which will create a corresponding instance of a ProxyFactory\n> + * subclass and register it with the static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a Proxy factory\n> + * \\param[in] name Name of the Proxy 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 Proxy matching purposes\n> + * and shall be unique.\n> + */\n> +ProxyFactory::ProxyFactory(const char *name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\fn ProxyFactory::create()\n> + * \\brief Create an instance of the Proxy 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 Proxy 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 Proxy subclass\n> + * corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\fn ProxyFactory::name()\n> + * \\brief Retrieve the factory name\n> + * \\return The factory name\n> + */\n> +\n> +/**\n> + * \\brief Add a Proxy class to the registry\n> + * \\param[in] factory Factory to use to construct the Proxy\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the Proxy name.\n> + */\n> +void ProxyFactory::registerType(ProxyFactory *factory)\n> +{\n> +\tstd::vector<ProxyFactory *> &factories = ProxyFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +\n> +\tLOG(Proxy, Debug)\n> +\t\t<< \"Registered proxy \\\"\" << factory->name() << \"\\\"\";\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all Proxy factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n\ns/ensures/ensure/\n\n(feel free to fix the typo in pipeline_handler.cpp)\n\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return the list of pipeline handler factories\n> + */\n> +std::vector<ProxyFactory *> &ProxyFactory::factories()\n> +{\n> +\tstatic std::vector<ProxyFactory *> factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\def REGISTER_IPA_PROXY\n> + * \\brief Register a Proxy with the Proxy factory\n> + * \\param[in] proxy Class name of Proxy 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 087b578..412564f 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> @@ -64,6 +66,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\nHow about moving this to src/libcamera/ipa/ to group it with the related\nIPA_MODULE_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..35aa021 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 23EB660C01\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  3 Jul 2019 23:27:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D8F324B;\n\tWed,  3 Jul 2019 23:27:03 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562189223;\n\tbh=VapyzvwBmU/AIXIwSSEmLuW4xyElJU7uR2WSBkPz4gU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lstcW/w4GK4rJGGLX4Y1Y8mCjW9nlOoE5UEDK2HKV4CaTHoTnR/CqMJQPrBbUcpr6\n\tEp0fX4KYEMrBAV2L8MLaRyFl/CRgwsYZSwTtxc17h/TSPaafYBRR5KBVzy8mxxC++g\n\tl4qI6CniK0bFS89NNB06E5IWgBuiyuffEXKu2w+8=","Date":"Thu, 4 Jul 2019 00:26:43 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190703212643.GJ5007@pendragon.ideasonboard.com>","References":"<20190703080007.21376-1-paul.elder@ideasonboard.com>\n\t<20190703080007.21376-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190703080007.21376-4-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 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":"Wed, 03 Jul 2019 21:27:04 -0000"}}]