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

[Jacopo Mondi]

Hi Umang

On Wed, Jul 17, 2024 at 06:10:42PM GMT, Umang Jain wrote:
> 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?

I see, so we need something like a structure that associates to a
compatible a set of data, which for now can only include capabilities
(or features)..

>
> 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;
> >   	}
>