[libcamera-devel,5/8] libcamera: ipa_manager: implement class for managing IPA modules

1323 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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/include/ipa_manager.h |  40 +++++++++
 src/libcamera/ipa_manager.cpp       | 129 ++++++++++++++++++++++++++++
 src/libcamera/meson.build           |   2 +
 3 files changed, 171 insertions(+)
 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, May 27, 2019 at 06:35:37PM -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.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/include/ipa_manager.h |  40 +++++++++
>  src/libcamera/ipa_manager.cpp       | 129 ++++++++++++++++++++++++++++
>  src/libcamera/meson.build           |   2 +
>  3 files changed, 171 insertions(+)
>  create mode 100644 src/libcamera/include/ipa_manager.h
>  create mode 100644 src/libcamera/ipa_manager.cpp
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> new file mode 100644
> index 0000000..fafafad
> --- /dev/null
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -0,0 +1,40 @@
> +/* 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 <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <list>
> +#include <string>

Same comment as for the previous patch.

> +
> +#include "ipa_module.h"
> +#include "pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class IPAManager
> +{
> +public:
> +	static IPAManager *instance();
> +
> +	int addDir(const std::string &libDir);

I would pass a const char * here, as that's what the caller has, so
conversion to std::string would only happen internally, as needed. I
would also name the method addDirectory.

This shouldn't be a public method, otherwise pipeline handlers would
have to call it, and thus all have a list of directories to consider. I
see two options:

- Make this method private, and call it from the constructor, with a
  hardcoded directory.

- Call this method from the CameraManager at init time, with the
  hardcoded directory specified in camera_manager.cpp. You could still
  make the method private, and specify the CameraManager as a friend.

The hardcoded directory should come from meson, and set to
join_paths(get_option('libdir'), 'libcamera'). You will need to get
meson to generate a config.h, see commit 2caceca8e135.

> +
> +	std::unique_ptr<IPAInterface> createIPA(PipelineHandler *pipe) const;

I wouldn't make this method const, as conceptually it modifies (by
calling load()) one of the IPA modules that the manager contains.

> +
> +private:
> +	std::list<IPAModule *> modules_;

Any reason to use list instead of vector ?

> +
> +	IPAManager();
> +	~IPAManager();
> +
> +	bool match(const IPAModule *ipam, PipelineHandler *pipe) const;
> +};
> +
> +} /* 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..3b3c1a6
> --- /dev/null
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -0,0 +1,129 @@
> +/* 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 "ipa_module.h"
> +#include "pipeline_handler.h"
> +#include "utils.h"
> +
> +#include <dlfcn.h>
> +#include <dirent.h>
> +#include <string.h>
> +#include <sys/types.h>
> +
> +#include "log.h"

ipa_manager.h first, followed by system headers, followed by libcamera
headers.

> +
> +/**
> + * \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()
> +{
> +}
> +
> +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
> + *
> + * Goes through \a libDir and tries to create an IPAModule instance for every
> + * found shared object. Skips invalid IPA modules.

Please add subjects to your sentences :-)

> + *
> + * \return total number of modules loaded (including modules loaded in
> + * previous calls of this function), or negative error code

That's a bit of a weird return value. I would either return the number
of modules loaded by this call, or just zero (on success).

", or a negative error code otherwise"

> + */
> +int IPAManager::addDir(const std::string &libDir)
> +{
> +	struct dirent *ent;
> +	DIR *dir;
> +
> +	dir = opendir(libDir.c_str());
> +	if (!dir) {
> +		int ret = -errno;
> +		LOG(IPAManager, Error) << "Invalid path for IPA modules: "
> +				       << strerror(ret);

I would print the path as part of the message (and use the usual LOG()
style as explained in a previous patch).

> +		return ret;
> +	}
> +
> +	while ((ent = readdir(dir)) != nullptr) {
> +		int offset = strlen(ent->d_name) - 3;

What if the file name is shorter than 3 characters ?

> +		if (strncmp(&ent->d_name[offset], ".so", 3))

I think you can use strcmp() as both strings are zero-terminated.

> +			continue;
> +
> +		IPAModule *ipaModule = new IPAModule(libDir + "/" + ent->d_name);
> +		if (ipaModule->isValid())
> +			modules_.push_back(ipaModule);

		else
			delete ipaModule;

otherwise you will leak it. You could also write

		if (!ipaModule->isValid()) {
			delete ipaModule;
			continue;
		}

		modules_.push_back(ipaModule);

> +	}
> +
> +	closedir(dir);
> +	return modules_.size();
> +}
> +
> +/**
> + * \brief Create an IPA interface that matches a given pipeline handler
> + * \param[in] pipe the pipeline handler that wants a matching IPA interface

s/the/The/

> + *
> + * \return IPA interface, or nullptr if no matching IPA module is found
> + */
> +std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe) const
> +{
> +	IPAModule *m = nullptr;
> +
> +	for (IPAModule *module : modules_) {
> +		if (match(module, pipe)) {
> +			m = module;
> +			break;
> +		}
> +	}
> +
> +	if (!m || !m->load())
> +		return nullptr;
> +
> +	return m->createInstance();
> +}
> +
> +bool IPAManager::match(const IPAModule *ipam, PipelineHandler *pipe) const

Open question, would it make sense to move this method to the IPAModule
class ?

> +{
> +	const struct IPAModuleInfo *info = &ipam->info();
> +
> +	return !strcmp(info->pipelineName, pipe->name()) &&
> +	       info->pipelineVersionMajor == pipe->majorVersion() &&
> +	       info->pipelineVersionMinor >= pipe->minorVersion() &&
> +	       info->moduleAPIVersion == IPA_MODULE_API_VERSION;

You should compare moduleAPIVersion first, as this conditions the layout
of the rest of the structure (granted, right now we support a single
version, so it doesn't matter and we'll have to change this code when
different versions will be needed, but it doesn't hurt to handle the
version correctly right away, with a comment explaining why).

> +}
> +
> +} /* 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',