[RFC 2/9] fixup: Add Features at Converter registration time

[Umang Jain]

Hi Jacopo

On 17/07/24 3:39 pm, Jacopo Mondi wrote:
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>   include/libcamera/internal/converter.h | 23 +++++++++++++----------
>   src/libcamera/converter.cpp            | 10 ++++++----
>   2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 7e4783566a44..ead465170d01 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -39,7 +39,7 @@ public:
>   
>   	using Features = Flags<Feature>;
>   
> -	Converter(MediaDevice *media, Features features = Feature::None);
> +	Converter(MediaDevice *media, Features features);
>   	virtual ~Converter();
>   
>   	virtual int loadConfiguration(const std::string &filename) = 0;
> @@ -68,21 +68,22 @@ public:
>   
>   	const std::string &deviceNode() const { return deviceNode_; }
>   
> -	Features getFeatures() const { return features_; }
> +	Features features() const { return features_; }
>   
>   private:
>   	std::string deviceNode_;
> -
>   	Features features_;
>   };
>   
>   class ConverterFactoryBase
>   {
>   public:
> -	ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
> +	ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles,
> +			     Converter::Features feat);

I might have trouble understanding, but I think each compatible string 
would have a different set of features it will support no?

Isn't each string in compatibles - would be a standalone converter in 
itself?
>   	virtual ~ConverterFactoryBase() = default;
>   
>   	const std::vector<std::string> &compatibles() const { return compatibles_; }
> +	const Converter::Features features() const { return features_; }
>   
>   	static std::unique_ptr<Converter> create(MediaDevice *media);
>   	static std::vector<ConverterFactoryBase *> &factories();
> @@ -93,28 +94,30 @@ private:
>   
>   	static void registerType(ConverterFactoryBase *factory);
>   
> -	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> +	virtual std::unique_ptr<Converter> createInstance(MediaDevice *media, Converter::Features feat) const = 0;
>   
>   	std::string name_;
>   	std::vector<std::string> compatibles_;
> +	Converter::Features features_;
>   };
>   
>   template<typename _Converter>
>   class ConverterFactory : public ConverterFactoryBase
>   {
>   public:
> -	ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
> -		: ConverterFactoryBase(name, compatibles)
> +	ConverterFactory(const char *name, std::initializer_list<std::string> compatibles, Converter::Features feat)
> +		: ConverterFactoryBase(name, compatibles, feat)
>   	{
>   	}
>   
> -	std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> +	std::unique_ptr<Converter> createInstance(MediaDevice *media, Converter::Features feat) const override
>   	{
> -		return std::make_unique<_Converter>(media);
> +		return std::make_unique<_Converter>(media, feat);
>   	}
>   };
>   
>   #define REGISTER_CONVERTER(name, converter, compatibles) \
> -	static ConverterFactory<converter> global_##converter##Factory(name, compatibles);
> +	static ConverterFactory<converter> global_##converter##Factory(name, compatibles, \
> +								       Converter::Feature::None);
>   
>   } /* namespace libcamera */
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2c3da6d4502b..dcbc442ccf68 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -178,7 +178,7 @@ Converter::~Converter()
>    */
>   
>   /**
> - * \fn Converter::getFeatures()
> + * \fn Converter::features()
>    * \brief Gets the supported features by the converter
>    * \return The converter Features flag
>    */
> @@ -209,8 +209,10 @@ Converter::~Converter()
>    * 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)
> +ConverterFactoryBase::ConverterFactoryBase(const std::string name,
> +					   std::initializer_list<std::string> compatibles,
> +					   Converter::Features feat)
> +	: name_(name), compatibles_(compatibles), features_(feat)
>   {
>   	registerType(this);
>   }
> @@ -247,7 +249,7 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>   			<< factory->name_ << " factory with "
>   			<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";
>   
> -		std::unique_ptr<Converter> converter = factory->createInstance(media);
> +		std::unique_ptr<Converter> converter = factory->createInstance(media, factory->features());
>   		if (converter->isValid())
>   			return converter;
>   	}