[libcamera-devel,v2,06/10] libcamera: ipa_manager: implement class for managing IPA modules

1343 diff mbox series

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.

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
variable. Make the test framework populate this environment variable.

Signed-off-by: Paul Elder <paul.elder@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

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@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@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;