[v6,1/5] libcamera: converter: Add interface for feature flags

20710 diff mbox series

This patch intends to extend the converter interface to have feature
flags, which enables each converter to expose the set of features
it supports.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/internal/converter.h        | 13 ++++++++++-
 .../internal/converter/converter_v4l2_m2m.h   |  2 +-
 src/libcamera/converter.cpp                   | 22 ++++++++++++++++++-
 .../converter/converter_v4l2_m2m.cpp          |  5 +++--
 4 files changed, 37 insertions(+), 5 deletions(-)

Hi Umang,

Thank you for the patch.

On Fri, Jul 26, 2024 at 05:17:11PM +0530, Umang Jain wrote:
> This patch intends to extend the converter interface to have feature
> flags, which enables each converter to expose the set of features
> it supports.

Making the interface more generic and more widely usable sounds very
good to me.

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/internal/converter.h        | 13 ++++++++++-
>  .../internal/converter/converter_v4l2_m2m.h   |  2 +-
>  src/libcamera/converter.cpp                   | 22 ++++++++++++++++++-
>  .../converter/converter_v4l2_m2m.cpp          |  5 +++--
>  4 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index b51563d7..652ff519 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -17,6 +17,7 @@
>  #include <vector>
>  
>  #include <libcamera/base/class.h>
> +#include <libcamera/base/flags.h>
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/geometry.h>
> @@ -32,7 +33,13 @@ struct StreamConfiguration;
>  class Converter
>  {
>  public:
> -	Converter(MediaDevice *media);
> +	enum class Feature {
> +		None = 0,
> +	};
> +
> +	using Features = Flags<Feature>;
> +
> +	Converter(MediaDevice *media, Features features = Feature::None);
>  	virtual ~Converter();
>  
>  	virtual int loadConfiguration(const std::string &filename) = 0;
> @@ -61,8 +68,12 @@ public:
>  
>  	const std::string &deviceNode() const { return deviceNode_; }
>  
> +	Features features() const { return features_; }
> +
>  private:
>  	std::string deviceNode_;
> +
> +	Features features_;
>  };
>  
>  class ConverterFactoryBase
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index b9e59899..91701dbe 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -35,7 +35,7 @@ class V4L2M2MDevice;
>  class V4L2M2MConverter : public Converter
>  {
>  public:
> -	V4L2M2MConverter(MediaDevice *media);
> +	V4L2M2MConverter(MediaDevice *media, Features features = Feature::None);
>  
>  	int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }
>  	bool isValid() const { return m2m_ != nullptr; }
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2ab46133..e2dbf5e0 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -34,14 +34,27 @@ LOG_DEFINE_CATEGORY(Converter)
>   * parameters from the same input stream.
>   */
>  
> +/**
> + * \enum Converter::Feature
> + * \brief Specify the features supported by the converter
> + * \var Converter::Feature::None
> + * \brief No extra features supported by the converter
> + */
> +
> +/**
> + * \typedef Converter::Features
> + * \brief A bitwise combination of features supported by the converter
> + */
> +
>  /**
>   * \brief Construct a Converter instance
>   * \param[in] media The media device implementing the converter
> + * \param[in] features Features flags representing supported features
>   *
>   * 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)
> +Converter::Converter(MediaDevice *media, Features features)
>  {
>  	const std::vector<MediaEntity *> &entities = media->entities();
>  	auto it = std::find_if(entities.begin(), entities.end(),
> @@ -56,6 +69,7 @@ Converter::Converter(MediaDevice *media)
>  	}
>  
>  	deviceNode_ = (*it)->deviceNode();
> +	features_ = features;
>  }
>  
>  Converter::~Converter()
> @@ -163,6 +177,12 @@ Converter::~Converter()
>   * \return The converter device node string
>   */
>  
> +/**
> + * \fn Converter::features()
> + * \brief Gets the supported features by the converter

 * \brief Get the features supported by the converter

Overall this looks fine, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

but I may come back to this patch as I read the rest of the series.

> + * \return The converter Features flags
> + */
> +
>  /**
>   * \class ConverterFactoryBase
>   * \brief Base class for converter factories
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 2e77872e..4aeb7dd9 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -191,10 +191,11 @@ void V4L2M2MConverter::V4L2M2MStream::captureBufferReady(FrameBuffer *buffer)
>   * \fn V4L2M2MConverter::V4L2M2MConverter
>   * \brief Construct a V4L2M2MConverter instance
>   * \param[in] media The media device implementing the converter
> + * \param[in] features Features flags representing supported features
>   */
>  
> -V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> -	: Converter(media)
> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media, Features features)
> +	: Converter(media, features)
>  {
>  	if (deviceNode().empty())
>  		return;