[libcamera-devel,v2,2/4] libcamera: converter: make using MediaDevice optional for the Converter

19062 diff mbox series

Make Converter a bit more generic by making pointer to MediaDevice
an optional argument for Converter::Converter(),
ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),
and ConverterFactory::createInstance() functions.

Instead of the MediaDevice driver name, use a generic string to match
against the convertor factory name and its compatibles list. For
MediaDevice based converters this string will be the MediaDevice driver
name as before.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 include/libcamera/internal/converter.h   |  9 ++--
 src/libcamera/converter.cpp              | 53 ++++++++++++++----------
 src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----
 3 files changed, 60 insertions(+), 35 deletions(-)

Hi Andrey

On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:
> Make Converter a bit more generic by making pointer to MediaDevice
> an optional argument for Converter::Converter(),
> ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),
> and ConverterFactory::createInstance() functions.

.. to prepare for adding support for coverters not backed by a media
device.

>
> Instead of the MediaDevice driver name, use a generic string to match
> against the convertor factory name and its compatibles list. For

s/convertor/converter

> MediaDevice based converters this string will be the MediaDevice driver
> name as before.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  include/libcamera/internal/converter.h   |  9 ++--
>  src/libcamera/converter.cpp              | 53 ++++++++++++++----------
>  src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----
>  3 files changed, 60 insertions(+), 35 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 834ec5ab..d0c70296 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (C) 2020, Laurent Pinchart
>   * Copyright 2022 NXP
> + * Copyright 2023, Linaro Ltd
>   *
>   * converter.h - Generic format converter interface
>   */
> @@ -31,7 +32,7 @@ struct StreamConfiguration;
>  class Converter
>  {
>  public:
> -	Converter(MediaDevice *media);
> +	Converter(MediaDevice *media = nullptr);
>  	virtual ~Converter();
>
>  	virtual int loadConfiguration(const std::string &filename) = 0;
> @@ -72,7 +73,7 @@ public:
>
>  	const std::vector<std::string> &compatibles() const { return compatibles_; }
>
> -	static std::unique_ptr<Converter> create(MediaDevice *media);
> +	static std::unique_ptr<Converter> create(std::string name, MediaDevice *media = nullptr);

I'm debated. Should we overload create() instead ?

What's others opinion here ?

>  	static std::vector<ConverterFactoryBase *> &factories();
>  	static std::vector<std::string> names();
>
> @@ -81,7 +82,7 @@ private:
>
>  	static void registerType(ConverterFactoryBase *factory);
>
> -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> +	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const = 0;
>
>  	std::string name_;
>  	std::vector<std::string> compatibles_;
> @@ -96,7 +97,7 @@ public:
>  	{
>  	}
>
> -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> +	std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const override
>  	{
>  		return std::make_unique<_Converter>(media);
>  	}
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 466f8421..5070eabc 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
>   * Copyright 2022 NXP
> + * Copyright 2023 Linaro Ltd
>   *
>   * converter.cpp - Generic format converter interface
>   */
> @@ -13,8 +14,6 @@
>
>  #include "libcamera/internal/media_device.h"
>
> -#include "linux/media.h"
> -

unrelated to this change, isn't it ?

>  /**
>   * \file internal/converter.h
>   * \brief Abstract converter
> @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)
>
>  /**
>   * \brief Construct a Converter instance
> - * \param[in] media The media device implementing the converter
> + * \param[in] media The media device implementing the converter (optional)
>   *
>   * 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)
>  {
> -	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;
> +	if (media) {

or
        if (!media)
                return;

> +		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();
>  	}
> -
> -	deviceNode_ = (*it)->deviceNode();
>  }
>
>  Converter::~Converter()
> @@ -162,7 +163,8 @@ Converter::~Converter()
>  /**
>   * \fn Converter::deviceNode()
>   * \brief The converter device node attribute accessor
> - * \return The converter device node string
> + * \return The converter device node string. If there is no device node for
> + * the converter returnes an empty string.

s/returnes/returns/

>   */
>
>  /**
> @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
>   */
>
>  /**
> - * \brief Create an instance of the converter corresponding to the media device
> - * \param[in] media the media device to create the converter for
> + * \brief Create an instance of the converter corresponding to the converter
> + * name
> + * \param[in] name the name of the converter to create
                      ^ The

> + * \param[in] media the media device to create the converter for (optional)
> + *
> + * The converter \a name must match the name of the converter factory, or one
> + * of its compatibles.
>   *
>   * \return A unique pointer to a new instance of the converter subclass
>   * corresponding to the media device. The converter is created by the factory

You should also modify this one

> @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
>   * If the media device driver name doesn't match anything a null pointer is
>   * returned.
>   */
> -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> +std::unique_ptr<Converter> ConverterFactoryBase::create(std::string name, 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());
> +		auto it = std::find(compatibles.begin(), compatibles.end(), name);
>
> -		if (it == compatibles.end() && media->driver() != factory->name_)
> +		if (it == compatibles.end() && name != factory->name_)
>  			continue;
>
>  		LOG(Converter, Debug)
>  			<< "Creating converter from "
>  			<< factory->name_ << " factory with "
> -			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> +			<< (it == compatibles.end() ? "no" : name) << " alias.";
>
>  		std::unique_ptr<Converter> converter = factory->createInstance(media);
>  		if (converter->isValid())
> @@ -318,7 +325,7 @@ std::vector<ConverterFactoryBase *> &ConverterFactoryBase::factories()
>  /**
>   * \fn ConverterFactory::createInstance() const
>   * \brief Create an instance of the Converter corresponding to the factory
> - * \param[in] media Media device pointer
> + * \param[in] media Media device pointer (optional)
>   * \return A unique pointer to a newly constructed instance of the Converter
>   * subclass corresponding to the factory
>   */
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 05ba76bc..3f1b523a 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
>
>  class SimplePipelineHandler;
>
> +enum class ConverterFlag {
> +	NoFlag = 0,
> +	MediaDevice = (1 << 0),
> +};
> +using ConverterFlags = Flags<ConverterFlag>;
> +

Do you expect this to be used as a bitmask or a plain enum would be
enough ? Not that it makes that much difference after all..

>  struct SimplePipelineInfo {
>  	const char *driver;
>  	/*
>  	 * Each converter in the list contains the name
>  	 * and the number of streams it supports.
>  	 */
> -	std::vector<std::pair<const char *, unsigned int>> converters;
> +	std::vector<std::tuple<ConverterFlags, const char *, unsigned int>> converters;
>  };
>
>  namespace {
>
>  static const SimplePipelineInfo supportedDevices[] = {
>  	{ "dcmipp", {} },
> -	{ "imx7-csi", { { "pxp", 1 } } },
> +	{ "imx7-csi", { { ConverterFlag::MediaDevice, "pxp", 1 } } },
>  	{ "j721e-csi2rx", {} },
>  	{ "mxc-isi", {} },
>  	{ "qcom-camss", {} },
> @@ -330,6 +336,7 @@ public:
>
>  	V4L2VideoDevice *video(const MediaEntity *entity);
>  	V4L2Subdevice *subdev(const MediaEntity *entity);
> +	const char *converterName() { return converterName_; }
>  	MediaDevice *converter() { return converter_; }
>
>  protected:
> @@ -358,6 +365,7 @@ private:
>  	MediaDevice *media_;
>  	std::map<const MediaEntity *, EntityData> entities_;
>
> +	const char *converterName_;
>  	MediaDevice *converter_;
>  };
>
> @@ -496,8 +504,10 @@ int SimpleCameraData::init()
>
>  	/* Open the converter, if any. */
>  	MediaDevice *converter = pipe->converter();

you could drop this

> -	if (converter) {
> -		converter_ = ConverterFactoryBase::create(converter);
> +	const char *converterName = pipe->converterName();
> +	if (converterName) {
> +		LOG(SimplePipeline, Info) << "Creating converter '" << converterName << "'";
> +		converter_ = ConverterFactoryBase::create(std::string(converterName), converter);

And
		converter_ = ConverterFactoryBase::create(std::string(converterName),
                                                          pipe->converter());

>  		if (!converter_) {
>  			LOG(SimplePipeline, Warning)
>  				<< "Failed to create converter, disabling format conversion";
> @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  	if (!media_)
>  		return false;
>
> -	for (const auto &[name, streams] : info->converters) {
> -		DeviceMatch converterMatch(name);
> -		converter_ = acquireMediaDevice(enumerator, converterMatch);
> -		if (converter_) {
> +	for (const auto &[flags, name, streams] : info->converters) {
> +		if (flags & ConverterFlag::MediaDevice) {
> +			DeviceMatch converterMatch(name);
> +			converter_ = acquireMediaDevice(enumerator, converterMatch);
> +			if (converter_) {
> +				converterName_ = name;
> +				numStreams = streams;
> +				break;
> +			}
> +		} else {
> +			converterName_ = name;
>  			numStreams = streams;
>  			break;
>  		}

Isn't this equivalent to:

	for (const auto &[flags, name, streams] : info->converters) {
		if (flags & ConverterFlag::MediaDevice) {
			DeviceMatch converterMatch(name);
			converter_ = acquireMediaDevice(enumerator, converterMatch);
			if (!converter_) {
				LOG(SimplePipeline, Debug)
					<< "Failed to acquire converter media device";
				continue;
			}
		}

		converterName_ = name;
		numStreams = streams;
		break;
	}
?

> --
> 2.34.1
>

Hi Jacopo,

Thank you for the review!

On 21.09.2023 11:51, Jacopo Mondi wrote:
> Hi Andrey
> 
> On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:
>> Make Converter a bit more generic by making pointer to MediaDevice
>> an optional argument for Converter::Converter(),
>> ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),
>> and ConverterFactory::createInstance() functions.
> 
> .. to prepare for adding support for coverters not backed by a media
> device.

Will add that.

>>
>> Instead of the MediaDevice driver name, use a generic string to match
>> against the convertor factory name and its compatibles list. For
> 
> s/convertor/converter

Will fix.

>> MediaDevice based converters this string will be the MediaDevice driver
>> name as before.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   include/libcamera/internal/converter.h   |  9 ++--
>>   src/libcamera/converter.cpp              | 53 ++++++++++++++----------
>>   src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----
>>   3 files changed, 60 insertions(+), 35 deletions(-)
>>
>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
>> index 834ec5ab..d0c70296 100644
>> --- a/include/libcamera/internal/converter.h
>> +++ b/include/libcamera/internal/converter.h
>> @@ -2,6 +2,7 @@
>>   /*
>>    * Copyright (C) 2020, Laurent Pinchart
>>    * Copyright 2022 NXP
>> + * Copyright 2023, Linaro Ltd
>>    *
>>    * converter.h - Generic format converter interface
>>    */
>> @@ -31,7 +32,7 @@ struct StreamConfiguration;
>>   class Converter
>>   {
>>   public:
>> -	Converter(MediaDevice *media);
>> +	Converter(MediaDevice *media = nullptr);
>>   	virtual ~Converter();
>>
>>   	virtual int loadConfiguration(const std::string &filename) = 0;
>> @@ -72,7 +73,7 @@ public:
>>
>>   	const std::vector<std::string> &compatibles() const { return compatibles_; }
>>
>> -	static std::unique_ptr<Converter> create(MediaDevice *media);
>> +	static std::unique_ptr<Converter> create(std::string name, MediaDevice *media = nullptr);
> 
> I'm debated. Should we overload create() instead ?

Probably. But currently I am not sure.

> What's others opinion here ?

If there are no comments from the others I'll send the next version still using the default arg.
But we can always get back to this later (even in the next versions of this patch set).

>>   	static std::vector<ConverterFactoryBase *> &factories();
>>   	static std::vector<std::string> names();
>>
>> @@ -81,7 +82,7 @@ private:
>>
>>   	static void registerType(ConverterFactoryBase *factory);
>>
>> -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
>> +	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const = 0;
>>
>>   	std::string name_;
>>   	std::vector<std::string> compatibles_;
>> @@ -96,7 +97,7 @@ public:
>>   	{
>>   	}
>>
>> -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
>> +	std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const override
>>   	{
>>   		return std::make_unique<_Converter>(media);
>>   	}
>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>> index 466f8421..5070eabc 100644
>> --- a/src/libcamera/converter.cpp
>> +++ b/src/libcamera/converter.cpp
>> @@ -1,6 +1,7 @@
>>   /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>   /*
>>    * Copyright 2022 NXP
>> + * Copyright 2023 Linaro Ltd
>>    *
>>    * converter.cpp - Generic format converter interface
>>    */
>> @@ -13,8 +14,6 @@
>>
>>   #include "libcamera/internal/media_device.h"
>>
>> -#include "linux/media.h"
>> -
> 
> unrelated to this change, isn't it ?

I'll create a separate patch for this change.
What should I credit you with in the commit message as it was you who spotted this
issue and suggested to fix it?

>>   /**
>>    * \file internal/converter.h
>>    * \brief Abstract converter
>> @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)
>>
>>   /**
>>    * \brief Construct a Converter instance
>> - * \param[in] media The media device implementing the converter
>> + * \param[in] media The media device implementing the converter (optional)
>>    *
>>    * 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)
>>   {
>> -	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;
>> +	if (media) {
> 
> or
>          if (!media)
>                  return;

OK

>> +		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();
>>   	}
>> -
>> -	deviceNode_ = (*it)->deviceNode();
>>   }
>>
>>   Converter::~Converter()
>> @@ -162,7 +163,8 @@ Converter::~Converter()
>>   /**
>>    * \fn Converter::deviceNode()
>>    * \brief The converter device node attribute accessor
>> - * \return The converter device node string
>> + * \return The converter device node string. If there is no device node for
>> + * the converter returnes an empty string.
> 
> s/returnes/returns/

Will fix

>>    */
>>
>>   /**
>> @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
>>    */
>>
>>   /**
>> - * \brief Create an instance of the converter corresponding to the media device
>> - * \param[in] media the media device to create the converter for
>> + * \brief Create an instance of the converter corresponding to the converter
>> + * name
>> + * \param[in] name the name of the converter to create
>                        ^ The

Right. Will fix that

>> + * \param[in] media the media device to create the converter for (optional)
>> + *
>> + * The converter \a name must match the name of the converter factory, or one
>> + * of its compatibles.
>>    *
>>    * \return A unique pointer to a new instance of the converter subclass
>>    * corresponding to the media device. The converter is created by the factory
> 
> You should also modify this one

Will do

>> @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
>>    * If the media device driver name doesn't match anything a null pointer is
>>    * returned.
>>    */
>> -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>> +std::unique_ptr<Converter> ConverterFactoryBase::create(std::string name, 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());
>> +		auto it = std::find(compatibles.begin(), compatibles.end(), name);
>>
>> -		if (it == compatibles.end() && media->driver() != factory->name_)
>> +		if (it == compatibles.end() && name != factory->name_)
>>   			continue;
>>
>>   		LOG(Converter, Debug)
>>   			<< "Creating converter from "
>>   			<< factory->name_ << " factory with "
>> -			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
>> +			<< (it == compatibles.end() ? "no" : name) << " alias.";
>>
>>   		std::unique_ptr<Converter> converter = factory->createInstance(media);
>>   		if (converter->isValid())
>> @@ -318,7 +325,7 @@ std::vector<ConverterFactoryBase *> &ConverterFactoryBase::factories()
>>   /**
>>    * \fn ConverterFactory::createInstance() const
>>    * \brief Create an instance of the Converter corresponding to the factory
>> - * \param[in] media Media device pointer
>> + * \param[in] media Media device pointer (optional)
>>    * \return A unique pointer to a newly constructed instance of the Converter
>>    * subclass corresponding to the factory
>>    */
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 05ba76bc..3f1b523a 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
>>
>>   class SimplePipelineHandler;
>>
>> +enum class ConverterFlag {
>> +	NoFlag = 0,
>> +	MediaDevice = (1 << 0),
>> +};
>> +using ConverterFlags = Flags<ConverterFlag>;
>> +
> 
> Do you expect this to be used as a bitmask or a plain enum would be
> enough ? Not that it makes that much difference after all..

For now a plain enum (or even bool) would be enough.
But having a bitmask might become useful if in future some of different Converter
subclasses could share some features. And it doesn't add a lot of complexity, so
I'd probably keep it.

>>   struct SimplePipelineInfo {
>>   	const char *driver;
>>   	/*
>>   	 * Each converter in the list contains the name
>>   	 * and the number of streams it supports.
>>   	 */
>> -	std::vector<std::pair<const char *, unsigned int>> converters;
>> +	std::vector<std::tuple<ConverterFlags, const char *, unsigned int>> converters;
>>   };
>>
>>   namespace {
>>
>>   static const SimplePipelineInfo supportedDevices[] = {
>>   	{ "dcmipp", {} },
>> -	{ "imx7-csi", { { "pxp", 1 } } },
>> +	{ "imx7-csi", { { ConverterFlag::MediaDevice, "pxp", 1 } } },
>>   	{ "j721e-csi2rx", {} },
>>   	{ "mxc-isi", {} },
>>   	{ "qcom-camss", {} },
>> @@ -330,6 +336,7 @@ public:
>>
>>   	V4L2VideoDevice *video(const MediaEntity *entity);
>>   	V4L2Subdevice *subdev(const MediaEntity *entity);
>> +	const char *converterName() { return converterName_; }
>>   	MediaDevice *converter() { return converter_; }
>>
>>   protected:
>> @@ -358,6 +365,7 @@ private:
>>   	MediaDevice *media_;
>>   	std::map<const MediaEntity *, EntityData> entities_;
>>
>> +	const char *converterName_;
>>   	MediaDevice *converter_;
>>   };
>>
>> @@ -496,8 +504,10 @@ int SimpleCameraData::init()
>>
>>   	/* Open the converter, if any. */
>>   	MediaDevice *converter = pipe->converter();
> 
> you could drop this
> 
>> -	if (converter) {
>> -		converter_ = ConverterFactoryBase::create(converter);
>> +	const char *converterName = pipe->converterName();
>> +	if (converterName) {
>> +		LOG(SimplePipeline, Info) << "Creating converter '" << converterName << "'";
>> +		converter_ = ConverterFactoryBase::create(std::string(converterName), converter);
> 
> And
> 		converter_ = ConverterFactoryBase::create(std::string(converterName),
>                                                            pipe->converter());

OK, sounds good.

>>   		if (!converter_) {
>>   			LOG(SimplePipeline, Warning)
>>   				<< "Failed to create converter, disabling format conversion";
>> @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>>   	if (!media_)
>>   		return false;
>>
>> -	for (const auto &[name, streams] : info->converters) {
>> -		DeviceMatch converterMatch(name);
>> -		converter_ = acquireMediaDevice(enumerator, converterMatch);
>> -		if (converter_) {
>> +	for (const auto &[flags, name, streams] : info->converters) {
>> +		if (flags & ConverterFlag::MediaDevice) {
>> +			DeviceMatch converterMatch(name);
>> +			converter_ = acquireMediaDevice(enumerator, converterMatch);
>> +			if (converter_) {
>> +				converterName_ = name;
>> +				numStreams = streams;
>> +				break;
>> +			}
>> +		} else {
>> +			converterName_ = name;
>>   			numStreams = streams;
>>   			break;
>>   		}
> 
> Isn't this equivalent to:
> 
> 	for (const auto &[flags, name, streams] : info->converters) {
> 		if (flags & ConverterFlag::MediaDevice) {
> 			DeviceMatch converterMatch(name);
> 			converter_ = acquireMediaDevice(enumerator, converterMatch);
> 			if (!converter_) {
> 				LOG(SimplePipeline, Debug)
> 					<< "Failed to acquire converter media device";
> 				continue;
> 			}
> 		}
> 
> 		converterName_ = name;
> 		numStreams = streams;
> 		break;
> 	}
> ?

Yep, it is. Will update the patch.


Thanks,
Andrey

>> --
>> 2.34.1
>>

HI Andrey

On Thu, Sep 21, 2023 at 09:52:13PM +0300, Andrey Konovalov wrote:
> Hi Jacopo,
>
> Thank you for the review!
>
> On 21.09.2023 11:51, Jacopo Mondi wrote:
> > Hi Andrey
> >
> > On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:
> > > Make Converter a bit more generic by making pointer to MediaDevice
> > > an optional argument for Converter::Converter(),
> > > ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),
> > > and ConverterFactory::createInstance() functions.
> >
> > .. to prepare for adding support for coverters not backed by a media
> > device.
>
> Will add that.
>
> > >
> > > Instead of the MediaDevice driver name, use a generic string to match
> > > against the convertor factory name and its compatibles list. For
> >
> > s/convertor/converter
>
> Will fix.
>
> > > MediaDevice based converters this string will be the MediaDevice driver
> > > name as before.
> > >
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   include/libcamera/internal/converter.h   |  9 ++--
> > >   src/libcamera/converter.cpp              | 53 ++++++++++++++----------
> > >   src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----
> > >   3 files changed, 60 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > index 834ec5ab..d0c70296 100644
> > > --- a/include/libcamera/internal/converter.h
> > > +++ b/include/libcamera/internal/converter.h
> > > @@ -2,6 +2,7 @@
> > >   /*
> > >    * Copyright (C) 2020, Laurent Pinchart
> > >    * Copyright 2022 NXP
> > > + * Copyright 2023, Linaro Ltd
> > >    *
> > >    * converter.h - Generic format converter interface
> > >    */
> > > @@ -31,7 +32,7 @@ struct StreamConfiguration;
> > >   class Converter
> > >   {
> > >   public:
> > > -	Converter(MediaDevice *media);
> > > +	Converter(MediaDevice *media = nullptr);
> > >   	virtual ~Converter();
> > >
> > >   	virtual int loadConfiguration(const std::string &filename) = 0;
> > > @@ -72,7 +73,7 @@ public:
> > >
> > >   	const std::vector<std::string> &compatibles() const { return compatibles_; }
> > >
> > > -	static std::unique_ptr<Converter> create(MediaDevice *media);
> > > +	static std::unique_ptr<Converter> create(std::string name, MediaDevice *media = nullptr);
> >
> > I'm debated. Should we overload create() instead ?
>
> Probably. But currently I am not sure.
>
> > What's others opinion here ?
>
> If there are no comments from the others I'll send the next version still using the default arg.
> But we can always get back to this later (even in the next versions of this patch set).
>
> > >   	static std::vector<ConverterFactoryBase *> &factories();
> > >   	static std::vector<std::string> names();
> > >
> > > @@ -81,7 +82,7 @@ private:
> > >
> > >   	static void registerType(ConverterFactoryBase *factory);
> > >
> > > -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> > > +	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const = 0;
> > >
> > >   	std::string name_;
> > >   	std::vector<std::string> compatibles_;
> > > @@ -96,7 +97,7 @@ public:
> > >   	{
> > >   	}
> > >
> > > -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> > > +	std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const override
> > >   	{
> > >   		return std::make_unique<_Converter>(media);
> > >   	}
> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > index 466f8421..5070eabc 100644
> > > --- a/src/libcamera/converter.cpp
> > > +++ b/src/libcamera/converter.cpp
> > > @@ -1,6 +1,7 @@
> > >   /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >   /*
> > >    * Copyright 2022 NXP
> > > + * Copyright 2023 Linaro Ltd
> > >    *
> > >    * converter.cpp - Generic format converter interface
> > >    */
> > > @@ -13,8 +14,6 @@
> > >
> > >   #include "libcamera/internal/media_device.h"
> > >
> > > -#include "linux/media.h"
> > > -
> >
> > unrelated to this change, isn't it ?
>
> I'll create a separate patch for this change.
> What should I credit you with in the commit message as it was you who spotted this
> issue and suggested to fix it?
>

That would be a Suggested-by: tag probably, but it's not that
necessary in this case. Up to you :)

Thanks
  j

> > >   /**
> > >    * \file internal/converter.h
> > >    * \brief Abstract converter
> > > @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)
> > >
> > >   /**
> > >    * \brief Construct a Converter instance
> > > - * \param[in] media The media device implementing the converter
> > > + * \param[in] media The media device implementing the converter (optional)
> > >    *
> > >    * 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)
> > >   {
> > > -	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;
> > > +	if (media) {
> >
> > or
> >          if (!media)
> >                  return;
>
> OK
>
> > > +		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();
> > >   	}
> > > -
> > > -	deviceNode_ = (*it)->deviceNode();
> > >   }
> > >
> > >   Converter::~Converter()
> > > @@ -162,7 +163,8 @@ Converter::~Converter()
> > >   /**
> > >    * \fn Converter::deviceNode()
> > >    * \brief The converter device node attribute accessor
> > > - * \return The converter device node string
> > > + * \return The converter device node string. If there is no device node for
> > > + * the converter returnes an empty string.
> >
> > s/returnes/returns/
>
> Will fix
>
> > >    */
> > >
> > >   /**
> > > @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
> > >    */
> > >
> > >   /**
> > > - * \brief Create an instance of the converter corresponding to the media device
> > > - * \param[in] media the media device to create the converter for
> > > + * \brief Create an instance of the converter corresponding to the converter
> > > + * name
> > > + * \param[in] name the name of the converter to create
> >                        ^ The
>
> Right. Will fix that
>
> > > + * \param[in] media the media device to create the converter for (optional)
> > > + *
> > > + * The converter \a name must match the name of the converter factory, or one
> > > + * of its compatibles.
> > >    *
> > >    * \return A unique pointer to a new instance of the converter subclass
> > >    * corresponding to the media device. The converter is created by the factory
> >
> > You should also modify this one
>
> Will do
>
> > > @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
> > >    * If the media device driver name doesn't match anything a null pointer is
> > >    * returned.
> > >    */
> > > -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> > > +std::unique_ptr<Converter> ConverterFactoryBase::create(std::string name, 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());
> > > +		auto it = std::find(compatibles.begin(), compatibles.end(), name);
> > >
> > > -		if (it == compatibles.end() && media->driver() != factory->name_)
> > > +		if (it == compatibles.end() && name != factory->name_)
> > >   			continue;
> > >
> > >   		LOG(Converter, Debug)
> > >   			<< "Creating converter from "
> > >   			<< factory->name_ << " factory with "
> > > -			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> > > +			<< (it == compatibles.end() ? "no" : name) << " alias.";
> > >
> > >   		std::unique_ptr<Converter> converter = factory->createInstance(media);
> > >   		if (converter->isValid())
> > > @@ -318,7 +325,7 @@ std::vector<ConverterFactoryBase *> &ConverterFactoryBase::factories()
> > >   /**
> > >    * \fn ConverterFactory::createInstance() const
> > >    * \brief Create an instance of the Converter corresponding to the factory
> > > - * \param[in] media Media device pointer
> > > + * \param[in] media Media device pointer (optional)
> > >    * \return A unique pointer to a newly constructed instance of the Converter
> > >    * subclass corresponding to the factory
> > >    */
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 05ba76bc..3f1b523a 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> > >
> > >   class SimplePipelineHandler;
> > >
> > > +enum class ConverterFlag {
> > > +	NoFlag = 0,
> > > +	MediaDevice = (1 << 0),
> > > +};
> > > +using ConverterFlags = Flags<ConverterFlag>;
> > > +
> >
> > Do you expect this to be used as a bitmask or a plain enum would be
> > enough ? Not that it makes that much difference after all..
>
> For now a plain enum (or even bool) would be enough.
> But having a bitmask might become useful if in future some of different Converter
> subclasses could share some features. And it doesn't add a lot of complexity, so
> I'd probably keep it.
>
> > >   struct SimplePipelineInfo {
> > >   	const char *driver;
> > >   	/*
> > >   	 * Each converter in the list contains the name
> > >   	 * and the number of streams it supports.
> > >   	 */
> > > -	std::vector<std::pair<const char *, unsigned int>> converters;
> > > +	std::vector<std::tuple<ConverterFlags, const char *, unsigned int>> converters;
> > >   };
> > >
> > >   namespace {
> > >
> > >   static const SimplePipelineInfo supportedDevices[] = {
> > >   	{ "dcmipp", {} },
> > > -	{ "imx7-csi", { { "pxp", 1 } } },
> > > +	{ "imx7-csi", { { ConverterFlag::MediaDevice, "pxp", 1 } } },
> > >   	{ "j721e-csi2rx", {} },
> > >   	{ "mxc-isi", {} },
> > >   	{ "qcom-camss", {} },
> > > @@ -330,6 +336,7 @@ public:
> > >
> > >   	V4L2VideoDevice *video(const MediaEntity *entity);
> > >   	V4L2Subdevice *subdev(const MediaEntity *entity);
> > > +	const char *converterName() { return converterName_; }
> > >   	MediaDevice *converter() { return converter_; }
> > >
> > >   protected:
> > > @@ -358,6 +365,7 @@ private:
> > >   	MediaDevice *media_;
> > >   	std::map<const MediaEntity *, EntityData> entities_;
> > >
> > > +	const char *converterName_;
> > >   	MediaDevice *converter_;
> > >   };
> > >
> > > @@ -496,8 +504,10 @@ int SimpleCameraData::init()
> > >
> > >   	/* Open the converter, if any. */
> > >   	MediaDevice *converter = pipe->converter();
> >
> > you could drop this
> >
> > > -	if (converter) {
> > > -		converter_ = ConverterFactoryBase::create(converter);
> > > +	const char *converterName = pipe->converterName();
> > > +	if (converterName) {
> > > +		LOG(SimplePipeline, Info) << "Creating converter '" << converterName << "'";
> > > +		converter_ = ConverterFactoryBase::create(std::string(converterName), converter);
> >
> > And
> > 		converter_ = ConverterFactoryBase::create(std::string(converterName),
> >                                                            pipe->converter());
>
> OK, sounds good.
>
> > >   		if (!converter_) {
> > >   			LOG(SimplePipeline, Warning)
> > >   				<< "Failed to create converter, disabling format conversion";
> > > @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > >   	if (!media_)
> > >   		return false;
> > >
> > > -	for (const auto &[name, streams] : info->converters) {
> > > -		DeviceMatch converterMatch(name);
> > > -		converter_ = acquireMediaDevice(enumerator, converterMatch);
> > > -		if (converter_) {
> > > +	for (const auto &[flags, name, streams] : info->converters) {
> > > +		if (flags & ConverterFlag::MediaDevice) {
> > > +			DeviceMatch converterMatch(name);
> > > +			converter_ = acquireMediaDevice(enumerator, converterMatch);
> > > +			if (converter_) {
> > > +				converterName_ = name;
> > > +				numStreams = streams;
> > > +				break;
> > > +			}
> > > +		} else {
> > > +			converterName_ = name;
> > >   			numStreams = streams;
> > >   			break;
> > >   		}
> >
> > Isn't this equivalent to:
> >
> > 	for (const auto &[flags, name, streams] : info->converters) {
> > 		if (flags & ConverterFlag::MediaDevice) {
> > 			DeviceMatch converterMatch(name);
> > 			converter_ = acquireMediaDevice(enumerator, converterMatch);
> > 			if (!converter_) {
> > 				LOG(SimplePipeline, Debug)
> > 					<< "Failed to acquire converter media device";
> > 				continue;
> > 			}
> > 		}
> >
> > 		converterName_ = name;
> > 		numStreams = streams;
> > 		break;
> > 	}
> > ?
>
> Yep, it is. Will update the patch.
>
>
> Thanks,
> Andrey
>
> > > --
> > > 2.34.1
> > >