[libcamera-devel] [PATCH v2 06/10] libcamera: ipa_manager: implement class for managing IPA modules
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 4 13:53:26 CEST 2019
Hi Paul,
Thank you for the patch.
On Mon, Jun 03, 2019 at 07:16:33PM -0400, Paul Elder wrote:
> IPAManager is a class that will search in given directories for IPA
> modules, and will load them into a list. It also provides an interface
> for pipeline handlers to aquire an IPA.
s/aquire/acquire/
> A meson build file for the IPAs is added, which also specifies a
> hard-coded path for where to load the IPAs from in the installation
> directory. More paths can be specified with the environment variable
> IPA_MODULE_PATH, with the same syntax as the regular PATH environment
I would name the environment variable LIBCAMERA_IPA_MODULE_PATH to use
the same prefix for all our environment variables.
> variable. Make the test framework populate this environment variable.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - make addDir private, and called from constructor
> - add hard-coded IPA modules path from meson
> - read environment variable for additional IPA module paths
> - move match to IPAModule
> - make addDir return value more sensible
> - add the build IPA directory to the IPA module path for all tests
>
> src/ipa/meson.build | 2 +
> src/libcamera/include/ipa_manager.h | 39 ++++++++
> src/libcamera/ipa_manager.cpp | 141 ++++++++++++++++++++++++++++
> src/libcamera/meson.build | 2 +
> src/meson.build | 1 +
> test/libtest/test.cpp | 6 ++
> 6 files changed, 191 insertions(+)
> create mode 100644 src/ipa/meson.build
> create mode 100644 src/libcamera/include/ipa_manager.h
> create mode 100644 src/libcamera/ipa_manager.cpp
>
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> new file mode 100644
> index 0000000..be4f954
> --- /dev/null
> +++ b/src/ipa/meson.build
> @@ -0,0 +1,2 @@
> +config_h.set('IPA_MODULE_DIR',
> + '"' + join_paths(get_option('prefix'), get_option('libdir'), 'libcamera') + '"')
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> new file mode 100644
> index 0000000..694df64
> --- /dev/null
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_manager.h - Image Processing Algorithm module manager
> + */
> +#ifndef __LIBCAMERA_IPA_MANAGER_H__
> +#define __LIBCAMERA_IPA_MANAGER_H__
> +
> +#include <list>
You don't use std::list below, but you use std::vector.
> +#include <string>
I don't think this is needed.
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +
> +#include "ipa_module.h"
> +#include "pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class IPAManager
> +{
> +public:
> + static IPAManager *instance();
> +
> + std::unique_ptr<IPAInterface> createIPA(PipelineHandler *pipe);
> +
> +private:
> + std::vector<IPAModule *> modules_;
> +
> + IPAManager();
> + ~IPAManager();
> +
> + int addDir(const char *libDir);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_MANAGER_H__ */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> new file mode 100644
> index 0000000..6a946a2
> --- /dev/null
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_manager.cpp - Image Processing Algorithm module manager
> + */
> +
> +#include "ipa_manager.h"
> +
> +#include <dlfcn.h>
Do you need this file ?
> +#include <dirent.h>
> +#include <string.h>
> +#include <sys/types.h>
> +
> +#include "ipa_module.h"
> +#include "log.h"
> +#include "pipeline_handler.h"
> +#include "utils.h"
> +
> +/**
> + * \file ipa_manager.h
> + * \brief Image Processing Algorithm module manager
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPAManager)
> +
> +/**
> + * \class IPAManager
> + * \brief Manager for IPA modules
> + */
> +
> +IPAManager::IPAManager()
> +{
> + addDir(IPA_MODULE_DIR);
> +
> + char *modulePaths = utils::secure_getenv("IPA_MODULE_PATH");
> + if (!modulePaths)
> + return;
> +
> + char *saveptr;
> + char *dir;
> + if ((dir = strtok_r(modulePaths, ":", &saveptr)))
> + addDir(dir);
> + while ((dir = strtok_r(nullptr, ":", &saveptr)))
> + addDir(dir);
>From the getenv man page:
As typically implemented, getenv() returns a pointer to a string
within the environment list. The caller must take care not to
modify this string, since that would change the environment of
the process.
>From the strtok man page:
Be cautious when using these functions. If you do use them, note that:
* These functions modify their first argument.
You should strdup() the string, or avoid using strtok. As the token is a
single character you could use strchr() instead of strtok(), but you
would have to make a copy of the string anyway to add the terminating 0,
so making a copy of the whole environment variable string is likely
best (don't forget to free it of course).
Alternatively, you could go the C++ way and use std::string::find() to
locate the delimiter, and std::string::substr() to extract the
sub-string.
std::string modulePaths = utils::secure_getenv("IPA_MODULE_PATH");
if (modulePaths.empty())
return;
for (size_type pos = 0, delim = 0; delim != std::string::npos;
pos = delim + 1) {
delim = modulePaths.find(':', pos));
size_type count = delim == std::string::npos ? delim : delim - pos;
std::string path = modulePaths.substr(pos, count);
if (path.empty())
continue;
addDir(path);
}
(untested, with an additional safety checks for empty elements)
In that case I would modify addDir() to take a const std::string &.
> +}
> +
> +IPAManager::~IPAManager()
> +{
> + for (IPAModule *module : modules_)
> + delete module;
> +}
> +
> +/**
> + * \brief Retrieve the IPA manager instance
> + *
> + * The IPAManager is a singleton and can't be constructed manually. This
> + * function shall instead be used to retrieve the single global instance of the
> + * manager.
> + *
> + * \return The IPA manager instance
> + */
> +IPAManager *IPAManager::instance()
> +{
> + static IPAManager ipaManager;
> + return &ipaManager;
> +}
> +
> +/**
> + * \brief Load IPA modules from a directory
> + * \param[in] libDir directory to search for IPA modules
> + *
> + * This method tries to create an IPAModule instance for every found
> + * shared object in \a libDir, and skips invalid IPA modules.
s/found shared object/shared object found/ ?
> + *
> + * \return number of modules loaded by this call, or a negative error code
> + * otherwise
s/number/Number/
(I think you got the message by now :-))
> + */
> +int IPAManager::addDir(const char *libDir)
> +{
> + struct dirent *ent;
> + DIR *dir;
> +
> + dir = opendir(libDir);
> + if (!dir) {
> + int ret = -errno;
> + LOG(IPAManager, Error)
> + << "Invalid path " << libDir << " for IPA modules: "
> + << strerror(ret);
strerror(-ret)
> + return ret;
> + }
> +
> + int count = 0;
unsigned int
> + while ((ent = readdir(dir)) != nullptr) {
> + if (strlen(ent->d_name) < 3)
> + continue;
> + int offset = strlen(ent->d_name) - 3;
You could optimise this slightly by writing
int offset = strlen(ent->d_name) - 3;
if (offset < 0)
continue;
> + if (strcmp(&ent->d_name[offset], ".so"))
> + continue;
> +
> + IPAModule *ipaModule = new IPAModule(std::string(libDir) +
If libDir becomes an std::string reference as proposed above, you can
remove the explicit construction.
> + "/" + ent->d_name);
> + if (!ipaModule->isValid()) {
> + delete ipaModule;
> + continue;
> + }
> +
> + modules_.push_back(ipaModule);
> + count++;
> + }
> +
> + closedir(dir);
> + return count;
> +}
> +
> +/**
> + * \brief Create an IPA interface that matches a given pipeline handler
> + * \param[in] pipe The pipeline handler that wants a matching IPA interface
> + *
> + * \return IPA interface, or nullptr if no matching IPA module is found
s/IPA interface/A newly created IPA interface/
With all those small issues fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe)
> +{
> + IPAModule *m = nullptr;
> +
> + for (IPAModule *module : modules_) {
> + if (module->match(pipe)) {
> + m = module;
> + break;
> + }
> + }
> +
> + if (!m || !m->load())
> + return nullptr;
> +
> + return m->createInstance();
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 32f7da4..0889b0d 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -11,6 +11,7 @@ libcamera_sources = files([
> 'formats.cpp',
> 'geometry.cpp',
> 'ipa_interface.cpp',
> + 'ipa_manager.cpp',
> 'ipa_module.cpp',
> 'log.cpp',
> 'media_device.cpp',
> @@ -33,6 +34,7 @@ libcamera_headers = files([
> 'include/device_enumerator_udev.h',
> 'include/event_dispatcher_poll.h',
> 'include/formats.h',
> + 'include/ipa_manager.h',
> 'include/ipa_module.h',
> 'include/log.h',
> 'include/media_device.h',
> diff --git a/src/meson.build b/src/meson.build
> index 4e41fd3..628e7a7 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -1,3 +1,4 @@
> subdir('libcamera')
> +subdir('ipa')
> subdir('cam')
> subdir('qcam')
> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> index 9d537ea..451c111 100644
> --- a/test/libtest/test.cpp
> +++ b/test/libtest/test.cpp
> @@ -5,6 +5,8 @@
> * test.cpp - libcamera test base class
> */
>
> +#include <stdlib.h>
> +
> #include "test.h"
>
> Test::Test()
> @@ -19,6 +21,10 @@ int Test::execute()
> {
> int ret;
>
> + ret = setenv("IPA_MODULE_PATH", "test/ipa", 1);
> + if (ret)
> + return errno;
> +
> ret = init();
> if (ret)
> return ret;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list