[libcamera-devel] [PATCH 04/14] libcamera: Declare generic size and format converter interface

[Jacopo Mondi]

Hi Xavier
  I would drop "size and format" and use "Converter" in the subject
title.

On Thu, Sep 08, 2022 at 08:48:40PM +0200, Xavier Roumegue via libcamera-devel wrote:
> Declare a converter Abstract Base Class intented to provide generic
> interfaces to hardware offering size and format conversion services on
> streams. This is mainly based on the public interfaces of the current
> converter class implementation found in the simple pipeline handler.
>
> The main change is the introduction of load_configuration() method which
> can be used by the concrete implementation to load hardware specific
> runtime parameters defined by the application.
>
> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> ---
>  include/libcamera/internal/converter.h | 101 ++++++++++++++++++++++++
>  include/libcamera/internal/meson.build |   1 +
>  src/libcamera/converter.cpp            | 102 +++++++++++++++++++++++++
>  src/libcamera/meson.build              |   1 +
>  4 files changed, 205 insertions(+)
>  create mode 100644 include/libcamera/internal/converter.h
>  create mode 100644 src/libcamera/converter.cpp
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> new file mode 100644
> index 00000000..e2237c57
> --- /dev/null
> +++ b/include/libcamera/internal/converter.h
> @@ -0,0 +1,101 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Laurent Pinchart
> + * Copyright 2022 NXP
> + *
> + * converter.h - Generic stream converter infrastructure
> + */
> +
> +#pragma once
> +
> +#include <functional>

I might have missed what is functional included for

> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class MediaDevice;
> +class Size;
> +class SizeRange;
> +struct StreamConfiguration;
> +
> +class Converter
> +{
> +public:
> +	Converter(MediaDevice *media);
> +	virtual ~Converter();

Empty line maybe ?

> +	virtual int loadConfiguration(const std::string &filename) = 0;
> +
> +	virtual bool isValid() const = 0;
> +
> +	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
> +	virtual SizeRange sizes(const Size &input) = 0;
> +
> +	virtual std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> +
> +	virtual int configure(const StreamConfiguration &inputCfg,
> +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg) = 0;
> +	virtual int exportBuffers(unsigned int ouput, unsigned int count,
> +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> +
> +	virtual int start() = 0;
> +	virtual void stop() = 0;
> +
> +	virtual int queueBuffers(FrameBuffer *input,
> +				 const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> +
> +	std::string deviceNode_;
> +	Signal<FrameBuffer *> inputBufferReady;
> +	Signal<FrameBuffer *> outputBufferReady;
> +};
> +
> +class ConverterFactory
> +{
> +public:
> +	ConverterFactory(const std::string name);
> +	virtual ~ConverterFactory() = default;
> +
> +	static std::unique_ptr<Converter> create(MediaDevice *media);
> +
> +	static void registerType(ConverterFactory *factory);
> +	static std::vector<ConverterFactory *> &factories();
> +	static std::vector<std::string> names();
> +
> +protected:
> +	virtual Converter *createInstance(MediaDevice *media) = 0;
> +	virtual const std::vector<std::string> aliases() const = 0;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactory)
> +
> +	std::string name_;
> +};
> +
> +#define REGISTER_CONVERTER(name, converter, ...)                            \
> +	class converter##Factory final : public ConverterFactory                \
> +	{                                                                       \
> +	public:                                                                 \
> +		converter##Factory() : ConverterFactory(name) {}                    \
> +                                                                            \
> +	private:                                                                \
> +		Converter *createInstance(MediaDevice *media)                       \
> +		{                                                                   \
> +			return new converter(media);                                    \
> +		}                                                                   \
> +		std::vector<std::string> aliases_ = { __VA_ARGS__ };                \
> +		const std::vector<std::string> aliases() const { return aliases_; } \
> +	};                                                                      \
> +	static converter##Factory global_##converter##Factory;
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 7a780d48..8f50d755 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -19,6 +19,7 @@ libcamera_internal_headers = files([
>      'camera_sensor_properties.h',
>      'control_serializer.h',
>      'control_validator.h',
> +    'converter.h',
>      'delayed_controls.h',
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> new file mode 100644
> index 00000000..89a594d1
> --- /dev/null
> +++ b/src/libcamera/converter.cpp
> @@ -0,0 +1,102 @@
> +

Rogue empty line

> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright 2022 NXP
> + *
> + * converter.cpp - Generic Format converter interface
> + */
> +
> +#include <algorithm>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/converter.h"
> +#include "libcamera/internal/media_device.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Converter)
> +
> +Converter::Converter(MediaDevice *media)
> +{
> +	const std::vector<MediaEntity *> &entities = media->entities();
> +	auto it = std::find_if(entities.begin(), entities.end(),
> +			       [](MediaEntity *entity) {
> +				       return entity->function() == MEDIA_ENT_F_IO_V4L;
> +			       });
> +	if (it == entities.end())
> +		return;

Should we complain loud and set some state variable that can be
inspected by the isValid() function already ?

> +
> +	deviceNode_ = (*it)->deviceNode();

I'm not sure storing a string here is any useful if not fo debugging
purposes. What if you store a pointer to the entity instead, so
derived classes can use the static fromMediaEntity() function (which
probably have to be added to the M2M device) to open the device node ?

> +}
> +
> +Converter::~Converter()
> +{
> +}
> +
> +ConverterFactory::ConverterFactory(const std::string name)
> +	: name_(name)
> +{
> +	registerType(this);
> +}
> +
> +std::unique_ptr<Converter> ConverterFactory::create(MediaDevice *media)
> +{
> +	std::vector<ConverterFactory *> &factories =
> +		ConverterFactory::factories();

Oh, this is different from the other favtories we have.
The usage here is:

        ConverterFavtory::create()
                for (factory: factories())
                        factory->create();

While other factories (looking at the pipeline handler factory) have a
static method to retrieve factories, and then call create() on them.

My understanding is that this shouldn't change much in regard to the
link order protection realized by declaring factories_ inside the
factories() function, just pointing it out to see if someone else see
an issue here.

> +
> +	for (ConverterFactory *factory : factories) {
> +		std::vector<std::string> aliases = factory->aliases();
> +		auto it = std::find(aliases.begin(), aliases.end(), media->driver());

How do you envision aliases to be used ? I mean, do you expect a
Converter instance to handle different devices ? Probably yes, as a
generic M2M converter will follow in the series and could handle
multiple devices...

> +
> +		if (it == aliases.end() && media->driver() != factory->name_)

what is "media->driver() != factory->name_)" for ? Shouldn't the media
driver name be part of aliases only ?

> +			continue;
> +
> +		LOG(Converter, Debug)
> +			<< "Creating converter from "
> +			<< factory->name_ << " factory with "
> +			<< (it == aliases.end() ? "no" : media->driver()) << " alias.";

How long is this message ? Can it be shortened or the factory name
here is relevant ?

> +
> +		Converter *converter = factory->createInstance(media);
> +		return std::unique_ptr<Converter>(converter);
> +	}
> +
> +	return nullptr;
> +}
> +
> +void ConverterFactory::registerType(ConverterFactory *factory)
> +{
> +	std::vector<ConverterFactory *> &factories =
> +		ConverterFactory::factories();
> +
> +	factories.push_back(factory);
> +}
> +
> +std::vector<std::string> ConverterFactory::names()
> +{
> +	std::vector<std::string> list;
> +
> +	std::vector<ConverterFactory *> &factories =
> +		ConverterFactory::factories();
> +
> +	for (ConverterFactory *factory : factories) {
> +		list.push_back(factory->name_);
> +		for (auto alias : factory->aliases())
> +			list.push_back(alias);
> +	}
> +
> +	return list;
> +}
> +
> +std::vector<ConverterFactory *> &ConverterFactory::factories()
> +{
> +	/*
> +	 * The static factories map is defined inside the function to ensure
> +	 * it gets initialized on first use, without any dependency on link
> +	 * order.
> +	 */
> +	static std::vector<ConverterFactory *> factories;
> +	return factories;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 63b47b17..a261d4b4 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -13,6 +13,7 @@ libcamera_sources = files([
>      'controls.cpp',
>      'control_serializer.cpp',
>      'control_validator.cpp',
> +    'converter.cpp',
>      'delayed_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
> --
> 2.37.3
>