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

[Umang Jain]

Hi Jacopo,

On 17/07/24 3:24 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Jul 10, 2024 at 01:51:47AM GMT, 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.
>>
>> Signed-off-by: Umang Jain <umang.jain 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..7e478356 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 getFeatures() const { return features_; }
> no "get" in getters
> I know you'll hit a name clash with function arguments, just rename
> them
>
>> +
>>   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);
> personally I feel it would be nicer to be able to do something like
>
>
> REGISTER_CONVERTER("dw100", ConverterDW100, compatibles,
> 		   Converter::Feature::Crop)
>
> and specify features at converter registration time.
>
> I have patches for this if you're interested.

definitively looks on the right path. For now, I left out the 
REGISTER_CONVERTER parts (since I don't have the test setup yet for this.)

Ideally, a YUV sensor with i.MX8MP ISI and dw100 as converter. And I 
don't have a YUV sensor yet - which can be used with simple pipeline 
handler.

>
>>   	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..2c3da6d4 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::getFeatures()
>> + * \brief Gets the supported features by the converter
>> + * \return The converter Features flag
>> + */
>> +
>>   /**
>>    * \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;
>> --
>> 2.45.2
>>