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

[Laurent Pinchart]

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 at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at 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 at 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;

-- 
Regards,

Laurent Pinchart