[RFC,2/9] fixup: Add Features at Converter registration time
diff mbox series

Message ID 20240717100913.16640-3-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • Handle Converter features differently
Related show

Commit Message

Jacopo Mondi July 17, 2024, 10:09 a.m. UTC 
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/internal/converter.h | 23 +++++++++++++----------
 src/libcamera/converter.cpp            | 10 ++++++----
 2 files changed, 19 insertions(+), 14 deletions(-)

Comments

Umang Jain July 17, 2024, 12:40 p.m. UTC | #1 
Hi Jacopo

On 17/07/24 3:39 pm, Jacopo Mondi wrote:
> Signed-off-by: Jacopo Mondi <jacopo.mondi@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;
>   	}
Jacopo Mondi July 18, 2024, 10:43 a.m. UTC | #2 
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@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;
> >   	}
>
Laurent Pinchart Aug. 2, 2024, 9:57 p.m. UTC | #3 
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@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;
> > >   	}
> >

Patch
diff mbox series 

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