[{"id":1756,"web_url":"https://patchwork.libcamera.org/comment/1756/","msgid":"<20190604115326.GJ4771@pendragon.ideasonboard.com>","date":"2019-06-04T11:53:26","subject":"Re: [libcamera-devel] [PATCH v2 06/10] libcamera: ipa_manager:\n\timplement class for managing IPA modules","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 Mon, Jun 03, 2019 at 07:16:33PM -0400, Paul Elder wrote:\n> IPAManager is a class that will search in given directories for IPA\n> modules, and will load them into a list. It also provides an interface\n> for pipeline handlers to aquire an IPA.\n\ns/aquire/acquire/\n\n> A meson build file for the IPAs is added, which also specifies a\n> hard-coded path for where to load the IPAs from in the installation\n> directory. More paths can be specified with the environment variable\n> IPA_MODULE_PATH, with the same syntax as the regular PATH environment\n\nI would name the environment variable LIBCAMERA_IPA_MODULE_PATH to use\nthe same prefix for all our environment variables.\n\n> variable. Make the test framework populate this environment variable.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> Changes in v2:\n> - make addDir private, and called from constructor\n> - add hard-coded IPA modules path from meson\n> - read environment variable for additional IPA module paths\n> - move match to IPAModule\n> - make addDir return value more sensible\n> - add the build IPA directory to the IPA module path for all tests\n> \n>  src/ipa/meson.build                 |   2 +\n>  src/libcamera/include/ipa_manager.h |  39 ++++++++\n>  src/libcamera/ipa_manager.cpp       | 141 ++++++++++++++++++++++++++++\n>  src/libcamera/meson.build           |   2 +\n>  src/meson.build                     |   1 +\n>  test/libtest/test.cpp               |   6 ++\n>  6 files changed, 191 insertions(+)\n>  create mode 100644 src/ipa/meson.build\n>  create mode 100644 src/libcamera/include/ipa_manager.h\n>  create mode 100644 src/libcamera/ipa_manager.cpp\n> \n> diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> new file mode 100644\n> index 0000000..be4f954\n> --- /dev/null\n> +++ b/src/ipa/meson.build\n> @@ -0,0 +1,2 @@\n> +config_h.set('IPA_MODULE_DIR',\n> +             '\"' + join_paths(get_option('prefix'), get_option('libdir'), 'libcamera') + '\"')\n> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h\n> new file mode 100644\n> index 0000000..694df64\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_manager.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_manager.h - Image Processing Algorithm module manager\n> + */\n> +#ifndef __LIBCAMERA_IPA_MANAGER_H__\n> +#define __LIBCAMERA_IPA_MANAGER_H__\n> +\n> +#include <list>\n\nYou don't use std::list below, but you use std::vector.\n\n> +#include <string>\n\nI don't think this is needed.\n\n> +\n> +#include <libcamera/ipa/ipa_interface.h>\n> +#include <libcamera/ipa/ipa_module_info.h>\n> +\n> +#include \"ipa_module.h\"\n> +#include \"pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +class IPAManager\n> +{\n> +public:\n> +\tstatic IPAManager *instance();\n> +\n> +\tstd::unique_ptr<IPAInterface> createIPA(PipelineHandler *pipe);\n> +\n> +private:\n> +\tstd::vector<IPAModule *> modules_;\n> +\n> +\tIPAManager();\n> +\t~IPAManager();\n> +\n> +\tint addDir(const char *libDir);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_MANAGER_H__ */\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> new file mode 100644\n> index 0000000..6a946a2\n> --- /dev/null\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -0,0 +1,141 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_manager.cpp - Image Processing Algorithm module manager\n> + */\n> +\n> +#include \"ipa_manager.h\"\n> +\n> +#include <dlfcn.h>\n\nDo you need this file ?\n\n> +#include <dirent.h>\n> +#include <string.h>\n> +#include <sys/types.h>\n> +\n> +#include \"ipa_module.h\"\n> +#include \"log.h\"\n> +#include \"pipeline_handler.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file ipa_manager.h\n> + * \\brief Image Processing Algorithm module manager\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPAManager)\n> +\n> +/**\n> + * \\class IPAManager\n> + * \\brief Manager for IPA modules\n> + */\n> +\n> +IPAManager::IPAManager()\n> +{\n> +\taddDir(IPA_MODULE_DIR);\n> +\n> +\tchar *modulePaths = utils::secure_getenv(\"IPA_MODULE_PATH\");\n> +\tif (!modulePaths)\n> +\t\treturn;\n> +\n> +\tchar *saveptr;\n> +\tchar *dir;\n> +\tif ((dir = strtok_r(modulePaths, \":\", &saveptr)))\n> +\t\taddDir(dir);\n> +\twhile ((dir = strtok_r(nullptr, \":\", &saveptr)))\n> +\t\taddDir(dir);\n\n>From the getenv man page:\n\n       As typically implemented, getenv() returns a pointer to a string\n       within the environment list. The caller must take care not to\n       modify this string, since that would change the environment of\n       the process.\n\n>From the strtok man page:\n\n\tBe cautious when using these functions.  If you do use them, note that:\n\n       * These functions modify their first argument.\n\nYou should strdup() the string, or avoid using strtok. As the token is a\nsingle character you could use strchr() instead of strtok(), but you\nwould have to make a copy of the string anyway to add the terminating 0,\nso making a copy of the whole environment variable string is likely\nbest (don't forget to free it of course).\n\nAlternatively, you could go the C++ way and use std::string::find() to\nlocate the delimiter, and std::string::substr() to extract the\nsub-string.\n\n\tstd::string modulePaths = utils::secure_getenv(\"IPA_MODULE_PATH\");\n\tif (modulePaths.empty())\n\t\treturn;\n\n\tfor (size_type pos = 0, delim = 0; delim != std::string::npos;\n\t     pos = delim + 1) {\n\t\tdelim = modulePaths.find(':', pos));\n\t\tsize_type count = delim == std::string::npos ? delim : delim - pos;\n\t\tstd::string path = modulePaths.substr(pos, count);\n\t\tif (path.empty())\n\t\t\tcontinue;\n\n\t\taddDir(path);\n\t}\n\n(untested, with an additional safety checks for empty elements)\n\nIn that case I would modify addDir() to take a const std::string &.\n\n> +}\n> +\n> +IPAManager::~IPAManager()\n> +{\n> +\tfor (IPAModule *module : modules_)\n> +\t\tdelete module;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the IPA manager instance\n> + *\n> + * The IPAManager is a singleton and can't be constructed manually. This\n> + * function shall instead be used to retrieve the single global instance of the\n> + * manager.\n> + *\n> + * \\return The IPA manager instance\n> + */\n> +IPAManager *IPAManager::instance()\n> +{\n> +\tstatic IPAManager ipaManager;\n> +\treturn &ipaManager;\n> +}\n> +\n> +/**\n> + * \\brief Load IPA modules from a directory\n> + * \\param[in] libDir directory to search for IPA modules\n> + *\n> + * This method tries to create an IPAModule instance for every found\n> + * shared object in \\a libDir, and skips invalid IPA modules.\n\ns/found shared object/shared object found/ ?\n\n> + *\n> + * \\return number of modules loaded by this call, or a negative error code\n> + * otherwise\n\ns/number/Number/\n\n(I think you got the message by now :-))\n\n> + */\n> +int IPAManager::addDir(const char *libDir)\n> +{\n> +\tstruct dirent *ent;\n> +\tDIR *dir;\n> +\n> +\tdir = opendir(libDir);\n> +\tif (!dir) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(IPAManager, Error)\n> +\t\t\t<< \"Invalid path \" << libDir << \" for IPA modules: \"\n> +\t\t\t<< strerror(ret);\n\nstrerror(-ret)\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tint count = 0;\n\nunsigned int\n\n> +\twhile ((ent = readdir(dir)) != nullptr) {\n> +\t\tif (strlen(ent->d_name) < 3)\n> +\t\t\tcontinue;\n> +\t\tint offset = strlen(ent->d_name) - 3;\n\nYou could optimise this slightly by writing\n\n\t\tint offset = strlen(ent->d_name) - 3;\n\t\tif (offset < 0)\n\t\t\tcontinue;\n\n> +\t\tif (strcmp(&ent->d_name[offset], \".so\"))\n> +\t\t\tcontinue;\n> +\n> +\t\tIPAModule *ipaModule = new IPAModule(std::string(libDir) +\n\nIf libDir becomes an std::string reference as proposed above, you can\nremove the explicit construction.\n\n> +\t\t\t\t\t\t     \"/\" + ent->d_name);\n> +\t\tif (!ipaModule->isValid()) {\n> +\t\t\tdelete ipaModule;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tmodules_.push_back(ipaModule);\n> +\t\tcount++;\n> +\t}\n> +\n> +\tclosedir(dir);\n> +\treturn count;\n> +}\n> +\n> +/**\n> + * \\brief Create an IPA interface that matches a given pipeline handler\n> + * \\param[in] pipe The pipeline handler that wants a matching IPA interface\n> + *\n> + * \\return IPA interface, or nullptr if no matching IPA module is found\n\ns/IPA interface/A newly created IPA interface/\n\nWith all those small issues fixed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> + */\n> +std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe)\n> +{\n> +\tIPAModule *m = nullptr;\n> +\n> +\tfor (IPAModule *module : modules_) {\n> +\t\tif (module->match(pipe)) {\n> +\t\t\tm = module;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!m || !m->load())\n> +\t\treturn nullptr;\n> +\n> +\treturn m->createInstance();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 32f7da4..0889b0d 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -11,6 +11,7 @@ libcamera_sources = files([\n>      'formats.cpp',\n>      'geometry.cpp',\n>      'ipa_interface.cpp',\n> +    'ipa_manager.cpp',\n>      'ipa_module.cpp',\n>      'log.cpp',\n>      'media_device.cpp',\n> @@ -33,6 +34,7 @@ libcamera_headers = files([\n>      'include/device_enumerator_udev.h',\n>      'include/event_dispatcher_poll.h',\n>      'include/formats.h',\n> +    'include/ipa_manager.h',\n>      'include/ipa_module.h',\n>      'include/log.h',\n>      'include/media_device.h',\n> diff --git a/src/meson.build b/src/meson.build\n> index 4e41fd3..628e7a7 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -1,3 +1,4 @@\n>  subdir('libcamera')\n> +subdir('ipa')\n>  subdir('cam')\n>  subdir('qcam')\n> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp\n> index 9d537ea..451c111 100644\n> --- a/test/libtest/test.cpp\n> +++ b/test/libtest/test.cpp\n> @@ -5,6 +5,8 @@\n>   * test.cpp - libcamera test base class\n>   */\n>  \n> +#include <stdlib.h>\n> +\n>  #include \"test.h\"\n>  \n>  Test::Test()\n> @@ -19,6 +21,10 @@ int Test::execute()\n>  {\n>  \tint ret;\n>  \n> +\tret = setenv(\"IPA_MODULE_PATH\", \"test/ipa\", 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 69A1F630A9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Jun 2019 13:53:40 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DDBAA2D2;\n\tTue,  4 Jun 2019 13:53:39 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559649220;\n\tbh=xLn86UtceJ81KB9nrfrcx1OF30PogG94jfzY6yfp9t0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tQoJthFy4dXeDqhq0q3dOaZ7DGoYZIzb3w7xjHXFf0kBNaWyE6JHdg0cS3Aselnns\n\tMuxDKzp7IdNxy8y7qE5LM6l9fr8y0fgewSB2dmPfi55JLCTiEBd5iK29WeSnBfpEWp\n\teBzhwi4VF5zhz7fx/td0BWZgGsVjE7bWo4/b3Pgg=","Date":"Tue, 4 Jun 2019 14:53:26 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604115326.GJ4771@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190603231637.28554-7-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 06/10] libcamera: ipa_manager:\n\timplement class for managing IPA modules","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":"Tue, 04 Jun 2019 11:53:40 -0000"}}]