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

[Laurent Pinchart]

On Thu, Jul 18, 2024 at 12:43:17PM +0200, Jacopo Mondi wrote:
> 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)..

As I mentioned in the review of Umang's series, it would be nicer if we
could detect those features dynamically.

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

-- 
Regards,

Laurent Pinchart