[{"id":34790,"web_url":"https://patchwork.libcamera.org/comment/34790/","msgid":"<175188196895.40598.690081040021461386@localhost>","date":"2025-07-07T09:52:48","subject":"Re: [PATCH v2 1/8] libcamera: ipa_manager: Factor out .so file\n\tsearching","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nQuoting Paul Elder (2025-07-03 13:42:16)\n> In the near future we will add support for layers/shoes on top of\n> libcamera, which will need to search for shared object files similar to\n> how IPAManager does. To avoid code duplication, move this code out of\n> IPAManager into internal utils.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> No change in v2\n> ---\n> include/libcamera/internal/ipa_manager.h | 4 -\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/utils.h | 26 ++++++\n> src/libcamera/ipa_manager.cpp | 108 ++++-------------------\n> src/libcamera/meson.build | 1 +\n> src/libcamera/utils.cpp | 107 ++++++++++++++++++++++\n> 6 files changed, 154 insertions(+), 93 deletions(-)\n> create mode 100644 include/libcamera/internal/utils.h\n> create mode 100644 src/libcamera/utils.cpp\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index a0d448cf9862..6b0ba4b5477f 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -59,10 +59,6 @@ public:\n> #endif\n> \n> private:\n> - void parseDir(const char *libDir, unsigned int maxDepth,\n> - std::vector &files);\n> - unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> -\n> IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> uint32_t maxVersion);\n> \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 5c80a28c4cbe..690f5c5ec9f6 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -41,6 +41,7 @@ libcamera_internal_headers = files([\n> 'shared_mem_object.h',\n> 'source_paths.h',\n> 'sysfs.h',\n> + 'utils.h',\n> 'v4l2_device.h',\n> 'v4l2_pixelformat.h',\n> 'v4l2_subdevice.h',\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> new file mode 100644\n> index 000000000000..742657bebb28\n> --- /dev/null\n> +++ b/include/libcamera/internal/utils.h\n> @@ -0,0 +1,26 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * Miscellaneous utility functions\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace utils {\n> +\n> +void parseDir(const char *libDir, unsigned int maxDepth,\n> + std::vector &files);\n> +\n> +unsigned int addDir(const char *libDir, unsigned int maxDepth,\n> + std::function func);\n> +\n\nBy moving these into utils I feel like the function names are not self\nexplanatory anymore. Should parseDir possible renamed to something like\n\"findSharedObjects\"?\n\n> +} /* namespace utils */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 830750dcc0fb..ecad4845a077 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -8,9 +8,8 @@\n> #include \"libcamera/internal/ipa_manager.h\"\n> \n> #include \n> -#include \n> +#include \n> #include \n> -#include \n> \n> #include \n> #include \n> @@ -19,6 +18,7 @@\n> #include \"libcamera/internal/ipa_module.h\"\n> #include \"libcamera/internal/ipa_proxy.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/utils.h\"\n> \n> /**\n> * \\file ipa_manager.h\n> @@ -110,6 +110,20 @@ IPAManager::IPAManager()\n> \n> unsigned int ipaCount = 0;\n> \n> + auto &modules = modules_;\n> + std::function addDirHandler =\n> + [&modules](const std::string &file) {\n> + auto ipaModule = std::make_unique(file);\n> + if (!ipaModule->isValid())\n> + return 0;\n> +\n> + LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> +\n> + modules.push_back(std::move(ipaModule));\n> +\n> + return 1;\n> + };\n> +\n> /* User-specified paths take precedence. */\n> const char *modulePaths = utils::secure_getenv(\"LIBCAMERA_IPA_MODULE_PATH\");\n> if (modulePaths) {\n> @@ -117,7 +131,7 @@ IPAManager::IPAManager()\n> if (dir.empty())\n> continue;\n> \n> - ipaCount += addDir(dir.c_str());\n> + ipaCount += utils::addDir(dir.c_str(), 0, addDirHandler);\n> }\n> \n> if (!ipaCount)\n> @@ -138,11 +152,11 @@ IPAManager::IPAManager()\n> << \"libcamera is not installed. Adding '\"\n> << ipaBuildPath << \"' to the IPA search path\";\n> \n> - ipaCount += addDir(ipaBuildPath.c_str(), maxDepth);\n> + ipaCount += utils::addDir(ipaBuildPath.c_str(), maxDepth, addDirHandler);\n> }\n> \n> /* Finally try to load IPAs from the installed system path. */\n> - ipaCount += addDir(IPA_MODULE_DIR);\n> + ipaCount += utils::addDir(IPA_MODULE_DIR, 0, addDirHandler);\n> \n> if (!ipaCount)\n> LOG(IPAManager, Warning)\n> @@ -151,90 +165,6 @@ IPAManager::IPAManager()\n> \n> IPAManager::~IPAManager() = default;\n> \n> -/**\n> - * \\brief Identify shared library objects within a directory\n> - * \\param[in] libDir The directory to search for shared objects\n> - * \\param[in] maxDepth The maximum depth of sub-directories to parse\n> - * \\param[out] files A vector of paths to shared object library files\n> - *\n> - * Search a directory for .so files, allowing recursion down to sub-directories\n> - * no further than the depth specified by \\a maxDepth.\n> - *\n> - * Discovered shared objects are added to the \\a files vector.\n> - */\n> -void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,\n> - std::vector &files)\n> -{\n> - struct dirent *ent;\n> - DIR *dir;\n> -\n> - dir = opendir(libDir);\n> - if (!dir)\n> - return;\n> -\n> - while ((ent = readdir(dir)) != nullptr) {\n> - if (ent->d_type == DT_DIR && maxDepth) {\n> - if (strcmp(ent->d_name, \".\") == 0 ||\n> - strcmp(ent->d_name, \"..\") == 0)\n> - continue;\n> -\n> - std::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> -\n> - /* Recursion is limited to maxDepth. */\n> - parseDir(subdir.c_str(), maxDepth - 1, files);\n> -\n> - continue;\n> - }\n> -\n> - int offset = strlen(ent->d_name) - 3;\n> - if (offset < 0)\n> - continue;\n> - if (strcmp(&ent->d_name[offset], \".so\"))\n> - continue;\n> -\n> - files.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> - }\n> -\n> - closedir(dir);\n> -}\n> -\n> -/**\n> - * \\brief Load IPA modules from a directory\n> - * \\param[in] libDir The directory to search for IPA modules\n> - * \\param[in] maxDepth The maximum depth of sub-directories to search\n> - *\n> - * This function tries to create an IPAModule instance for every shared object\n> - * found in \\a libDir, and skips invalid IPA modules.\n> - *\n> - * Sub-directories are searched up to a depth of \\a maxDepth. A \\a maxDepth\n> - * value of 0 only searches the directory specified in \\a libDir.\n> - *\n> - * \\return Number of modules loaded by this call\n> - */\n> -unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> -{\n> - std::vector files;\n> -\n> - parseDir(libDir, maxDepth, files);\n> -\n> - /* Ensure a stable ordering of modules. */\n> - std::sort(files.begin(), files.end());\n> -\n> - unsigned int count = 0;\n> - for (const std::string &file : files) {\n> - auto ipaModule = std::make_unique(file);\n> - if (!ipaModule->isValid())\n> - continue;\n> -\n> - LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> -\n> - modules_.push_back(std::move(ipaModule));\n> - count++;\n> - }\n> -\n> - return count;\n> -}\n> -\n> /**\n> * \\brief Retrieve an IPA module that matches a given pipeline handler\n> * \\param[in] pipe The pipeline handler\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index de1eb99b28fd..6a71b2903d27 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -51,6 +51,7 @@ libcamera_internal_sources = files([\n> 'shared_mem_object.cpp',\n> 'source_paths.cpp',\n> 'sysfs.cpp',\n> + 'utils.cpp',\n> 'v4l2_device.cpp',\n> 'v4l2_pixelformat.cpp',\n> 'v4l2_subdevice.cpp',\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> new file mode 100644\n> index 000000000000..ef046ac3134e\n> --- /dev/null\n> +++ b/src/libcamera/utils.cpp\n> @@ -0,0 +1,107 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * Miscellaneous utility functions (internal)\n> + */\n> +\n> +#include \"libcamera/internal/utils.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file internal/utils.h\n> + * \\brief Miscellaneous utility functions (internal)\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace utils {\n> +\n> +/**\n> + * \\brief Identify shared library objects within a directory\n> + * \\param[in] libDir The directory to search for shared objects\n> + * \\param[in] maxDepth The maximum depth of sub-directories to parse\n> + * \\param[out] files A vector of paths to shared object library files\n> + *\n> + * Search a directory for .so files, allowing recursion down to sub-directories\n> + * no further than the depth specified by \\a maxDepth.\n> + *\n> + * Discovered shared objects are added to the \\a files vector.\n> + */\n> +void parseDir(const char *libDir, unsigned int maxDepth,\n\nShould we make that a std::string_view? But string_view is not null\nterminated - don't know.\n\n> + std::vector &files)\n> +{\n> + struct dirent *ent;\n> + DIR *dir;\n> +\n> + dir = opendir(libDir);\n> + if (!dir)\n> + return;\n> +\n> + while ((ent = readdir(dir)) != nullptr) {\n> + if (ent->d_type == DT_DIR && maxDepth) {\n> + if (strcmp(ent->d_name, \".\") == 0 ||\n> + strcmp(ent->d_name, \"..\") == 0)\n> + continue;\n> +\n> + std::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> +\n> + /* Recursion is limited to maxDepth. */\n> + parseDir(subdir.c_str(), maxDepth - 1, files);\n> +\n> + continue;\n> + }\n> +\n> + int offset = strlen(ent->d_name) - 3;\n> + if (offset < 0)\n> + continue;\n> + if (strcmp(&ent->d_name[offset], \".so\"))\n> + continue;\n> +\n> + files.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> + }\n> +\n> + closedir(dir);\n> +}\n> +\n> +/**\n> + * \\brief Execute some function on shared object files from a directory\n> + * \\param[in] libDir The directory to search for shared objects\n> + * \\param[in] maxDepth The maximum depth of sub-directories to search\n> + * \\param[in] func The function to execute on every shared object\n> + *\n> + * This function tries to execute the given function \\a func for every shared\n> + * object found in \\a libDir.\n> + *\n> + * Sub-directories are searched up to a depth of \\a maxDepth. A \\a maxDepth\n> + * value of 0 only searches the directory specified in \\a libDir.\n> + *\n> + * \\return Number of shared objects loaded by this call\n> + */\n> +unsigned int addDir(const char *libDir, unsigned int maxDepth,\n> + std::function func)\n> +{\n> + std::vector files;\n> +\n> + parseDir(libDir, maxDepth, files);\n> +\n> + /* Ensure a stable ordering of modules. */\n> + std::sort(files.begin(), files.end());\n\nWith the refactoring addDir is basically an invocation of \nstd::sort(files.begin(), files.end());\nstd::accumulate(files.begin(), files.end(), 0, func);\n\nSo I wonder if parseDir and addDir should be merged into a single\n\nint findSharedObjects(std::string libdir, int maxDepth, std::function handler)\nwhich returns the number of sucessfully handled shared objects.\n\nWhat do you think?\n\nBest regards,\nStefan\n\n> +\n> + unsigned int count = 0;\n> + for (const std::string &file : files)\n> + count += func(file);\n> +\n> + return count;\n> +}\n> +\n> +} /* namespace utils */\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.47.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C7ED7C3237\n\tfor ;\n\tMon, 7 Jul 2025 09:52:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BD7368E8A;\n\tMon, 7 Jul 2025 11:52:53 +0200 (CEST)","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 11E6268E7F\n\tfor ;\n\tMon, 7 Jul 2025 11:52:52 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c79f:85df:e7f5:4c31])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 6061A50A;\n\tMon, 7 Jul 2025 11:52:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r3tiS8L6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751881945;\n\tbh=oCL+DVaZVGh47let7phI5/a73kJsdW1MVOuOSEwxptY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=r3tiS8L6FPZHaLq2dMuyEUyjw3GspW0GtGsOBcLC2IY7x392lGtFD0fEHXjXxrEkS\n\t4pE1ZfIbIzRmo3eCFL+Ov0z4vqmkpZiENIjUyEw1PJ2RJqafxAv0+ah/64k1YeUqq+\n\txghykGqYr5YmE2UjcDjK96EPq7kaOHzabOH26CvY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250703114225.2074071-2-paul.elder@ideasonboard.com>","References":"<20250703114225.2074071-1-paul.elder@ideasonboard.com>\n\t<20250703114225.2074071-2-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v2 1/8] libcamera: ipa_manager: Factor out .so file\n\tsearching","From":"Stefan Klug ","Cc":"Paul Elder , kieran.bingham@ideasonboard.com,\n\tbarnabas.pocze@ideasonboard.com","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jul 2025 11:52:48 +0200","Message-ID":"<175188196895.40598.690081040021461386@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35034,"web_url":"https://patchwork.libcamera.org/comment/35034/","msgid":"<175325685476.774561.13002314475475682861@neptunite.rasen.tech>","date":"2025-07-23T07:47:34","subject":"Re: [PATCH v2 1/8] libcamera: ipa_manager: Factor out .so file\n\tsearching","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Stefan,\n\nThanks for the review.\n\nQuoting Stefan Klug (2025-07-07 18:52:48)\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> Quoting Paul Elder (2025-07-03 13:42:16)\n> > In the near future we will add support for layers/shoes on top of\n> > libcamera, which will need to search for shared object files similar to\n> > how IPAManager does. To avoid code duplication, move this code out of\n> > IPAManager into internal utils.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > No change in v2\n> > ---\n> > include/libcamera/internal/ipa_manager.h | 4 -\n> > include/libcamera/internal/meson.build | 1 +\n> > include/libcamera/internal/utils.h | 26 ++++++\n> > src/libcamera/ipa_manager.cpp | 108 ++++-------------------\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/utils.cpp | 107 ++++++++++++++++++++++\n> > 6 files changed, 154 insertions(+), 93 deletions(-)\n> > create mode 100644 include/libcamera/internal/utils.h\n> > create mode 100644 src/libcamera/utils.cpp\n> > \n> > diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> > index a0d448cf9862..6b0ba4b5477f 100644\n> > --- a/include/libcamera/internal/ipa_manager.h\n> > +++ b/include/libcamera/internal/ipa_manager.h\n> > @@ -59,10 +59,6 @@ public:\n> > #endif\n> > \n> > private:\n> > - void parseDir(const char *libDir, unsigned int maxDepth,\n> > - std::vector &files);\n> > - unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> > -\n> > IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> > uint32_t maxVersion);\n> > \n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 5c80a28c4cbe..690f5c5ec9f6 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([\n> > 'shared_mem_object.h',\n> > 'source_paths.h',\n> > 'sysfs.h',\n> > + 'utils.h',\n> > 'v4l2_device.h',\n> > 'v4l2_pixelformat.h',\n> > 'v4l2_subdevice.h',\n> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > new file mode 100644\n> > index 000000000000..742657bebb28\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -0,0 +1,26 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * Miscellaneous utility functions\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace utils {\n> > +\n> > +void parseDir(const char *libDir, unsigned int maxDepth,\n> > + std::vector &files);\n> > +\n> > +unsigned int addDir(const char *libDir, unsigned int maxDepth,\n> > + std::function func);\n> > +\n> \n> By moving these into utils I feel like the function names are not self\n> explanatory anymore. Should parseDir possible renamed to something like\n> \"findSharedObjects\"?\n> \n\nGood point. I'll make parseDir static and rename addDir to findSharedObjects.\n\n> > +} /* namespace utils */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> > index 830750dcc0fb..ecad4845a077 100644\n> > --- a/src/libcamera/ipa_manager.cpp\n> > +++ b/src/libcamera/ipa_manager.cpp\n> > @@ -8,9 +8,8 @@\n> > #include \"libcamera/internal/ipa_manager.h\"\n> > \n> > #include \n> > -#include \n> > +#include \n> > #include \n> > -#include \n> > \n> > #include \n> > #include \n> > @@ -19,6 +18,7 @@\n> > #include \"libcamera/internal/ipa_module.h\"\n> > #include \"libcamera/internal/ipa_proxy.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/utils.h\"\n> > \n> > /**\n> > * \\file ipa_manager.h\n> > @@ -110,6 +110,20 @@ IPAManager::IPAManager()\n> > \n> > unsigned int ipaCount = 0;\n> > \n> > + auto &modules = modules_;\n> > + std::function addDirHandler =\n> > + [&modules](const std::string &file) {\n> > + auto ipaModule = std::make_unique(file);\n> > + if (!ipaModule->isValid())\n> > + return 0;\n> > +\n> > + LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> > +\n> > + modules.push_back(std::move(ipaModule));\n> > +\n> > + return 1;\n> > + };\n> > +\n> > /* User-specified paths take precedence. */\n> > const char *modulePaths = utils::secure_getenv(\"LIBCAMERA_IPA_MODULE_PATH\");\n> > if (modulePaths) {\n> > @@ -117,7 +131,7 @@ IPAManager::IPAManager()\n> > if (dir.empty())\n> > continue;\n> > \n> > - ipaCount += addDir(dir.c_str());\n> > + ipaCount += utils::addDir(dir.c_str(), 0, addDirHandler);\n> > }\n> > \n> > if (!ipaCount)\n> > @@ -138,11 +152,11 @@ IPAManager::IPAManager()\n> > << \"libcamera is not installed. Adding '\"\n> > << ipaBuildPath << \"' to the IPA search path\";\n> > \n> > - ipaCount += addDir(ipaBuildPath.c_str(), maxDepth);\n> > + ipaCount += utils::addDir(ipaBuildPath.c_str(), maxDepth, addDirHandler);\n> > }\n> > \n> > /* Finally try to load IPAs from the installed system path. */\n> > - ipaCount += addDir(IPA_MODULE_DIR);\n> > + ipaCount += utils::addDir(IPA_MODULE_DIR, 0, addDirHandler);\n> > \n> > if (!ipaCount)\n> > LOG(IPAManager, Warning)\n> > @@ -151,90 +165,6 @@ IPAManager::IPAManager()\n> > \n> > IPAManager::~IPAManager() = default;\n> > \n> > -/**\n> > - * \\brief Identify shared library objects within a directory\n> > - * \\param[in] libDir The directory to search for shared objects\n> > - * \\param[in] maxDepth The maximum depth of sub-directories to parse\n> > - * \\param[out] files A vector of paths to shared object library files\n> > - *\n> > - * Search a directory for .so files, allowing recursion down to sub-directories\n> > - * no further than the depth specified by \\a maxDepth.\n> > - *\n> > - * Discovered shared objects are added to the \\a files vector.\n> > - */\n> > -void IPAManager::parseDir(const char *libDir, unsigned int maxDepth,\n> > - std::vector &files)\n> > -{\n> > - struct dirent *ent;\n> > - DIR *dir;\n> > -\n> > - dir = opendir(libDir);\n> > - if (!dir)\n> > - return;\n> > -\n> > - while ((ent = readdir(dir)) != nullptr) {\n> > - if (ent->d_type == DT_DIR && maxDepth) {\n> > - if (strcmp(ent->d_name, \".\") == 0 ||\n> > - strcmp(ent->d_name, \"..\") == 0)\n> > - continue;\n> > -\n> > - std::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> > -\n> > - /* Recursion is limited to maxDepth. */\n> > - parseDir(subdir.c_str(), maxDepth - 1, files);\n> > -\n> > - continue;\n> > - }\n> > -\n> > - int offset = strlen(ent->d_name) - 3;\n> > - if (offset < 0)\n> > - continue;\n> > - if (strcmp(&ent->d_name[offset], \".so\"))\n> > - continue;\n> > -\n> > - files.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> > - }\n> > -\n> > - closedir(dir);\n> > -}\n> > -\n> > -/**\n> > - * \\brief Load IPA modules from a directory\n> > - * \\param[in] libDir The directory to search for IPA modules\n> > - * \\param[in] maxDepth The maximum depth of sub-directories to search\n> > - *\n> > - * This function tries to create an IPAModule instance for every shared object\n> > - * found in \\a libDir, and skips invalid IPA modules.\n> > - *\n> > - * Sub-directories are searched up to a depth of \\a maxDepth. A \\a maxDepth\n> > - * value of 0 only searches the directory specified in \\a libDir.\n> > - *\n> > - * \\return Number of modules loaded by this call\n> > - */\n> > -unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> > -{\n> > - std::vector files;\n> > -\n> > - parseDir(libDir, maxDepth, files);\n> > -\n> > - /* Ensure a stable ordering of modules. */\n> > - std::sort(files.begin(), files.end());\n> > -\n> > - unsigned int count = 0;\n> > - for (const std::string &file : files) {\n> > - auto ipaModule = std::make_unique(file);\n> > - if (!ipaModule->isValid())\n> > - continue;\n> > -\n> > - LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> > -\n> > - modules_.push_back(std::move(ipaModule));\n> > - count++;\n> > - }\n> > -\n> > - return count;\n> > -}\n> > -\n> > /**\n> > * \\brief Retrieve an IPA module that matches a given pipeline handler\n> > * \\param[in] pipe The pipeline handler\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index de1eb99b28fd..6a71b2903d27 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([\n> > 'shared_mem_object.cpp',\n> > 'source_paths.cpp',\n> > 'sysfs.cpp',\n> > + 'utils.cpp',\n> > 'v4l2_device.cpp',\n> > 'v4l2_pixelformat.cpp',\n> > 'v4l2_subdevice.cpp',\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > new file mode 100644\n> > index 000000000000..ef046ac3134e\n> > --- /dev/null\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -0,0 +1,107 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * Miscellaneous utility functions (internal)\n> > + */\n> > +\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +/**\n> > + * \\file internal/utils.h\n> > + * \\brief Miscellaneous utility functions (internal)\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace utils {\n> > +\n> > +/**\n> > + * \\brief Identify shared library objects within a directory\n> > + * \\param[in] libDir The directory to search for shared objects\n> > + * \\param[in] maxDepth The maximum depth of sub-directories to parse\n> > + * \\param[out] files A vector of paths to shared object library files\n> > + *\n> > + * Search a directory for .so files, allowing recursion down to sub-directories\n> > + * no further than the depth specified by \\a maxDepth.\n> > + *\n> > + * Discovered shared objects are added to the \\a files vector.\n> > + */\n> > +void parseDir(const char *libDir, unsigned int maxDepth,\n> \n> Should we make that a std::string_view? But string_view is not null\n> terminated - don't know.\n\nI think we do want the null termination.\n\n> \n> > + std::vector &files)\n> > +{\n> > + struct dirent *ent;\n> > + DIR *dir;\n> > +\n> > + dir = opendir(libDir);\n> > + if (!dir)\n> > + return;\n> > +\n> > + while ((ent = readdir(dir)) != nullptr) {\n> > + if (ent->d_type == DT_DIR && maxDepth) {\n> > + if (strcmp(ent->d_name, \".\") == 0 ||\n> > + strcmp(ent->d_name, \"..\") == 0)\n> > + continue;\n> > +\n> > + std::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> > +\n> > + /* Recursion is limited to maxDepth. */\n> > + parseDir(subdir.c_str(), maxDepth - 1, files);\n> > +\n> > + continue;\n> > + }\n> > +\n> > + int offset = strlen(ent->d_name) - 3;\n> > + if (offset < 0)\n> > + continue;\n> > + if (strcmp(&ent->d_name[offset], \".so\"))\n> > + continue;\n> > +\n> > + files.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> > + }\n> > +\n> > + closedir(dir);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Execute some function on shared object files from a directory\n> > + * \\param[in] libDir The directory to search for shared objects\n> > + * \\param[in] maxDepth The maximum depth of sub-directories to search\n> > + * \\param[in] func The function to execute on every shared object\n> > + *\n> > + * This function tries to execute the given function \\a func for every shared\n> > + * object found in \\a libDir.\n> > + *\n> > + * Sub-directories are searched up to a depth of \\a maxDepth. A \\a maxDepth\n> > + * value of 0 only searches the directory specified in \\a libDir.\n> > + *\n> > + * \\return Number of shared objects loaded by this call\n> > + */\n> > +unsigned int addDir(const char *libDir, unsigned int maxDepth,\n> > + std::function func)\n> > +{\n> > + std::vector files;\n> > +\n> > + parseDir(libDir, maxDepth, files);\n> > +\n> > + /* Ensure a stable ordering of modules. */\n> > + std::sort(files.begin(), files.end());\n> \n> With the refactoring addDir is basically an invocation of \n> std::sort(files.begin(), files.end());\n> std::accumulate(files.begin(), files.end(), 0, func);\n> \n> So I wonder if parseDir and addDir should be merged into a single\n> \n> int findSharedObjects(std::string libdir, int maxDepth, std::function handler)\n> which returns the number of sucessfully handled shared objects.\n> \n> What do you think?\n\nI don't think it's nice to make func have to do the accumulation (since\nstd::accumulate does acc = op(acc, *i) and not acc = acc + op(*i)), and also\naddDir is recursive so we can't merge this into one function without\nredesigning it.\n\n\nThanks,\n\nPaul\n\n> \n> Best regards,\n> Stefan\n> \n> > +\n> > + unsigned int count = 0;\n> > + for (const std::string &file : files)\n> > + count += func(file);\n> > +\n> > + return count;\n> > +}\n> > +\n> > +} /* namespace utils */\n> > +\n> > +} /* namespace libcamera */\n> > -- \n> > 2.47.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 51B42C3237\n\tfor ;\n\tWed, 23 Jul 2025 07:47:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C3D869048;\n\tWed, 23 Jul 2025 09:47:43 +0200 (CEST)","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 D4B6068FB6\n\tfor ;\n\tWed, 23 Jul 2025 09:47:41 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:1d36:d4e9:a0a5:d2df])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id C7080111F; \n\tWed, 23 Jul 2025 09:47:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aFnIFjqr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753256823;\n\tbh=JcPgFCwuxrZpWg1Nc7P9MOziHIMc2PcIBqv1iZGkqVk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=aFnIFjqrjtVF/sAEci8YelCISDJ/jYZtQiJpIRcAwcO2cjAiEx01gSdRiwqmjrHPY\n\tqFoEgT26B1GpoXGE1Vkw50sF1YAOnezuXMjAHIUhxFVOE2MKONlRCxagMxM9DiJEsV\n\t8nr2OSF2ejzP3WRaKQ9LMOaZdTM1ip6+O6F0WeTE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175188196895.40598.690081040021461386@localhost>","References":"<20250703114225.2074071-1-paul.elder@ideasonboard.com>\n\t<20250703114225.2074071-2-paul.elder@ideasonboard.com>\n\t<175188196895.40598.690081040021461386@localhost>","Subject":"Re: [PATCH v2 1/8] libcamera: ipa_manager: Factor out .so file\n\tsearching","From":"Paul Elder ","Cc":"kieran.bingham@ideasonboard.com, barnabas.pocze@ideasonboard.com","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 23 Jul 2025 16:47:34 +0900","Message-ID":"<175325685476.774561.13002314475475682861@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]