[libcamera-devel,1/3] libcamera: converter: split ConverterMD (media device) out of Converter

18980 diff mbox series

Make Converter a bit more generic by not requiring it to use a media device.
For this split out ConverterMD from Converter class, and rename
ConverterFactoryBase / ConverterFactory to ConverterMDFactoryBase /
ConverterMDFactory for consistency.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 include/libcamera/internal/converter.h        |  49 +---
 .../internal/converter/converter_v4l2_m2m.h   |   4 +-
 include/libcamera/internal/converter_media.h  |  86 +++++++
 include/libcamera/internal/meson.build        |   1 +
 src/libcamera/converter.cpp                   | 191 +-------------
 .../converter/converter_v4l2_m2m.cpp          |   4 +-
 src/libcamera/converter_media.cpp             | 241 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 src/libcamera/pipeline/simple/simple.cpp      |   4 +-
 9 files changed, 337 insertions(+), 244 deletions(-)
 create mode 100644 include/libcamera/internal/converter_media.h
 create mode 100644 src/libcamera/converter_media.cpp

Hi Andrey

On Sun, Sep 10, 2023 at 08:50:25PM +0300, Andrey Konovalov via libcamera-devel wrote:
> Make Converter a bit more generic by not requiring it to use a media device.
> For this split out ConverterMD from Converter class, and rename
> ConverterFactoryBase / ConverterFactory to ConverterMDFactoryBase /
> ConverterMDFactory for consistency.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

I understand where this is going, but before going into a full review
I would like to explore the possibility of generalizing the existing
hierarchy before splitting it in media and non-media versions, which
would require re-defining another FactoryBase because as you said, I
do expect as well software converter to need a factory too..

As far as I can see the MediaDevice is mostly used to get the
->driver() name out and match it against a list of compatibles the
Converter-derived class has been instantiated with.

It seems to possible to make the '*media' parameter optional to most
functions that accept it, or even overload the
ConverterFactoryBase::create() and createInstance() functions to
accept instead a string that identifies the software converter version
(ie "linaro-sw-converter" in your case?).

I would go as far as wondering if insted we shouldn't make the whole
hierarchy string-based and MediaDevice completely.

The only thing we would lose for real would be

	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()) {
		LOG(Converter, Error)
			<< "No entity suitable for implementing a converter in "
			<< media->driver() << " entities list.";
		return;
	}

at construction-time. Would this be that bad ? (if we want to keep it
we can have 2 overloaded constructors, in example).

Have this options been discussed in the previous version or have you
considered them but then discarded by any chance ?

Below some drive-by comments


> ---
>  include/libcamera/internal/converter.h        |  49 +---
>  .../internal/converter/converter_v4l2_m2m.h   |   4 +-
>  include/libcamera/internal/converter_media.h  |  86 +++++++
>  include/libcamera/internal/meson.build        |   1 +
>  src/libcamera/converter.cpp                   | 191 +-------------
>  .../converter/converter_v4l2_m2m.cpp          |   4 +-
>  src/libcamera/converter_media.cpp             | 241 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/pipeline/simple/simple.cpp      |   4 +-
>  9 files changed, 337 insertions(+), 244 deletions(-)
>  create mode 100644 include/libcamera/internal/converter_media.h
>  create mode 100644 src/libcamera/converter_media.cpp
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 834ec5ab..da1f5d73 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -24,14 +24,13 @@
>  namespace libcamera {
>
>  class FrameBuffer;
> -class MediaDevice;
>  class PixelFormat;
>  struct StreamConfiguration;
>
>  class Converter
>  {
>  public:
> -	Converter(MediaDevice *media);
> +	Converter();
>  	virtual ~Converter();
>
>  	virtual int loadConfiguration(const std::string &filename) = 0;
> @@ -57,52 +56,6 @@ public:
>
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
> -
> -	const std::string &deviceNode() const { return deviceNode_; }
> -
> -private:
> -	std::string deviceNode_;
>  };
>
> -class ConverterFactoryBase
> -{
> -public:
> -	ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
> -	virtual ~ConverterFactoryBase() = default;
> -
> -	const std::vector<std::string> &compatibles() const { return compatibles_; }
> -
> -	static std::unique_ptr<Converter> create(MediaDevice *media);
> -	static std::vector<ConverterFactoryBase *> &factories();
> -	static std::vector<std::string> names();
> -
> -private:
> -	LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)
> -
> -	static void registerType(ConverterFactoryBase *factory);
> -
> -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> -
> -	std::string name_;
> -	std::vector<std::string> compatibles_;
> -};
> -
> -template<typename _Converter>
> -class ConverterFactory : public ConverterFactoryBase
> -{
> -public:
> -	ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
> -		: ConverterFactoryBase(name, compatibles)
> -	{
> -	}
> -
> -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> -	{
> -		return std::make_unique<_Converter>(media);
> -	}
> -};
> -
> -#define REGISTER_CONVERTER(name, converter, compatibles) \
> -	static ConverterFactory<converter> global_##converter##Factory(name, compatibles);
> -
>  } /* namespace libcamera */
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 815916d0..aeb8c0e9 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -20,7 +20,7 @@
>
>  #include <libcamera/pixel_format.h>
>
> -#include "libcamera/internal/converter.h"
> +#include "libcamera/internal/converter_media.h"
>
>  namespace libcamera {
>
> @@ -31,7 +31,7 @@ class SizeRange;
>  struct StreamConfiguration;
>  class V4L2M2MDevice;
>
> -class V4L2M2MConverter : public Converter
> +class V4L2M2MConverter : public ConverterMD
>  {
>  public:
>  	V4L2M2MConverter(MediaDevice *media);
> diff --git a/include/libcamera/internal/converter_media.h b/include/libcamera/internal/converter_media.h
> new file mode 100644
> index 00000000..2b56ebdb
> --- /dev/null
> +++ b/include/libcamera/internal/converter_media.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Laurent Pinchart
> + * Copyright 2022 NXP
> + *
> + * converter_media.h - Generic media device based format converter interface
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#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/internal/converter.h"

A lot of stuff (inclusions an forward declaration) are duplicated in
this and in the libcamera/internal/converter.h header. You can include
converter.h first and add only what is missing.

> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class MediaDevice;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +class ConverterMD : public Converter
> +{
> +public:
> +	ConverterMD(MediaDevice *media);
> +	~ConverterMD();
> +
> +	const std::string &deviceNode() const { return deviceNode_; }
> +
> +private:
> +	std::string deviceNode_;
> +};
> +
> +class ConverterMDFactoryBase
> +{
> +public:
> +	ConverterMDFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
> +	virtual ~ConverterMDFactoryBase() = default;
> +
> +	const std::vector<std::string> &compatibles() const { return compatibles_; }
> +
> +	static std::unique_ptr<ConverterMD> create(MediaDevice *media);
> +	static std::vector<ConverterMDFactoryBase *> &factories();
> +	static std::vector<std::string> names();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterMDFactoryBase)
> +
> +	static void registerType(ConverterMDFactoryBase *factory);
> +
> +	virtual std::unique_ptr<ConverterMD> createInstance(MediaDevice *media) const = 0;
> +
> +	std::string name_;
> +	std::vector<std::string> compatibles_;
> +};
> +
> +template<typename _ConverterMD>
> +class ConverterMDFactory : public ConverterMDFactoryBase
> +{
> +public:
> +	ConverterMDFactory(const char *name, std::initializer_list<std::string> compatibles)
> +		: ConverterMDFactoryBase(name, compatibles)
> +	{
> +	}
> +
> +	std::unique_ptr<ConverterMD> createInstance(MediaDevice *media) const override
> +	{
> +		return std::make_unique<_ConverterMD>(media);
> +	}
> +};
> +
> +#define REGISTER_CONVERTER_MD(name, converter, compatibles) \
> +	static ConverterMDFactory<converter> global_##converter##MDFactory(name, compatibles);
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 7f1f3440..e9c41346 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([
>      'control_serializer.h',
>      'control_validator.h',
>      'converter.h',
> +    'converter_media.h',
>      'delayed_controls.h',
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index fa0f1ec8..92dcdc03 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -38,26 +38,9 @@ LOG_DEFINE_CATEGORY(Converter)
>
>  /**
>   * \brief Construct a Converter instance
> - * \param[in] media The media device implementing the converter
> - *
> - * This searches for the entity implementing the data streaming function in the
> - * media graph entities and use its device node as the converter device node.
>   */
> -Converter::Converter(MediaDevice *media)
> +Converter::Converter()
>  {
> -	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()) {
> -		LOG(Converter, Error)
> -			<< "No entity suitable for implementing a converter in "
> -			<< media->driver() << " entities list.";
> -		return;
> -	}
> -
> -	deviceNode_ = (*it)->deviceNode();
>  }
>
>  Converter::~Converter()
> @@ -159,176 +142,4 @@ Converter::~Converter()
>   * \brief A signal emitted on each frame buffer completion of the output queue
>   */
>
> -/**
> - * \fn Converter::deviceNode()
> - * \brief The converter device node attribute accessor
> - * \return The converter device node string
> - */
> -
> -/**
> - * \class ConverterFactoryBase
> - * \brief Base class for converter factories
> - *
> - * The ConverterFactoryBase class is the base of all specializations of the
> - * ConverterFactory class template. It implements the factory registration,
> - * maintains a registry of factories, and provides access to the registered
> - * factories.
> - */
> -
> -/**
> - * \brief Construct a converter factory base
> - * \param[in] name Name of the converter class
> - * \param[in] compatibles Name aliases of the converter class
> - *
> - * Creating an instance of the factory base registers it with the global list of
> - * factories, accessible through the factories() function.
> - *
> - * The factory \a name is used as unique identifier. If the converter
> - * implementation fully relies on a generic framework, the name should be the
> - * same as the framework. Otherwise, if the implementation is specialized, the
> - * factory name should match the driver name implementing the function.
> - *
> - * The factory \a compatibles holds a list of driver names implementing a generic
> - * subsystem without any personalizations.
> - */
> -ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles)
> -	: name_(name), compatibles_(compatibles)
> -{
> -	registerType(this);
> -}
> -
> -/**
> - * \fn ConverterFactoryBase::compatibles()
> - * \return The names compatibles
> - */
> -
> -/**
> - * \brief Create an instance of the converter corresponding to a named factory
> - * \param[in] media Name of the factory

This clearly was wrong already, and if you shuffle code around might
be a good occasion to change it.

> - *
> - * \return A unique pointer to a new instance of the converter subclass
> - * corresponding to the named factory or one of its alias. Otherwise a null
> - * pointer if no such factory exists
> - */
> -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> -{
> -	const std::vector<ConverterFactoryBase *> &factories =
> -		ConverterFactoryBase::factories();
> -
> -	for (const ConverterFactoryBase *factory : factories) {
> -		const std::vector<std::string> &compatibles = factory->compatibles();
> -		auto it = std::find(compatibles.begin(), compatibles.end(), media->driver());
> -
> -		if (it == compatibles.end() && media->driver() != factory->name_)
> -			continue;
> -
> -		LOG(Converter, Debug)
> -			<< "Creating converter from "
> -			<< factory->name_ << " factory with "
> -			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> -
> -		std::unique_ptr<Converter> converter = factory->createInstance(media);
> -		if (converter->isValid())
> -			return converter;
> -	}
> -
> -	return nullptr;
> -}
> -
> -/**
> - * \brief Add a converter class to the registry
> - * \param[in] factory Factory to use to construct the converter class
> - *
> - * The caller is responsible to guarantee the uniqueness of the converter name.
> - */
> -void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)
> -{
> -	std::vector<ConverterFactoryBase *> &factories =
> -		ConverterFactoryBase::factories();
> -
> -	factories.push_back(factory);
> -}
> -
> -/**
> - * \brief Retrieve the list of all converter factory names
> - * \return The list of all converter factory names
> - */
> -std::vector<std::string> ConverterFactoryBase::names()
> -{
> -	std::vector<std::string> list;
> -
> -	std::vector<ConverterFactoryBase *> &factories =
> -		ConverterFactoryBase::factories();
> -
> -	for (ConverterFactoryBase *factory : factories) {
> -		list.push_back(factory->name_);
> -		for (auto alias : factory->compatibles())
> -			list.push_back(alias);
> -	}
> -
> -	return list;
> -}
> -
> -/**
> - * \brief Retrieve the list of all converter factories
> - * \return The list of converter factories
> - */
> -std::vector<ConverterFactoryBase *> &ConverterFactoryBase::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<ConverterFactoryBase *> factories;
> -	return factories;
> -}
> -
> -/**
> - * \var ConverterFactoryBase::name_
> - * \brief The name of the factory
> - */
> -
> -/**
> - * \var ConverterFactoryBase::compatibles_
> - * \brief The list holding the factory compatibles
> - */
> -
> -/**
> - * \class ConverterFactory
> - * \brief Registration of ConverterFactory classes and creation of instances
> - * \param _Converter The converter class type for this factory
> - *
> - * To facilitate discovery and instantiation of Converter classes, the
> - * ConverterFactory class implements auto-registration of converter helpers.
> - * Each Converter subclass shall register itself using the REGISTER_CONVERTER()
> - * macro, which will create a corresponding instance of a ConverterFactory
> - * subclass and register it with the static list of factories.
> - */
> -
> -/**
> - * \fn ConverterFactory::ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
> - * \brief Construct a converter factory
> - * \details \copydetails ConverterFactoryBase::ConverterFactoryBase
> - */
> -
> -/**
> - * \fn ConverterFactory::createInstance() const
> - * \brief Create an instance of the Converter corresponding to the factory
> - * \param[in] media Media device pointer
> - * \return A unique pointer to a newly constructed instance of the Converter
> - * subclass corresponding to the factory
> - */
> -
> -/**
> - * \def REGISTER_CONVERTER
> - * \brief Register a converter with the Converter factory
> - * \param[in] name Converter name used to register the class
> - * \param[in] converter Class name of Converter derived class to register
> - * \param[in] compatibles List of compatible names
> - *
> - * Register a Converter subclass with the factory and make it available to try
> - * and match converters.
> - */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 2a4d1d99..d0a5e3bf 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -194,7 +194,7 @@ void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)
>   */
>
>  V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> -	: Converter(media)
> +	: ConverterMD(media)
>  {
>  	if (deviceNode().empty())
>  		return;
> @@ -448,6 +448,6 @@ static std::initializer_list<std::string> compatibles = {
>  	"pxp",
>  };
>
> -REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, compatibles)
> +REGISTER_CONVERTER_MD("v4l2_m2m", V4L2M2MConverter, compatibles)
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/converter_media.cpp b/src/libcamera/converter_media.cpp
> new file mode 100644
> index 00000000..f5024764
> --- /dev/null
> +++ b/src/libcamera/converter_media.cpp
> @@ -0,0 +1,241 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright 2022 NXP
> + *

You copyright here and in the header file if you like to

> + * converter.cpp - Generic format converter interface
> + */
> +
> +#include "libcamera/internal/converter_media.h"
> +
> +#include <algorithm>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/media_device.h"
> +
> +#include "linux/media.h"

I know it was already like this, but I would have expected to see <>

It shouldn't make a difference though, even if this is a system-wide
header we ship our own copy and adjust the inclusion prefix precendece
to use our own version (I presume) so both "" and <> will do the same
thing ?

And, btw, this is included by media_device.h (again, not your fault)

> +
> +/**
> + * \file internal/converter_media.h
> + * \brief Abstract media device based converter
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Converter)

Maybe ConverterMD ?

> +
> +/**
> + * \class ConverterMD
> + * \brief Abstract Base Class for media device based converter
> + *
> + * The ConverterMD class is an Abstract Base Class defining the interfaces of
> + * media device based converter implementations.
> + *
> + * Converters offer scaling and pixel format conversion services on an input
> + * stream. The converter can output multiple streams with individual conversion
> + * parameters from the same input stream.
> + */
> +
> +/**
> + * \brief Construct a ConverterMD instance
> + * \param[in] media The media device implementing the converter
> + *
> + * This searches for the entity implementing the data streaming function in the
> + * media graph entities and use its device node as the converter device node.
> + */
> +ConverterMD::ConverterMD(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()) {
> +		LOG(Converter, Error)
> +			<< "No entity suitable for implementing a converter in "
> +			<< media->driver() << " entities list.";
> +		return;
> +	}
> +
> +	deviceNode_ = (*it)->deviceNode();
> +}
> +
> +ConverterMD::~ConverterMD()
> +{
> +}
> +
> +/**
> + * \fn ConverterMD::deviceNode()
> + * \brief The converter device node attribute accessor
> + * \return The converter device node string
> + */
> +
> +/**
> + * \class ConverterMDFactoryBase
> + * \brief Base class for media device based converter factories
> + *
> + * The ConverterMDFactoryBase class is the base of all specializations of the
> + * ConverterMDFactory class template. It implements the factory registration,
> + * maintains a registry of factories, and provides access to the registered
> + * factories.
> + */
> +
> +/**
> + * \brief Construct a media device based converter factory base
> + * \param[in] name Name of the converter class
> + * \param[in] compatibles Name aliases of the converter class
> + *
> + * Creating an instance of the factory base registers it with the global list of
> + * factories, accessible through the factories() function.
> + *
> + * The factory \a name is used as unique identifier. If the converter
> + * implementation fully relies on a generic framework, the name should be the
> + * same as the framework. Otherwise, if the implementation is specialized, the
> + * factory name should match the driver name implementing the function.
> + *
> + * The factory \a compatibles holds a list of driver names implementing a generic
> + * subsystem without any personalizations.
> + */
> +ConverterMDFactoryBase::ConverterMDFactoryBase(const std::string name, std::initializer_list<std::string> compatibles)
> +	: name_(name), compatibles_(compatibles)
> +{
> +	registerType(this);
> +}
> +
> +/**
> + * \fn ConverterMDFactoryBase::compatibles()
> + * \return The names compatibles
> + */
> +
> +/**
> + * \brief Create an instance of the converter corresponding to a named factory
> + * \param[in] media Name of the factory
> + *
> + * \return A unique pointer to a new instance of the media device based
> + * converter subclass corresponding to the named factory or one of its alias.
> + * Otherwise a null pointer if no such factory exists.
> + */
> +std::unique_ptr<ConverterMD> ConverterMDFactoryBase::create(MediaDevice *media)
> +{
> +	const std::vector<ConverterMDFactoryBase *> &factories =
> +		ConverterMDFactoryBase::factories();
> +
> +	for (const ConverterMDFactoryBase *factory : factories) {
> +		const std::vector<std::string> &compatibles = factory->compatibles();
> +		auto it = std::find(compatibles.begin(), compatibles.end(), media->driver());
> +
> +		if (it == compatibles.end() && media->driver() != factory->name_)
> +			continue;
> +
> +		LOG(Converter, Debug)
> +			<< "Creating converter from "
> +			<< factory->name_ << " factory with "
> +			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> +
> +		std::unique_ptr<ConverterMD> converter = factory->createInstance(media);
> +		if (converter->isValid())
> +			return converter;
> +	}
> +
> +	return nullptr;
> +}
> +
> +/**
> + * \brief Add a media device based converter class to the registry
> + * \param[in] factory Factory to use to construct the media device based
> + * converter class
> + *
> + * The caller is responsible to guarantee the uniqueness of the converter name.
> + */
> +void ConverterMDFactoryBase::registerType(ConverterMDFactoryBase *factory)
> +{
> +	std::vector<ConverterMDFactoryBase *> &factories =
> +		ConverterMDFactoryBase::factories();
> +
> +	factories.push_back(factory);
> +}
> +
> +/**
> + * \brief Retrieve the list of all media device based converter factory names
> + * \return The list of all media device based converter factory names
> + */
> +std::vector<std::string> ConverterMDFactoryBase::names()
> +{
> +	std::vector<std::string> list;
> +
> +	std::vector<ConverterMDFactoryBase *> &factories =
> +		ConverterMDFactoryBase::factories();
> +
> +	for (ConverterMDFactoryBase *factory : factories) {
> +		list.push_back(factory->name_);
> +		for (auto alias : factory->compatibles())
> +			list.push_back(alias);
> +	}
> +
> +	return list;
> +}
> +
> +/**
> + * \brief Retrieve the list of all media device based converter factories
> + * \return The list of media device based converter factories
> + */
> +std::vector<ConverterMDFactoryBase *> &ConverterMDFactoryBase::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<ConverterMDFactoryBase *> factories;
> +	return factories;
> +}
> +
> +/**
> + * \var ConverterMDFactoryBase::name_
> + * \brief The name of the factory
> + */
> +
> +/**
> + * \var ConverterMDFactoryBase::compatibles_
> + * \brief The list holding the factory compatibles
> + */
> +
> +/**
> + * \class ConverterMDFactory
> + * \brief Registration of ConverterMDFactory classes and creation of instances
> + * \param _Converter The converter class type for this factory
> + *
> + * To facilitate discovery and instantiation of ConverterMD classes, the
> + * ConverterMDFactory class implements auto-registration of converter helpers.
> + * Each ConverterMD subclass shall register itself using the
> + * REGISTER_CONVERTER() macro, which will create a corresponding instance of a
> + * ConverterMDFactory subclass and register it with the static list of
> + * factories.
> + */
> +
> +/**
> + * \fn ConverterMDFactory::ConverterMDFactory(const char *name, std::initializer_list<std::string> compatibles)
> + * \brief Construct a media device converter factory
> + * \details \copydetails ConverterMDFactoryBase::ConverterMDFactoryBase
> + */
> +
> +/**
> + * \fn ConverterMDFactory::createInstance() const
> + * \brief Create an instance of the ConverterMD corresponding to the factory
> + * \param[in] media Media device pointer
> + * \return A unique pointer to a newly constructed instance of the ConverterMD
> + * subclass corresponding to the factory
> + */
> +
> +/**
> + * \def REGISTER_CONVERTER_MD
> + * \brief Register a media device based converter with the ConverterMD factory
> + * \param[in] name ConverterMD name used to register the class
> + * \param[in] converter Class name of ConverterMD derived class to register
> + * \param[in] compatibles List of compatible names
> + *
> + * Register a ConverterMD subclass with the factory and make it available to try
> + * and match converters.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b24f8296..af8b1812 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -14,6 +14,7 @@ libcamera_sources = files([
>      'control_serializer.cpp',
>      'control_validator.cpp',
>      'converter.cpp',
> +    'converter_media.cpp',
>      'delayed_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 05ba76bc..f0622a74 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -30,7 +30,7 @@
>
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_sensor.h"
> -#include "libcamera/internal/converter.h"
> +#include "libcamera/internal/converter_media.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
> @@ -497,7 +497,7 @@ int SimpleCameraData::init()
>  	/* Open the converter, if any. */
>  	MediaDevice *converter = pipe->converter();
>  	if (converter) {
> -		converter_ = ConverterFactoryBase::create(converter);
> +		converter_ = ConverterMDFactoryBase::create(converter);
>  		if (!converter_) {
>  			LOG(SimplePipeline, Warning)
>  				<< "Failed to create converter, disabling format conversion";
> --
> 2.34.1
>

Hi Jacopo,

Thank you for your review!

On 11.09.2023 18:55, Jacopo Mondi wrote:
> Hi Andrey
> 
> On Sun, Sep 10, 2023 at 08:50:25PM +0300, Andrey Konovalov via libcamera-devel wrote:
>> Make Converter a bit more generic by not requiring it to use a media device.
>> For this split out ConverterMD from Converter class, and rename
>> ConverterFactoryBase / ConverterFactory to ConverterMDFactoryBase /
>> ConverterMDFactory for consistency.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> I understand where this is going, but before going into a full review
> I would like to explore the possibility of generalizing the existing
> hierarchy before splitting it in media and non-media versions, which
> would require re-defining another FactoryBase because as you said, I
> do expect as well software converter to need a factory too..
> 
> As far as I can see the MediaDevice is mostly used to get the
> ->driver() name out and match it against a list of compatibles the
> Converter-derived class has been instantiated with.
> 
> It seems to possible to make the '*media' parameter optional to most
> functions that accept it, or even overload the
> ConverterFactoryBase::create() and createInstance() functions to
> accept instead a string that identifies the software converter version
> (ie "linaro-sw-converter" in your case?).
> 
> I would go as far as wondering if insted we shouldn't make the whole
> hierarchy string-based and MediaDevice completely.

... remove MediaDevice completely? This might be an option.
This would definitely work for software based convertor.
But when I tried to follow along the usage of MediaDevice in Converter
and how Converter is handled in the pipeline handler, I got an
impression of MediaDevice dependency going down to DeviceEnumerator which
is critical for media (now) and non-media USB (when implemented) devices,
and is irrelevant to "software based" "devices".
Let me double check, and experiment with the code a bit.

> The only thing we would lose for real would be
> 
> 	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()) {
> 		LOG(Converter, Error)
> 			<< "No entity suitable for implementing a converter in "
> 			<< media->driver() << " entities list.";
> 		return;
> 	}
> 
> at construction-time. Would this be that bad ? (if we want to keep it
> we can have 2 overloaded constructors, in example).

... I would expect 2 overloaded constructors to be needed.

> Have this options been discussed in the previous version or have you
> considered them but then discarded by any chance ?

This hasn't been discussed (the previous version used a quick hack instead of
this patch, so there was little to discuss I guess).




> Below some drive-by comments
> 
> 
>> ---
>>   include/libcamera/internal/converter.h        |  49 +---
>>   .../internal/converter/converter_v4l2_m2m.h   |   4 +-
>>   include/libcamera/internal/converter_media.h  |  86 +++++++
>>   include/libcamera/internal/meson.build        |   1 +
>>   src/libcamera/converter.cpp                   | 191 +-------------
>>   .../converter/converter_v4l2_m2m.cpp          |   4 +-
>>   src/libcamera/converter_media.cpp             | 241 ++++++++++++++++++
>>   src/libcamera/meson.build                     |   1 +
>>   src/libcamera/pipeline/simple/simple.cpp      |   4 +-
>>   9 files changed, 337 insertions(+), 244 deletions(-)
>>   create mode 100644 include/libcamera/internal/converter_media.h
>>   create mode 100644 src/libcamera/converter_media.cpp
>>
>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
>> index 834ec5ab..da1f5d73 100644
>> --- a/include/libcamera/internal/converter.h
>> +++ b/include/libcamera/internal/converter.h
>> @@ -24,14 +24,13 @@
>>   namespace libcamera {
>>
>>   class FrameBuffer;
>> -class MediaDevice;
>>   class PixelFormat;
>>   struct StreamConfiguration;
>>
>>   class Converter
>>   {
>>   public:
>> -	Converter(MediaDevice *media);
>> +	Converter();
>>   	virtual ~Converter();
>>
>>   	virtual int loadConfiguration(const std::string &filename) = 0;
>> @@ -57,52 +56,6 @@ public:
>>
>>   	Signal<FrameBuffer *> inputBufferReady;
>>   	Signal<FrameBuffer *> outputBufferReady;
>> -
>> -	const std::string &deviceNode() const { return deviceNode_; }
>> -
>> -private:
>> -	std::string deviceNode_;
>>   };
>>
>> -class ConverterFactoryBase
>> -{
>> -public:
>> -	ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
>> -	virtual ~ConverterFactoryBase() = default;
>> -
>> -	const std::vector<std::string> &compatibles() const { return compatibles_; }
>> -
>> -	static std::unique_ptr<Converter> create(MediaDevice *media);
>> -	static std::vector<ConverterFactoryBase *> &factories();
>> -	static std::vector<std::string> names();
>> -
>> -private:
>> -	LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)
>> -
>> -	static void registerType(ConverterFactoryBase *factory);
>> -
>> -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
>> -
>> -	std::string name_;
>> -	std::vector<std::string> compatibles_;
>> -};
>> -
>> -template<typename _Converter>
>> -class ConverterFactory : public ConverterFactoryBase
>> -{
>> -public:
>> -	ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
>> -		: ConverterFactoryBase(name, compatibles)
>> -	{
>> -	}
>> -
>> -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
>> -	{
>> -		return std::make_unique<_Converter>(media);
>> -	}
>> -};
>> -
>> -#define REGISTER_CONVERTER(name, converter, compatibles) \
>> -	static ConverterFactory<converter> global_##converter##Factory(name, compatibles);
>> -
>>   } /* namespace libcamera */
>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> index 815916d0..aeb8c0e9 100644
>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> @@ -20,7 +20,7 @@
>>
>>   #include <libcamera/pixel_format.h>
>>
>> -#include "libcamera/internal/converter.h"
>> +#include "libcamera/internal/converter_media.h"
>>
>>   namespace libcamera {
>>
>> @@ -31,7 +31,7 @@ class SizeRange;
>>   struct StreamConfiguration;
>>   class V4L2M2MDevice;
>>
>> -class V4L2M2MConverter : public Converter
>> +class V4L2M2MConverter : public ConverterMD
>>   {
>>   public:
>>   	V4L2M2MConverter(MediaDevice *media);
>> diff --git a/include/libcamera/internal/converter_media.h b/include/libcamera/internal/converter_media.h
>> new file mode 100644
>> index 00000000..2b56ebdb
>> --- /dev/null
>> +++ b/include/libcamera/internal/converter_media.h
>> @@ -0,0 +1,86 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Laurent Pinchart
>> + * Copyright 2022 NXP
>> + *
>> + * converter_media.h - Generic media device based format converter interface
>> + */
>> +
>> +#pragma once
>> +
>> +#include <functional>
>> +#include <initializer_list>
>> +#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/internal/converter.h"
> 
> A lot of stuff (inclusions an forward declaration) are duplicated in
> this and in the libcamera/internal/converter.h header. You can include
> converter.h first and add only what is missing.

Right. I'll fix that.

>> +
>> +namespace libcamera {
>> +
>> +class FrameBuffer;
>> +class MediaDevice;
>> +class PixelFormat;
>> +struct StreamConfiguration;
>> +
>> +class ConverterMD : public Converter
>> +{
>> +public:
>> +	ConverterMD(MediaDevice *media);
>> +	~ConverterMD();
>> +
>> +	const std::string &deviceNode() const { return deviceNode_; }
>> +
>> +private:
>> +	std::string deviceNode_;
>> +};
>> +
>> +class ConverterMDFactoryBase
>> +{
>> +public:
>> +	ConverterMDFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
>> +	virtual ~ConverterMDFactoryBase() = default;
>> +
>> +	const std::vector<std::string> &compatibles() const { return compatibles_; }
>> +
>> +	static std::unique_ptr<ConverterMD> create(MediaDevice *media);
>> +	static std::vector<ConverterMDFactoryBase *> &factories();
>> +	static std::vector<std::string> names();
>> +
>> +private:
>> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterMDFactoryBase)
>> +
>> +	static void registerType(ConverterMDFactoryBase *factory);
>> +
>> +	virtual std::unique_ptr<ConverterMD> createInstance(MediaDevice *media) const = 0;
>> +
>> +	std::string name_;
>> +	std::vector<std::string> compatibles_;
>> +};
>> +
>> +template<typename _ConverterMD>
>> +class ConverterMDFactory : public ConverterMDFactoryBase
>> +{
>> +public:
>> +	ConverterMDFactory(const char *name, std::initializer_list<std::string> compatibles)
>> +		: ConverterMDFactoryBase(name, compatibles)
>> +	{
>> +	}
>> +
>> +	std::unique_ptr<ConverterMD> createInstance(MediaDevice *media) const override
>> +	{
>> +		return std::make_unique<_ConverterMD>(media);
>> +	}
>> +};
>> +
>> +#define REGISTER_CONVERTER_MD(name, converter, compatibles) \
>> +	static ConverterMDFactory<converter> global_##converter##MDFactory(name, compatibles);
>> +
>> +} /* namespace libcamera */
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 7f1f3440..e9c41346 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([
>>       'control_serializer.h',
>>       'control_validator.h',
>>       'converter.h',
>> +    'converter_media.h',
>>       'delayed_controls.h',
>>       'device_enumerator.h',
>>       'device_enumerator_sysfs.h',
>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>> index fa0f1ec8..92dcdc03 100644
>> --- a/src/libcamera/converter.cpp
>> +++ b/src/libcamera/converter.cpp
>> @@ -38,26 +38,9 @@ LOG_DEFINE_CATEGORY(Converter)
>>
>>   /**
>>    * \brief Construct a Converter instance
>> - * \param[in] media The media device implementing the converter
>> - *
>> - * This searches for the entity implementing the data streaming function in the
>> - * media graph entities and use its device node as the converter device node.
>>    */
>> -Converter::Converter(MediaDevice *media)
>> +Converter::Converter()
>>   {
>> -	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()) {
>> -		LOG(Converter, Error)
>> -			<< "No entity suitable for implementing a converter in "
>> -			<< media->driver() << " entities list.";
>> -		return;
>> -	}
>> -
>> -	deviceNode_ = (*it)->deviceNode();
>>   }
>>
>>   Converter::~Converter()
>> @@ -159,176 +142,4 @@ Converter::~Converter()
>>    * \brief A signal emitted on each frame buffer completion of the output queue
>>    */
>>
>> -/**
>> - * \fn Converter::deviceNode()
>> - * \brief The converter device node attribute accessor
>> - * \return The converter device node string
>> - */
>> -
>> -/**
>> - * \class ConverterFactoryBase
>> - * \brief Base class for converter factories
>> - *
>> - * The ConverterFactoryBase class is the base of all specializations of the
>> - * ConverterFactory class template. It implements the factory registration,
>> - * maintains a registry of factories, and provides access to the registered
>> - * factories.
>> - */
>> -
>> -/**
>> - * \brief Construct a converter factory base
>> - * \param[in] name Name of the converter class
>> - * \param[in] compatibles Name aliases of the converter class
>> - *
>> - * Creating an instance of the factory base registers it with the global list of
>> - * factories, accessible through the factories() function.
>> - *
>> - * The factory \a name is used as unique identifier. If the converter
>> - * implementation fully relies on a generic framework, the name should be the
>> - * same as the framework. Otherwise, if the implementation is specialized, the
>> - * factory name should match the driver name implementing the function.
>> - *
>> - * The factory \a compatibles holds a list of driver names implementing a generic
>> - * subsystem without any personalizations.
>> - */
>> -ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles)
>> -	: name_(name), compatibles_(compatibles)
>> -{
>> -	registerType(this);
>> -}
>> -
>> -/**
>> - * \fn ConverterFactoryBase::compatibles()
>> - * \return The names compatibles
>> - */
>> -
>> -/**
>> - * \brief Create an instance of the converter corresponding to a named factory
>> - * \param[in] media Name of the factory
> 
> This clearly was wrong already, and if you shuffle code around might
> be a good occasion to change it.

OK

>> - *
>> - * \return A unique pointer to a new instance of the converter subclass
>> - * corresponding to the named factory or one of its alias. Otherwise a null
>> - * pointer if no such factory exists
>> - */
>> -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>> -{
>> -	const std::vector<ConverterFactoryBase *> &factories =
>> -		ConverterFactoryBase::factories();
>> -
>> -	for (const ConverterFactoryBase *factory : factories) {
>> -		const std::vector<std::string> &compatibles = factory->compatibles();
>> -		auto it = std::find(compatibles.begin(), compatibles.end(), media->driver());
>> -
>> -		if (it == compatibles.end() && media->driver() != factory->name_)
>> -			continue;
>> -
>> -		LOG(Converter, Debug)
>> -			<< "Creating converter from "
>> -			<< factory->name_ << " factory with "
>> -			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
>> -
>> -		std::unique_ptr<Converter> converter = factory->createInstance(media);
>> -		if (converter->isValid())
>> -			return converter;
>> -	}
>> -
>> -	return nullptr;
>> -}
>> -
>> -/**
>> - * \brief Add a converter class to the registry
>> - * \param[in] factory Factory to use to construct the converter class
>> - *
>> - * The caller is responsible to guarantee the uniqueness of the converter name.
>> - */
>> -void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)
>> -{
>> -	std::vector<ConverterFactoryBase *> &factories =
>> -		ConverterFactoryBase::factories();
>> -
>> -	factories.push_back(factory);
>> -}
>> -
>> -/**
>> - * \brief Retrieve the list of all converter factory names
>> - * \return The list of all converter factory names
>> - */
>> -std::vector<std::string> ConverterFactoryBase::names()
>> -{
>> -	std::vector<std::string> list;
>> -
>> -	std::vector<ConverterFactoryBase *> &factories =
>> -		ConverterFactoryBase::factories();
>> -
>> -	for (ConverterFactoryBase *factory : factories) {
>> -		list.push_back(factory->name_);
>> -		for (auto alias : factory->compatibles())
>> -			list.push_back(alias);
>> -	}
>> -
>> -	return list;
>> -}
>> -
>> -/**
>> - * \brief Retrieve the list of all converter factories
>> - * \return The list of converter factories
>> - */
>> -std::vector<ConverterFactoryBase *> &ConverterFactoryBase::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<ConverterFactoryBase *> factories;
>> -	return factories;
>> -}
>> -
>> -/**
>> - * \var ConverterFactoryBase::name_
>> - * \brief The name of the factory
>> - */
>> -
>> -/**
>> - * \var ConverterFactoryBase::compatibles_
>> - * \brief The list holding the factory compatibles
>> - */
>> -
>> -/**
>> - * \class ConverterFactory
>> - * \brief Registration of ConverterFactory classes and creation of instances
>> - * \param _Converter The converter class type for this factory
>> - *
>> - * To facilitate discovery and instantiation of Converter classes, the
>> - * ConverterFactory class implements auto-registration of converter helpers.
>> - * Each Converter subclass shall register itself using the REGISTER_CONVERTER()
>> - * macro, which will create a corresponding instance of a ConverterFactory
>> - * subclass and register it with the static list of factories.
>> - */
>> -
>> -/**
>> - * \fn ConverterFactory::ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
>> - * \brief Construct a converter factory
>> - * \details \copydetails ConverterFactoryBase::ConverterFactoryBase
>> - */
>> -
>> -/**
>> - * \fn ConverterFactory::createInstance() const
>> - * \brief Create an instance of the Converter corresponding to the factory
>> - * \param[in] media Media device pointer
>> - * \return A unique pointer to a newly constructed instance of the Converter
>> - * subclass corresponding to the factory
>> - */
>> -
>> -/**
>> - * \def REGISTER_CONVERTER
>> - * \brief Register a converter with the Converter factory
>> - * \param[in] name Converter name used to register the class
>> - * \param[in] converter Class name of Converter derived class to register
>> - * \param[in] compatibles List of compatible names
>> - *
>> - * Register a Converter subclass with the factory and make it available to try
>> - * and match converters.
>> - */
>> -
>>   } /* namespace libcamera */
>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> index 2a4d1d99..d0a5e3bf 100644
>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> @@ -194,7 +194,7 @@ void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)
>>    */
>>
>>   V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
>> -	: Converter(media)
>> +	: ConverterMD(media)
>>   {
>>   	if (deviceNode().empty())
>>   		return;
>> @@ -448,6 +448,6 @@ static std::initializer_list<std::string> compatibles = {
>>   	"pxp",
>>   };
>>
>> -REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, compatibles)
>> +REGISTER_CONVERTER_MD("v4l2_m2m", V4L2M2MConverter, compatibles)
>>
>>   } /* namespace libcamera */
>> diff --git a/src/libcamera/converter_media.cpp b/src/libcamera/converter_media.cpp
>> new file mode 100644
>> index 00000000..f5024764
>> --- /dev/null
>> +++ b/src/libcamera/converter_media.cpp
>> @@ -0,0 +1,241 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright 2022 NXP
>> + *
> 
> You copyright here and in the header file if you like to

I thought it is not worth it as this is by 99% moving the existing
code between two cpp and two header files.

>> + * converter.cpp - Generic format converter interface
>> + */
>> +
>> +#include "libcamera/internal/converter_media.h"
>> +
>> +#include <algorithm>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include "libcamera/internal/media_device.h"
>> +
>> +#include "linux/media.h"
> 
> I know it was already like this, but I would have expected to see <>
> 
> It shouldn't make a difference though, even if this is a system-wide
> header we ship our own copy and adjust the inclusion prefix precendece
> to use our own version (I presume) so both "" and <> will do the same
> thing ?
> 
> And, btw, this is included by media_device.h (again, not your fault)

OK, will sort it out.

>> +
>> +/**
>> + * \file internal/converter_media.h
>> + * \brief Abstract media device based converter
>> + */
>> +
>> +namespace libcamera {
>> +
>> +LOG_DECLARE_CATEGORY(Converter)
> 
> Maybe ConverterMD ?

Initially I added the ConverterMD category but then decided that single
Converter category might be used by both the media device, and software
converters? I'll think again.

Thanks,
Andrey

>> +
>> +/**
>> + * \class ConverterMD
>> + * \brief Abstract Base Class for media device based converter
>> + *
>> + * The ConverterMD class is an Abstract Base Class defining the interfaces of
>> + * media device based converter implementations.
>> + *
>> + * Converters offer scaling and pixel format conversion services on an input
>> + * stream. The converter can output multiple streams with individual conversion
>> + * parameters from the same input stream.
>> + */
>> +
>> +/**
>> + * \brief Construct a ConverterMD instance
>> + * \param[in] media The media device implementing the converter
>> + *
>> + * This searches for the entity implementing the data streaming function in the
>> + * media graph entities and use its device node as the converter device node.
>> + */
>> +ConverterMD::ConverterMD(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()) {
>> +		LOG(Converter, Error)
>> +			<< "No entity suitable for implementing a converter in "
>> +			<< media->driver() << " entities list.";
>> +		return;
>> +	}
>> +
>> +	deviceNode_ = (*it)->deviceNode();
>> +}
>> +
>> +ConverterMD::~ConverterMD()
>> +{
>> +}
>> +
>> +/**
>> + * \fn ConverterMD::deviceNode()
>> + * \brief The converter device node attribute accessor
>> + * \return The converter device node string
>> + */
>> +
>> +/**
>> + * \class ConverterMDFactoryBase
>> + * \brief Base class for media device based converter factories
>> + *
>> + * The ConverterMDFactoryBase class is the base of all specializations of the
>> + * ConverterMDFactory class template. It implements the factory registration,
>> + * maintains a registry of factories, and provides access to the registered
>> + * factories.
>> + */
>> +
>> +/**
>> + * \brief Construct a media device based converter factory base
>> + * \param[in] name Name of the converter class
>> + * \param[in] compatibles Name aliases of the converter class
>> + *
>> + * Creating an instance of the factory base registers it with the global list of
>> + * factories, accessible through the factories() function.
>> + *
>> + * The factory \a name is used as unique identifier. If the converter
>> + * implementation fully relies on a generic framework, the name should be the
>> + * same as the framework. Otherwise, if the implementation is specialized, the
>> + * factory name should match the driver name implementing the function.
>> + *
>> + * The factory \a compatibles holds a list of driver names implementing a generic
>> + * subsystem without any personalizations.
>> + */
>> +ConverterMDFactoryBase::ConverterMDFactoryBase(const std::string name, std::initializer_list<std::string> compatibles)
>> +	: name_(name), compatibles_(compatibles)
>> +{
>> +	registerType(this);
>> +}
>> +
>> +/**
>> + * \fn ConverterMDFactoryBase::compatibles()
>> + * \return The names compatibles
>> + */
>> +
>> +/**
>> + * \brief Create an instance of the converter corresponding to a named factory
>> + * \param[in] media Name of the factory
>> + *
>> + * \return A unique pointer to a new instance of the media device based
>> + * converter subclass corresponding to the named factory or one of its alias.
>> + * Otherwise a null pointer if no such factory exists.
>> + */
>> +std::unique_ptr<ConverterMD> ConverterMDFactoryBase::create(MediaDevice *media)
>> +{
>> +	const std::vector<ConverterMDFactoryBase *> &factories =
>> +		ConverterMDFactoryBase::factories();
>> +
>> +	for (const ConverterMDFactoryBase *factory : factories) {
>> +		const std::vector<std::string> &compatibles = factory->compatibles();
>> +		auto it = std::find(compatibles.begin(), compatibles.end(), media->driver());
>> +
>> +		if (it == compatibles.end() && media->driver() != factory->name_)
>> +			continue;
>> +
>> +		LOG(Converter, Debug)
>> +			<< "Creating converter from "
>> +			<< factory->name_ << " factory with "
>> +			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
>> +
>> +		std::unique_ptr<ConverterMD> converter = factory->createInstance(media);
>> +		if (converter->isValid())
>> +			return converter;
>> +	}
>> +
>> +	return nullptr;
>> +}
>> +
>> +/**
>> + * \brief Add a media device based converter class to the registry
>> + * \param[in] factory Factory to use to construct the media device based
>> + * converter class
>> + *
>> + * The caller is responsible to guarantee the uniqueness of the converter name.
>> + */
>> +void ConverterMDFactoryBase::registerType(ConverterMDFactoryBase *factory)
>> +{
>> +	std::vector<ConverterMDFactoryBase *> &factories =
>> +		ConverterMDFactoryBase::factories();
>> +
>> +	factories.push_back(factory);
>> +}
>> +
>> +/**
>> + * \brief Retrieve the list of all media device based converter factory names
>> + * \return The list of all media device based converter factory names
>> + */
>> +std::vector<std::string> ConverterMDFactoryBase::names()
>> +{
>> +	std::vector<std::string> list;
>> +
>> +	std::vector<ConverterMDFactoryBase *> &factories =
>> +		ConverterMDFactoryBase::factories();
>> +
>> +	for (ConverterMDFactoryBase *factory : factories) {
>> +		list.push_back(factory->name_);
>> +		for (auto alias : factory->compatibles())
>> +			list.push_back(alias);
>> +	}
>> +
>> +	return list;
>> +}
>> +
>> +/**
>> + * \brief Retrieve the list of all media device based converter factories
>> + * \return The list of media device based converter factories
>> + */
>> +std::vector<ConverterMDFactoryBase *> &ConverterMDFactoryBase::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<ConverterMDFactoryBase *> factories;
>> +	return factories;
>> +}
>> +
>> +/**
>> + * \var ConverterMDFactoryBase::name_
>> + * \brief The name of the factory
>> + */
>> +
>> +/**
>> + * \var ConverterMDFactoryBase::compatibles_
>> + * \brief The list holding the factory compatibles
>> + */
>> +
>> +/**
>> + * \class ConverterMDFactory
>> + * \brief Registration of ConverterMDFactory classes and creation of instances
>> + * \param _Converter The converter class type for this factory
>> + *
>> + * To facilitate discovery and instantiation of ConverterMD classes, the
>> + * ConverterMDFactory class implements auto-registration of converter helpers.
>> + * Each ConverterMD subclass shall register itself using the
>> + * REGISTER_CONVERTER() macro, which will create a corresponding instance of a
>> + * ConverterMDFactory subclass and register it with the static list of
>> + * factories.
>> + */
>> +
>> +/**
>> + * \fn ConverterMDFactory::ConverterMDFactory(const char *name, std::initializer_list<std::string> compatibles)
>> + * \brief Construct a media device converter factory
>> + * \details \copydetails ConverterMDFactoryBase::ConverterMDFactoryBase
>> + */
>> +
>> +/**
>> + * \fn ConverterMDFactory::createInstance() const
>> + * \brief Create an instance of the ConverterMD corresponding to the factory
>> + * \param[in] media Media device pointer
>> + * \return A unique pointer to a newly constructed instance of the ConverterMD
>> + * subclass corresponding to the factory
>> + */
>> +
>> +/**
>> + * \def REGISTER_CONVERTER_MD
>> + * \brief Register a media device based converter with the ConverterMD factory
>> + * \param[in] name ConverterMD name used to register the class
>> + * \param[in] converter Class name of ConverterMD derived class to register
>> + * \param[in] compatibles List of compatible names
>> + *
>> + * Register a ConverterMD subclass with the factory and make it available to try
>> + * and match converters.
>> + */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index b24f8296..af8b1812 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -14,6 +14,7 @@ libcamera_sources = files([
>>       'control_serializer.cpp',
>>       'control_validator.cpp',
>>       'converter.cpp',
>> +    'converter_media.cpp',
>>       'delayed_controls.cpp',
>>       'device_enumerator.cpp',
>>       'device_enumerator_sysfs.cpp',
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 05ba76bc..f0622a74 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -30,7 +30,7 @@
>>
>>   #include "libcamera/internal/camera.h"
>>   #include "libcamera/internal/camera_sensor.h"
>> -#include "libcamera/internal/converter.h"
>> +#include "libcamera/internal/converter_media.h"
>>   #include "libcamera/internal/device_enumerator.h"
>>   #include "libcamera/internal/media_device.h"
>>   #include "libcamera/internal/pipeline_handler.h"
>> @@ -497,7 +497,7 @@ int SimpleCameraData::init()
>>   	/* Open the converter, if any. */
>>   	MediaDevice *converter = pipe->converter();
>>   	if (converter) {
>> -		converter_ = ConverterFactoryBase::create(converter);
>> +		converter_ = ConverterMDFactoryBase::create(converter);
>>   		if (!converter_) {
>>   			LOG(SimplePipeline, Warning)
>>   				<< "Failed to create converter, disabling format conversion";
>> --
>> 2.34.1
>>