[v5,2/5] libcamera: converter: Add interface to support cropping capability

20644 diff mbox series

If the converter has cropping capability, the interface should support it
by providing appropriate virtual functions. Provide Feature::Crop in
Feature enumeration for the same.

Provide virtual setCrop() and getCropBounds() interfaces so that
the converter can implement their own cropping functionality.

The V4L2M2MConverter implements these interfaces of the Converter
interface. Not all V4L2M2M converters will have cropping capability,
hence Feature::Crop should be set while construction for all those
converters.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/internal/converter.h        |  5 ++
 .../internal/converter/converter_v4l2_m2m.h   |  6 ++
 src/libcamera/converter.cpp                   | 51 +++++++++++
 .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
 4 files changed, 150 insertions(+)

Hi Umang

On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
> If the converter has cropping capability, the interface should support it
> by providing appropriate virtual functions. Provide Feature::Crop in
> Feature enumeration for the same.
>
> Provide virtual setCrop() and getCropBounds() interfaces so that
> the converter can implement their own cropping functionality.
>
> The V4L2M2MConverter implements these interfaces of the Converter
> interface. Not all V4L2M2M converters will have cropping capability,
> hence Feature::Crop should be set while construction for all those
> converters.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/internal/converter.h        |  5 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>  src/libcamera/converter.cpp                   | 51 +++++++++++
>  .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>  4 files changed, 150 insertions(+)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 7e478356..9b1223fd 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -14,6 +14,7 @@
>  #include <memory>
>  #include <string>
>  #include <tuple>
> +#include <utility>
>  #include <vector>
>
>  #include <libcamera/base/class.h>
> @@ -35,6 +36,7 @@ class Converter
>  public:
>  	enum class Feature {
>  		None = 0,
> +		Crop = (1 << 0),
>  	};
>
>  	using Features = Flags<Feature>;
> @@ -63,6 +65,9 @@ public:
>  	virtual int queueBuffers(FrameBuffer *input,
>  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>
> +	virtual int setCrop(const Stream *stream, Rectangle *rect);

"crop" is loosely defined as a concept.
A converter has an input and an output and I presume it scales.

Where is the crop rectangle applied ?

Does this need to be part of the base class interface ? Or should only
derived classes that support it implement it ? (it's reasonable for a
V4L2 M2M device to support this, once better specified the interface
where the crop rectangle has to be applied).

> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);

not "get" in getters

> +
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 91701dbe..2697eed9 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -30,6 +30,7 @@ class Size;
>  class SizeRange;
>  class Stream;
>  struct StreamConfiguration;
> +class Rectangle;
>  class V4L2M2MDevice;
>
>  class V4L2M2MConverter : public Converter
> @@ -57,6 +58,9 @@ public:
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>
> +	int setCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  private:
>  	class V4L2M2MStream : protected Loggable
>  	{
> @@ -75,6 +79,8 @@ private:
>
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>
> +		int setSelection(unsigned int target, Rectangle *rect);
> +
>  	protected:
>  		std::string logPrefix() const override;
>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2c3da6d4..d51e77a0 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -11,6 +11,8 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/media_device.h"
>
>  /**
> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief Specify the features supported by the converter
>   * \var Converter::Feature::None
>   * \brief No extra features supported by the converter
> + * \var Converter::Feature::Crop
> + * \brief Cropping capability is supported by the converter
>   */
>
>  /**
> @@ -161,6 +165,53 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>
> +/**
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to be applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::Crop flag in this case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream
> + * \param[in] stream Pointer to output stream
> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::Crop).
> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	Rectangle rect;
> +
> +	if (!(getFeatures() & Feature::Crop))
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +
> +	/*
> +	 * This is base implementation for the Converter class, so just return
> +	 * the stream configured size as minimum and maximum crop bounds.
> +	 */
> +	rect.size() = config.size;
> +
> +	return { rect, rect };

I would rather not make it part of the base class interface

> +}
> +
>  /**
>   * \var Converter::inputBufferReady
>   * \brief A signal emitted when the input frame buffer completes
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 4aeb7dd9..eaae3528 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>  	return 0;
>  }
>
> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> +{
> +	int ret = m2m_->output()->setSelection(target, rect);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>  {
>  	return stream_->configuration().toString();
> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>  	return iter->second->exportBuffers(count, buffers);
>  }
>
> +/**
> + * \copydoc libcamera::Converter::setCrop
> + */
> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop))
> +		return -ENOTSUP;
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end())
> +		return -EINVAL;
> +
> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::getCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::getCropBounds(const Stream *stream)
> +{
> +	Rectangle minCrop;
> +	Rectangle maxCrop;
> +	int ret;
> +
> +	minCrop.width = 1;
> +	minCrop.height = 1;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;

You can move all of this after the check for the feature support

> +
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Crop functionality is not supported";
> +		return {};
> +	}
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end()) {
> +		/*
> +		 * No streams configured, return minimum and maximum crop
> +		 * bounds at initialization.
> +		 */
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> +			return {};
> +		}
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> +			return {};
> +		}
> +
> +		return { minCrop, maxCrop };
> +	}
> +
> +	/*
> +	 * If the streams are configured, return bounds from according to
> +	 * stream configuration.
> +	 */
> +	ret = setCrop(stream, &minCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> +		return {};
> +	}
> +
> +	ret = setCrop(stream, &maxCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> +		return {};
> +	}
> +
> +	return { minCrop, maxCrop };
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>  	return 0;
>  }
>
> +/*
> + * \todo: This should be extended to include Feature::Flag to denote
> + * what each converter supports feature-wise.
> + */

That's another option.

I'll send to the list the patches I have on top of your series that
makes it possible to specify features at registration time for a
comparison.

>  static std::initializer_list<std::string> compatibles = {
>  	"mtk-mdp",
>  	"pxp",
> --
> 2.45.2
>

Hi Jacopo

On 17/07/24 3:32 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
>> If the converter has cropping capability, the interface should support it
>> by providing appropriate virtual functions. Provide Feature::Crop in
>> Feature enumeration for the same.
>>
>> Provide virtual setCrop() and getCropBounds() interfaces so that
>> the converter can implement their own cropping functionality.
>>
>> The V4L2M2MConverter implements these interfaces of the Converter
>> interface. Not all V4L2M2M converters will have cropping capability,
>> hence Feature::Crop should be set while construction for all those
>> converters.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   include/libcamera/internal/converter.h        |  5 ++
>>   .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>>   src/libcamera/converter.cpp                   | 51 +++++++++++
>>   .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>>   4 files changed, 150 insertions(+)
>>
>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
>> index 7e478356..9b1223fd 100644
>> --- a/include/libcamera/internal/converter.h
>> +++ b/include/libcamera/internal/converter.h
>> @@ -14,6 +14,7 @@
>>   #include <memory>
>>   #include <string>
>>   #include <tuple>
>> +#include <utility>
>>   #include <vector>
>>
>>   #include <libcamera/base/class.h>
>> @@ -35,6 +36,7 @@ class Converter
>>   public:
>>   	enum class Feature {
>>   		None = 0,
>> +		Crop = (1 << 0),
>>   	};
>>
>>   	using Features = Flags<Feature>;
>> @@ -63,6 +65,9 @@ public:
>>   	virtual int queueBuffers(FrameBuffer *input,
>>   				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>>
>> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> "crop" is loosely defined as a concept.
> A converter has an input and an output and I presume it scales.
>
> Where is the crop rectangle applied ?

A converter is a processing block - outside of ISP. Hence there is a 
input and output, and the crop is applied at/w.r.t input. The resultant 
should be obtained/visible on the output.

>
> Does this need to be part of the base class interface ? Or should only
> derived classes that support it implement it ? (it's reasonable for a
> V4L2 M2M device to support this, once better specified the interface
> where the crop rectangle has to be applied).

I think so. Atleast that's what Laurent initally asked for. Converter 
interface should dictate the features supported, the implementation left 
to the derived converter classes. No implementation specific details 
should be handled by the interface itself, only the "feature" part

>> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> not "get" in getters

ah yes
>
>> +
>>   	Signal<FrameBuffer *> inputBufferReady;
>>   	Signal<FrameBuffer *> outputBufferReady;
>>
>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> index 91701dbe..2697eed9 100644
>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> @@ -30,6 +30,7 @@ class Size;
>>   class SizeRange;
>>   class Stream;
>>   struct StreamConfiguration;
>> +class Rectangle;
>>   class V4L2M2MDevice;
>>
>>   class V4L2M2MConverter : public Converter
>> @@ -57,6 +58,9 @@ public:
>>   	int queueBuffers(FrameBuffer *input,
>>   			 const std::map<const Stream *, FrameBuffer *> &outputs);
>>
>> +	int setCrop(const Stream *stream, Rectangle *rect);
>> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
>> +
>>   private:
>>   	class V4L2M2MStream : protected Loggable
>>   	{
>> @@ -75,6 +79,8 @@ private:
>>
>>   		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>>
>> +		int setSelection(unsigned int target, Rectangle *rect);
>> +
>>   	protected:
>>   		std::string logPrefix() const override;
>>
>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>> index 2c3da6d4..d51e77a0 100644
>> --- a/src/libcamera/converter.cpp
>> +++ b/src/libcamera/converter.cpp
>> @@ -11,6 +11,8 @@
>>
>>   #include <libcamera/base/log.h>
>>
>> +#include <libcamera/stream.h>
>> +
>>   #include "libcamera/internal/media_device.h"
>>
>>   /**
>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>>    * \brief Specify the features supported by the converter
>>    * \var Converter::Feature::None
>>    * \brief No extra features supported by the converter
>> + * \var Converter::Feature::Crop
>> + * \brief Cropping capability is supported by the converter
>>    */
>>
>>   /**
>> @@ -161,6 +165,53 @@ Converter::~Converter()
>>    * \return 0 on success or a negative error code otherwise
>>    */
>>
>> +/**
>> + * \brief Set the crop rectangle \a rect for \a stream
>> + * \param[in] stream Pointer to output stream
>> + * \param[inout] rect The crop rectangle to be applied
>> + *
>> + * Set the crop rectangle \a rect for \a stream provided the converter supports
>> + * cropping. The converter should have the Feature::Crop flag in this case.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
>> +{
>> +	if (!(getFeatures() & Feature::Crop)) {
>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Get the crop bounds \a stream
>> + * \param[in] stream Pointer to output stream
>> + *
>> + * Get the minimum and maximum crop bounds for \a stream. The converter
>> + * should supporting cropping (Feature::Crop).
>> + *
>> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
>> + * crop bound respectively.
>> + */
>> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
>> +{
>> +	const StreamConfiguration &config = stream->configuration();
>> +	Rectangle rect;
>> +
>> +	if (!(getFeatures() & Feature::Crop))
>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>> +
>> +	/*
>> +	 * This is base implementation for the Converter class, so just return
>> +	 * the stream configured size as minimum and maximum crop bounds.
>> +	 */
>> +	rect.size() = config.size;
>> +
>> +	return { rect, rect };
> I would rather not make it part of the base class interface

no? If the crop is a feature, getting the supported bounds would be 
something callers would like to know. Do you think this should be left 
only to derived converter classes ?
>> +}
>> +
>>   /**
>>    * \var Converter::inputBufferReady
>>    * \brief A signal emitted when the input frame buffer completes
>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> index 4aeb7dd9..eaae3528 100644
>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>>   	return 0;
>>   }
>>
>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
>> +{
>> +	int ret = m2m_->output()->setSelection(target, rect);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>>   {
>>   	return stream_->configuration().toString();
>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>>   	return iter->second->exportBuffers(count, buffers);
>>   }
>>
>> +/**
>> + * \copydoc libcamera::Converter::setCrop
>> + */
>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
>> +{
>> +	if (!(getFeatures() & Feature::Crop))
>> +		return -ENOTSUP;
>> +
>> +	auto iter = streams_.find(stream);
>> +	if (iter == streams_.end())
>> +		return -EINVAL;
>> +
>> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::Converter::getCropBounds
>> + */
>> +std::pair<Rectangle, Rectangle>
>> +V4L2M2MConverter::getCropBounds(const Stream *stream)
>> +{
>> +	Rectangle minCrop;
>> +	Rectangle maxCrop;
>> +	int ret;
>> +
>> +	minCrop.width = 1;
>> +	minCrop.height = 1;
>> +	maxCrop.width = UINT_MAX;
>> +	maxCrop.height = UINT_MAX;
> You can move all of this after the check for the feature support
>
>> +
>> +	if (!(getFeatures() & Feature::Crop)) {
>> +		LOG(Converter, Error) << "Crop functionality is not supported";
>> +		return {};
>> +	}
>> +
>> +	auto iter = streams_.find(stream);
>> +	if (iter == streams_.end()) {
>> +		/*
>> +		 * No streams configured, return minimum and maximum crop
>> +		 * bounds at initialization.
>> +		 */
>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
>> +		if (ret) {
>> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
>> +			return {};
>> +		}
>> +
>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
>> +		if (ret) {
>> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
>> +			return {};
>> +		}
>> +
>> +		return { minCrop, maxCrop };
>> +	}
>> +
>> +	/*
>> +	 * If the streams are configured, return bounds from according to
>> +	 * stream configuration.
>> +	 */
>> +	ret = setCrop(stream, &minCrop);
>> +	if (ret) {
>> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
>> +		return {};
>> +	}
>> +
>> +	ret = setCrop(stream, &maxCrop);
>> +	if (ret) {
>> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
>> +		return {};
>> +	}
>> +
>> +	return { minCrop, maxCrop };
>> +}
>> +
>>   /**
>>    * \copydoc libcamera::Converter::start
>>    */
>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>>   	return 0;
>>   }
>>
>> +/*
>> + * \todo: This should be extended to include Feature::Flag to denote
>> + * what each converter supports feature-wise.
>> + */
> That's another option.
>
> I'll send to the list the patches I have on top of your series that
> makes it possible to specify features at registration time for a
> comparison.

Right , this bit is something I have to develop once I get a testing setup.

The compatibles below are currently used for simple pipeline handler only.
>
>>   static std::initializer_list<std::string> compatibles = {
>>   	"mtk-mdp",
>>   	"pxp",
>> --
>> 2.45.2
>>

Hi Umang

On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
> Hi Jacopo
>
> On 17/07/24 3:32 pm, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
> > > If the converter has cropping capability, the interface should support it
> > > by providing appropriate virtual functions. Provide Feature::Crop in
> > > Feature enumeration for the same.
> > >
> > > Provide virtual setCrop() and getCropBounds() interfaces so that
> > > the converter can implement their own cropping functionality.
> > >
> > > The V4L2M2MConverter implements these interfaces of the Converter
> > > interface. Not all V4L2M2M converters will have cropping capability,
> > > hence Feature::Crop should be set while construction for all those
> > > converters.
> > >
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >   include/libcamera/internal/converter.h        |  5 ++
> > >   .../internal/converter/converter_v4l2_m2m.h   |  6 ++
> > >   src/libcamera/converter.cpp                   | 51 +++++++++++
> > >   .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
> > >   4 files changed, 150 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > index 7e478356..9b1223fd 100644
> > > --- a/include/libcamera/internal/converter.h
> > > +++ b/include/libcamera/internal/converter.h
> > > @@ -14,6 +14,7 @@
> > >   #include <memory>
> > >   #include <string>
> > >   #include <tuple>
> > > +#include <utility>
> > >   #include <vector>
> > >
> > >   #include <libcamera/base/class.h>
> > > @@ -35,6 +36,7 @@ class Converter
> > >   public:
> > >   	enum class Feature {
> > >   		None = 0,
> > > +		Crop = (1 << 0),
> > >   	};
> > >
> > >   	using Features = Flags<Feature>;
> > > @@ -63,6 +65,9 @@ public:
> > >   	virtual int queueBuffers(FrameBuffer *input,
> > >   				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
> > >
> > > +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> > "crop" is loosely defined as a concept.
> > A converter has an input and an output and I presume it scales.
> >
> > Where is the crop rectangle applied ?
>
> A converter is a processing block - outside of ISP. Hence there is a input
> and output, and the crop is applied at/w.r.t input. The resultant should be
> obtained/visible on the output.
>

And where is the assumption that crop is applied to the input
documented ? Aren't there converter that allow to crop on the output
maybe -after- scaling ?

Cropping before or after scaling (or, if you want to put it different,
cropping on the input or on the output) are both possible options, and
this interface doesn't allow you to specify that.

You have wired the V4L2 converter to apply selection on the TGT_CROP
target on the input queue (aka on the 'output' video device, oh funny
names)

int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
{

	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
}


int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
{
	int ret = m2m_->output()->setSelection(target, rect);
	if (ret < 0)
		return ret;

	return 0;
}

Can't a converter expose a TGT_CROP target on the capture device to allow
userspace to apply a crop on the "final" image before returning it to
applications ?

> >
> > Does this need to be part of the base class interface ? Or should only
> > derived classes that support it implement it ? (it's reasonable for a
> > V4L2 M2M device to support this, once better specified the interface
> > where the crop rectangle has to be applied).
>
> I think so. Atleast that's what Laurent initally asked for. Converter
> interface should dictate the features supported, the implementation left to
> the derived converter classes. No implementation specific details should be
> handled by the interface itself, only the "feature" part
>

Yes, but concepts like 'crop' and 'selection' might only apply to
certain types of converter, like the v4l2 m2m ones. I wonder if it is
better to provide a stub implementation in the whole hierarcy or only
expose such methods in the classes derived from, in example,
V4L2M2MConverter.

> > > +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > not "get" in getters
>
> ah yes
> >
> > > +
> > >   	Signal<FrameBuffer *> inputBufferReady;
> > >   	Signal<FrameBuffer *> outputBufferReady;
> > >
> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > index 91701dbe..2697eed9 100644
> > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > @@ -30,6 +30,7 @@ class Size;
> > >   class SizeRange;
> > >   class Stream;
> > >   struct StreamConfiguration;
> > > +class Rectangle;
> > >   class V4L2M2MDevice;
> > >
> > >   class V4L2M2MConverter : public Converter
> > > @@ -57,6 +58,9 @@ public:
> > >   	int queueBuffers(FrameBuffer *input,
> > >   			 const std::map<const Stream *, FrameBuffer *> &outputs);
> > >
> > > +	int setCrop(const Stream *stream, Rectangle *rect);
> > > +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > +
> > >   private:
> > >   	class V4L2M2MStream : protected Loggable
> > >   	{
> > > @@ -75,6 +79,8 @@ private:
> > >
> > >   		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > >
> > > +		int setSelection(unsigned int target, Rectangle *rect);
> > > +
> > >   	protected:
> > >   		std::string logPrefix() const override;
> > >
> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > index 2c3da6d4..d51e77a0 100644
> > > --- a/src/libcamera/converter.cpp
> > > +++ b/src/libcamera/converter.cpp
> > > @@ -11,6 +11,8 @@
> > >
> > >   #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/stream.h>
> > > +
> > >   #include "libcamera/internal/media_device.h"
> > >
> > >   /**
> > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
> > >    * \brief Specify the features supported by the converter
> > >    * \var Converter::Feature::None
> > >    * \brief No extra features supported by the converter
> > > + * \var Converter::Feature::Crop
> > > + * \brief Cropping capability is supported by the converter
> > >    */
> > >
> > >   /**
> > > @@ -161,6 +165,53 @@ Converter::~Converter()
> > >    * \return 0 on success or a negative error code otherwise
> > >    */
> > >
> > > +/**
> > > + * \brief Set the crop rectangle \a rect for \a stream
> > > + * \param[in] stream Pointer to output stream
> > > + * \param[inout] rect The crop rectangle to be applied
> > > + *
> > > + * Set the crop rectangle \a rect for \a stream provided the converter supports
> > > + * cropping. The converter should have the Feature::Crop flag in this case.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> > > +{
> > > +	if (!(getFeatures() & Feature::Crop)) {
> > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > +		return -ENOTSUP;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the crop bounds \a stream
> > > + * \param[in] stream Pointer to output stream
> > > + *
> > > + * Get the minimum and maximum crop bounds for \a stream. The converter
> > > + * should supporting cropping (Feature::Crop).
> > > + *
> > > + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> > > + * crop bound respectively.
> > > + */
> > > +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> > > +{
> > > +	const StreamConfiguration &config = stream->configuration();
> > > +	Rectangle rect;
> > > +
> > > +	if (!(getFeatures() & Feature::Crop))
> > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > +
> > > +	/*
> > > +	 * This is base implementation for the Converter class, so just return
> > > +	 * the stream configured size as minimum and maximum crop bounds.
> > > +	 */
> > > +	rect.size() = config.size;
> > > +
> > > +	return { rect, rect };
> > I would rather not make it part of the base class interface
>
> no? If the crop is a feature, getting the supported bounds would be
> something callers would like to know. Do you think this should be left only
> to derived converter classes ?

Only to the hierarchy of derived classes that supports it, like the
classes derived from V4L2M2MConverter (and the class itself). For some
other "converters" cropping and scaling might not make sense at all.

> > > +}
> > > +
> > >   /**
> > >    * \var Converter::inputBufferReady
> > >    * \brief A signal emitted when the input frame buffer completes
> > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > index 4aeb7dd9..eaae3528 100644
> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> > >   	return 0;
> > >   }
> > >
> > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> > > +{
> > > +	int ret = m2m_->output()->setSelection(target, rect);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> > >   {
> > >   	return stream_->configuration().toString();
> > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> > >   	return iter->second->exportBuffers(count, buffers);
> > >   }
> > >
> > > +/**
> > > + * \copydoc libcamera::Converter::setCrop
> > > + */
> > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> > > +{
> > > +	if (!(getFeatures() & Feature::Crop))
> > > +		return -ENOTSUP;
> > > +
> > > +	auto iter = streams_.find(stream);
> > > +	if (iter == streams_.end())
> > > +		return -EINVAL;
> > > +
> > > +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::Converter::getCropBounds
> > > + */
> > > +std::pair<Rectangle, Rectangle>
> > > +V4L2M2MConverter::getCropBounds(const Stream *stream)
> > > +{
> > > +	Rectangle minCrop;
> > > +	Rectangle maxCrop;
> > > +	int ret;
> > > +
> > > +	minCrop.width = 1;
> > > +	minCrop.height = 1;
> > > +	maxCrop.width = UINT_MAX;
> > > +	maxCrop.height = UINT_MAX;
> > You can move all of this after the check for the feature support
> >
> > > +
> > > +	if (!(getFeatures() & Feature::Crop)) {
> > > +		LOG(Converter, Error) << "Crop functionality is not supported";
> > > +		return {};
> > > +	}
> > > +
> > > +	auto iter = streams_.find(stream);
> > > +	if (iter == streams_.end()) {
> > > +		/*
> > > +		 * No streams configured, return minimum and maximum crop
> > > +		 * bounds at initialization.
> > > +		 */
> > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> > > +		if (ret) {
> > > +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > +			return {};
> > > +		}
> > > +
> > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > > +		if (ret) {
> > > +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > +			return {};
> > > +		}
> > > +
> > > +		return { minCrop, maxCrop };
> > > +	}
> > > +
> > > +	/*
> > > +	 * If the streams are configured, return bounds from according to
> > > +	 * stream configuration.
> > > +	 */
> > > +	ret = setCrop(stream, &minCrop);
> > > +	if (ret) {
> > > +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > +		return {};
> > > +	}
> > > +
> > > +	ret = setCrop(stream, &maxCrop);
> > > +	if (ret) {
> > > +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > +		return {};
> > > +	}
> > > +
> > > +	return { minCrop, maxCrop };
> > > +}
> > > +
> > >   /**
> > >    * \copydoc libcamera::Converter::start
> > >    */
> > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> > >   	return 0;
> > >   }
> > >
> > > +/*
> > > + * \todo: This should be extended to include Feature::Flag to denote
> > > + * what each converter supports feature-wise.
> > > + */
> > That's another option.
> >
> > I'll send to the list the patches I have on top of your series that
> > makes it possible to specify features at registration time for a
> > comparison.
>
> Right , this bit is something I have to develop once I get a testing setup.
>
> The compatibles below are currently used for simple pipeline handler only.
> >
> > >   static std::initializer_list<std::string> compatibles = {
> > >   	"mtk-mdp",
> > >   	"pxp",
> > > --
> > > 2.45.2
> > >
>

On Wed, Jul 10, 2024 at 01:51:48AM +0530, Umang Jain wrote:
> If the converter has cropping capability, the interface should support it
> by providing appropriate virtual functions. Provide Feature::Crop in
> Feature enumeration for the same.
> 
> Provide virtual setCrop() and getCropBounds() interfaces so that
> the converter can implement their own cropping functionality.
> 
> The V4L2M2MConverter implements these interfaces of the Converter
> interface. Not all V4L2M2M converters will have cropping capability,
> hence Feature::Crop should be set while construction for all those
> converters.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

My comments are satisfied... (although probably remove get from getter
is good)

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  include/libcamera/internal/converter.h        |  5 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>  src/libcamera/converter.cpp                   | 51 +++++++++++
>  .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>  4 files changed, 150 insertions(+)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 7e478356..9b1223fd 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -14,6 +14,7 @@
>  #include <memory>
>  #include <string>
>  #include <tuple>
> +#include <utility>
>  #include <vector>
>  
>  #include <libcamera/base/class.h>
> @@ -35,6 +36,7 @@ class Converter
>  public:
>  	enum class Feature {
>  		None = 0,
> +		Crop = (1 << 0),
>  	};
>  
>  	using Features = Flags<Feature>;
> @@ -63,6 +65,9 @@ public:
>  	virtual int queueBuffers(FrameBuffer *input,
>  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>  
> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>  
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 91701dbe..2697eed9 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -30,6 +30,7 @@ class Size;
>  class SizeRange;
>  class Stream;
>  struct StreamConfiguration;
> +class Rectangle;
>  class V4L2M2MDevice;
>  
>  class V4L2M2MConverter : public Converter
> @@ -57,6 +58,9 @@ public:
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>  
> +	int setCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  private:
>  	class V4L2M2MStream : protected Loggable
>  	{
> @@ -75,6 +79,8 @@ private:
>  
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> +		int setSelection(unsigned int target, Rectangle *rect);
> +
>  	protected:
>  		std::string logPrefix() const override;
>  
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2c3da6d4..d51e77a0 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -11,6 +11,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/media_device.h"
>  
>  /**
> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief Specify the features supported by the converter
>   * \var Converter::Feature::None
>   * \brief No extra features supported by the converter
> + * \var Converter::Feature::Crop
> + * \brief Cropping capability is supported by the converter
>   */
>  
>  /**
> @@ -161,6 +165,53 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>  
> +/**
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to be applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::Crop flag in this case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream
> + * \param[in] stream Pointer to output stream
> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::Crop).
> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	Rectangle rect;
> +
> +	if (!(getFeatures() & Feature::Crop))
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +
> +	/*
> +	 * This is base implementation for the Converter class, so just return
> +	 * the stream configured size as minimum and maximum crop bounds.
> +	 */
> +	rect.size() = config.size;
> +
> +	return { rect, rect };
> +}
> +
>  /**
>   * \var Converter::inputBufferReady
>   * \brief A signal emitted when the input frame buffer completes
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 4aeb7dd9..eaae3528 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>  	return 0;
>  }
>  
> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> +{
> +	int ret = m2m_->output()->setSelection(target, rect);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>  {
>  	return stream_->configuration().toString();
> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>  	return iter->second->exportBuffers(count, buffers);
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::setCrop
> + */
> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop))
> +		return -ENOTSUP;
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end())
> +		return -EINVAL;
> +
> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::getCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::getCropBounds(const Stream *stream)
> +{
> +	Rectangle minCrop;
> +	Rectangle maxCrop;
> +	int ret;
> +
> +	minCrop.width = 1;
> +	minCrop.height = 1;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;
> +
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Crop functionality is not supported";
> +		return {};
> +	}
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end()) {
> +		/*
> +		 * No streams configured, return minimum and maximum crop
> +		 * bounds at initialization.
> +		 */
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> +			return {};
> +		}
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> +			return {};
> +		}
> +
> +		return { minCrop, maxCrop };
> +	}
> +
> +	/*
> +	 * If the streams are configured, return bounds from according to
> +	 * stream configuration.
> +	 */
> +	ret = setCrop(stream, &minCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> +		return {};
> +	}
> +
> +	ret = setCrop(stream, &maxCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> +		return {};
> +	}
> +
> +	return { minCrop, maxCrop };
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>  	return 0;
>  }
>  
> +/*
> + * \todo: This should be extended to include Feature::Flag to denote
> + * what each converter supports feature-wise.
> + */
>  static std::initializer_list<std::string> compatibles = {
>  	"mtk-mdp",
>  	"pxp",
> -- 
> 2.45.2
>

On Thu, Jul 18, 2024 at 12:06:12PM +0200, Jacopo Mondi wrote:
> Hi Umang
> 
> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
> > Hi Jacopo
> >
> > On 17/07/24 3:32 pm, Jacopo Mondi wrote:
> > > Hi Umang
> > >
> > > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
> > > > If the converter has cropping capability, the interface should support it
> > > > by providing appropriate virtual functions. Provide Feature::Crop in
> > > > Feature enumeration for the same.
> > > >
> > > > Provide virtual setCrop() and getCropBounds() interfaces so that
> > > > the converter can implement their own cropping functionality.
> > > >
> > > > The V4L2M2MConverter implements these interfaces of the Converter
> > > > interface. Not all V4L2M2M converters will have cropping capability,
> > > > hence Feature::Crop should be set while construction for all those
> > > > converters.
> > > >
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > ---
> > > >   include/libcamera/internal/converter.h        |  5 ++
> > > >   .../internal/converter/converter_v4l2_m2m.h   |  6 ++
> > > >   src/libcamera/converter.cpp                   | 51 +++++++++++
> > > >   .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
> > > >   4 files changed, 150 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > > index 7e478356..9b1223fd 100644
> > > > --- a/include/libcamera/internal/converter.h
> > > > +++ b/include/libcamera/internal/converter.h
> > > > @@ -14,6 +14,7 @@
> > > >   #include <memory>
> > > >   #include <string>
> > > >   #include <tuple>
> > > > +#include <utility>
> > > >   #include <vector>
> > > >
> > > >   #include <libcamera/base/class.h>
> > > > @@ -35,6 +36,7 @@ class Converter
> > > >   public:
> > > >   	enum class Feature {
> > > >   		None = 0,
> > > > +		Crop = (1 << 0),
> > > >   	};
> > > >
> > > >   	using Features = Flags<Feature>;
> > > > @@ -63,6 +65,9 @@ public:
> > > >   	virtual int queueBuffers(FrameBuffer *input,
> > > >   				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
> > > >
> > > > +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> > > "crop" is loosely defined as a concept.
> > > A converter has an input and an output and I presume it scales.
> > >
> > > Where is the crop rectangle applied ?
> >
> > A converter is a processing block - outside of ISP. Hence there is a input
> > and output, and the crop is applied at/w.r.t input. The resultant should be
> > obtained/visible on the output.
> >
> 
> And where is the assumption that crop is applied to the input
> documented ? Aren't there converter that allow to crop on the output
> maybe -after- scaling ?

I suppose this should be added to the documentation of the cropping
feature flag.

> 
> Cropping before or after scaling (or, if you want to put it different,
> cropping on the input or on the output) are both possible options, and
> this interface doesn't allow you to specify that.
> 
> You have wired the V4L2 converter to apply selection on the TGT_CROP
> target on the input queue (aka on the 'output' video device, oh funny
> names)
> 
> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> {
> 
> 	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> }
> 
> 
> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> {
> 	int ret = m2m_->output()->setSelection(target, rect);
> 	if (ret < 0)
> 		return ret;
> 
> 	return 0;
> }
> 
> Can't a converter expose a TGT_CROP target on the capture device to allow
> userspace to apply a crop on the "final" image before returning it to
> applications ?
> 
> > >
> > > Does this need to be part of the base class interface ? Or should only
> > > derived classes that support it implement it ? (it's reasonable for a
> > > V4L2 M2M device to support this, once better specified the interface
> > > where the crop rectangle has to be applied).
> >
> > I think so. Atleast that's what Laurent initally asked for. Converter
> > interface should dictate the features supported, the implementation left to
> > the derived converter classes. No implementation specific details should be
> > handled by the interface itself, only the "feature" part
> >
> 
> Yes, but concepts like 'crop' and 'selection' might only apply to
> certain types of converter, like the v4l2 m2m ones. I wonder if it is
> better to provide a stub implementation in the whole hierarcy or only
> expose such methods in the classes derived from, in example,
> V4L2M2MConverter.

Maybe we can have another feature flag for output crop when the need
arises?

> 
> > > > +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > not "get" in getters
> >
> > ah yes
> > >
> > > > +
> > > >   	Signal<FrameBuffer *> inputBufferReady;
> > > >   	Signal<FrameBuffer *> outputBufferReady;
> > > >
> > > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > index 91701dbe..2697eed9 100644
> > > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > @@ -30,6 +30,7 @@ class Size;
> > > >   class SizeRange;
> > > >   class Stream;
> > > >   struct StreamConfiguration;
> > > > +class Rectangle;
> > > >   class V4L2M2MDevice;
> > > >
> > > >   class V4L2M2MConverter : public Converter
> > > > @@ -57,6 +58,9 @@ public:
> > > >   	int queueBuffers(FrameBuffer *input,
> > > >   			 const std::map<const Stream *, FrameBuffer *> &outputs);
> > > >
> > > > +	int setCrop(const Stream *stream, Rectangle *rect);
> > > > +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > > +
> > > >   private:
> > > >   	class V4L2M2MStream : protected Loggable
> > > >   	{
> > > > @@ -75,6 +79,8 @@ private:
> > > >
> > > >   		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > >
> > > > +		int setSelection(unsigned int target, Rectangle *rect);
> > > > +
> > > >   	protected:
> > > >   		std::string logPrefix() const override;
> > > >
> > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > > index 2c3da6d4..d51e77a0 100644
> > > > --- a/src/libcamera/converter.cpp
> > > > +++ b/src/libcamera/converter.cpp
> > > > @@ -11,6 +11,8 @@
> > > >
> > > >   #include <libcamera/base/log.h>
> > > >
> > > > +#include <libcamera/stream.h>
> > > > +
> > > >   #include "libcamera/internal/media_device.h"
> > > >
> > > >   /**
> > > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
> > > >    * \brief Specify the features supported by the converter
> > > >    * \var Converter::Feature::None
> > > >    * \brief No extra features supported by the converter
> > > > + * \var Converter::Feature::Crop
> > > > + * \brief Cropping capability is supported by the converter
> > > >    */
> > > >
> > > >   /**
> > > > @@ -161,6 +165,53 @@ Converter::~Converter()
> > > >    * \return 0 on success or a negative error code otherwise
> > > >    */
> > > >
> > > > +/**
> > > > + * \brief Set the crop rectangle \a rect for \a stream
> > > > + * \param[in] stream Pointer to output stream
> > > > + * \param[inout] rect The crop rectangle to be applied
> > > > + *
> > > > + * Set the crop rectangle \a rect for \a stream provided the converter supports
> > > > + * cropping. The converter should have the Feature::Crop flag in this case.
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> > > > +{
> > > > +	if (!(getFeatures() & Feature::Crop)) {
> > > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > > +		return -ENOTSUP;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Get the crop bounds \a stream
> > > > + * \param[in] stream Pointer to output stream
> > > > + *
> > > > + * Get the minimum and maximum crop bounds for \a stream. The converter
> > > > + * should supporting cropping (Feature::Crop).
> > > > + *
> > > > + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> > > > + * crop bound respectively.
> > > > + */
> > > > +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> > > > +{
> > > > +	const StreamConfiguration &config = stream->configuration();
> > > > +	Rectangle rect;
> > > > +
> > > > +	if (!(getFeatures() & Feature::Crop))
> > > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > > +
> > > > +	/*
> > > > +	 * This is base implementation for the Converter class, so just return
> > > > +	 * the stream configured size as minimum and maximum crop bounds.
> > > > +	 */
> > > > +	rect.size() = config.size;
> > > > +
> > > > +	return { rect, rect };
> > > I would rather not make it part of the base class interface
> >
> > no? If the crop is a feature, getting the supported bounds would be
> > something callers would like to know. Do you think this should be left only
> > to derived converter classes ?
> 
> Only to the hierarchy of derived classes that supports it, like the
> classes derived from V4L2M2MConverter (and the class itself). For some
> other "converters" cropping and scaling might not make sense at all.
> 

I thought the features flag tells you which features the converter
supports. And then for C++ purposes for converters that don't support
the feature, we have default nops for the setter , and a reasonable
default that makes sense for getters.


Paul

> > > > +}
> > > > +
> > > >   /**
> > > >    * \var Converter::inputBufferReady
> > > >    * \brief A signal emitted when the input frame buffer completes
> > > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > index 4aeb7dd9..eaae3528 100644
> > > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> > > >   	return 0;
> > > >   }
> > > >
> > > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> > > > +{
> > > > +	int ret = m2m_->output()->setSelection(target, rect);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> > > >   {
> > > >   	return stream_->configuration().toString();
> > > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> > > >   	return iter->second->exportBuffers(count, buffers);
> > > >   }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::Converter::setCrop
> > > > + */
> > > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> > > > +{
> > > > +	if (!(getFeatures() & Feature::Crop))
> > > > +		return -ENOTSUP;
> > > > +
> > > > +	auto iter = streams_.find(stream);
> > > > +	if (iter == streams_.end())
> > > > +		return -EINVAL;
> > > > +
> > > > +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \copydoc libcamera::Converter::getCropBounds
> > > > + */
> > > > +std::pair<Rectangle, Rectangle>
> > > > +V4L2M2MConverter::getCropBounds(const Stream *stream)
> > > > +{
> > > > +	Rectangle minCrop;
> > > > +	Rectangle maxCrop;
> > > > +	int ret;
> > > > +
> > > > +	minCrop.width = 1;
> > > > +	minCrop.height = 1;
> > > > +	maxCrop.width = UINT_MAX;
> > > > +	maxCrop.height = UINT_MAX;
> > > You can move all of this after the check for the feature support
> > >
> > > > +
> > > > +	if (!(getFeatures() & Feature::Crop)) {
> > > > +		LOG(Converter, Error) << "Crop functionality is not supported";
> > > > +		return {};
> > > > +	}
> > > > +
> > > > +	auto iter = streams_.find(stream);
> > > > +	if (iter == streams_.end()) {
> > > > +		/*
> > > > +		 * No streams configured, return minimum and maximum crop
> > > > +		 * bounds at initialization.
> > > > +		 */
> > > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> > > > +		if (ret) {
> > > > +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > > +			return {};
> > > > +		}
> > > > +
> > > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > > > +		if (ret) {
> > > > +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > +			return {};
> > > > +		}
> > > > +
> > > > +		return { minCrop, maxCrop };
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * If the streams are configured, return bounds from according to
> > > > +	 * stream configuration.
> > > > +	 */
> > > > +	ret = setCrop(stream, &minCrop);
> > > > +	if (ret) {
> > > > +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > > +		return {};
> > > > +	}
> > > > +
> > > > +	ret = setCrop(stream, &maxCrop);
> > > > +	if (ret) {
> > > > +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > +		return {};
> > > > +	}
> > > > +
> > > > +	return { minCrop, maxCrop };
> > > > +}
> > > > +
> > > >   /**
> > > >    * \copydoc libcamera::Converter::start
> > > >    */
> > > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> > > >   	return 0;
> > > >   }
> > > >
> > > > +/*
> > > > + * \todo: This should be extended to include Feature::Flag to denote
> > > > + * what each converter supports feature-wise.
> > > > + */
> > > That's another option.
> > >
> > > I'll send to the list the patches I have on top of your series that
> > > makes it possible to specify features at registration time for a
> > > comparison.
> >
> > Right , this bit is something I have to develop once I get a testing setup.
> >
> > The compatibles below are currently used for simple pipeline handler only.
> > >
> > > >   static std::initializer_list<std::string> compatibles = {
> > > >   	"mtk-mdp",
> > > >   	"pxp",
> > > > --
> > > > 2.45.2
> > > >
> >

On Wed, Jul 10, 2024 at 01:51:48AM +0530, Umang Jain wrote:
> If the converter has cropping capability, the interface should support it
> by providing appropriate virtual functions. Provide Feature::Crop in
> Feature enumeration for the same.
> 
> Provide virtual setCrop() and getCropBounds() interfaces so that
> the converter can implement their own cropping functionality.
> 
> The V4L2M2MConverter implements these interfaces of the Converter
> interface. Not all V4L2M2M converters will have cropping capability,
> hence Feature::Crop should be set while construction for all those
> converters.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/internal/converter.h        |  5 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>  src/libcamera/converter.cpp                   | 51 +++++++++++
>  .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>  4 files changed, 150 insertions(+)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 7e478356..9b1223fd 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -14,6 +14,7 @@
>  #include <memory>
>  #include <string>
>  #include <tuple>
> +#include <utility>
>  #include <vector>
>  
>  #include <libcamera/base/class.h>
> @@ -35,6 +36,7 @@ class Converter
>  public:
>  	enum class Feature {
>  		None = 0,
> +		Crop = (1 << 0),
>  	};
>  
>  	using Features = Flags<Feature>;
> @@ -63,6 +65,9 @@ public:
>  	virtual int queueBuffers(FrameBuffer *input,
>  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>  
> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>  
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 91701dbe..2697eed9 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -30,6 +30,7 @@ class Size;
>  class SizeRange;
>  class Stream;
>  struct StreamConfiguration;
> +class Rectangle;
>  class V4L2M2MDevice;
>  
>  class V4L2M2MConverter : public Converter
> @@ -57,6 +58,9 @@ public:
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>  
> +	int setCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  private:
>  	class V4L2M2MStream : protected Loggable
>  	{
> @@ -75,6 +79,8 @@ private:
>  
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> +		int setSelection(unsigned int target, Rectangle *rect);
> +
>  	protected:
>  		std::string logPrefix() const override;
>  
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2c3da6d4..d51e77a0 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -11,6 +11,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/media_device.h"
>  
>  /**
> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief Specify the features supported by the converter
>   * \var Converter::Feature::None
>   * \brief No extra features supported by the converter
> + * \var Converter::Feature::Crop
> + * \brief Cropping capability is supported by the converter
>   */
>  
>  /**
> @@ -161,6 +165,53 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>  
> +/**
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to be applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::Crop flag in this case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream
> + * \param[in] stream Pointer to output stream
> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::Crop).
> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	Rectangle rect;
> +
> +	if (!(getFeatures() & Feature::Crop))
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +
> +	/*
> +	 * This is base implementation for the Converter class, so just return
> +	 * the stream configured size as minimum and maximum crop bounds.
> +	 */
> +	rect.size() = config.size;
> +
> +	return { rect, rect };
> +}
> +
>  /**
>   * \var Converter::inputBufferReady
>   * \brief A signal emitted when the input frame buffer completes
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 4aeb7dd9..eaae3528 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>  	return 0;
>  }
>  
> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> +{
> +	int ret = m2m_->output()->setSelection(target, rect);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>  {
>  	return stream_->configuration().toString();
> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>  	return iter->second->exportBuffers(count, buffers);
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::setCrop
> + */
> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop))
> +		return -ENOTSUP;
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end())
> +		return -EINVAL;
> +
> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::getCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::getCropBounds(const Stream *stream)
> +{
> +	Rectangle minCrop;
> +	Rectangle maxCrop;
> +	int ret;
> +
> +	minCrop.width = 1;
> +	minCrop.height = 1;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;
> +
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Crop functionality is not supported";
> +		return {};

Hold on, shouldn't this return { maxCrop, maxCrop } ? Or at least the
configured output size. That's equivalent to "crop not supported".

> +	}
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end()) {
> +		/*
> +		 * No streams configured, return minimum and maximum crop
> +		 * bounds at initialization.
> +		 */
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> +			return {};

Here too.

> +		}
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> +			return {};

And here.


Paul

> +		}
> +
> +		return { minCrop, maxCrop };
> +	}
> +
> +	/*
> +	 * If the streams are configured, return bounds from according to
> +	 * stream configuration.
> +	 */
> +	ret = setCrop(stream, &minCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> +		return {};
> +	}
> +
> +	ret = setCrop(stream, &maxCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> +		return {};
> +	}
> +
> +	return { minCrop, maxCrop };
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>  	return 0;
>  }
>  
> +/*
> + * \todo: This should be extended to include Feature::Flag to denote
> + * what each converter supports feature-wise.
> + */
>  static std::initializer_list<std::string> compatibles = {
>  	"mtk-mdp",
>  	"pxp",
> -- 
> 2.45.2
>

Hi Jacopo

On 18/07/24 3:36 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
>> Hi Jacopo
>>
>> On 17/07/24 3:32 pm, Jacopo Mondi wrote:
>>> Hi Umang
>>>
>>> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
>>>> If the converter has cropping capability, the interface should support it
>>>> by providing appropriate virtual functions. Provide Feature::Crop in
>>>> Feature enumeration for the same.
>>>>
>>>> Provide virtual setCrop() and getCropBounds() interfaces so that
>>>> the converter can implement their own cropping functionality.
>>>>
>>>> The V4L2M2MConverter implements these interfaces of the Converter
>>>> interface. Not all V4L2M2M converters will have cropping capability,
>>>> hence Feature::Crop should be set while construction for all those
>>>> converters.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>    include/libcamera/internal/converter.h        |  5 ++
>>>>    .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>>>>    src/libcamera/converter.cpp                   | 51 +++++++++++
>>>>    .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>>>>    4 files changed, 150 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
>>>> index 7e478356..9b1223fd 100644
>>>> --- a/include/libcamera/internal/converter.h
>>>> +++ b/include/libcamera/internal/converter.h
>>>> @@ -14,6 +14,7 @@
>>>>    #include <memory>
>>>>    #include <string>
>>>>    #include <tuple>
>>>> +#include <utility>
>>>>    #include <vector>
>>>>
>>>>    #include <libcamera/base/class.h>
>>>> @@ -35,6 +36,7 @@ class Converter
>>>>    public:
>>>>    	enum class Feature {
>>>>    		None = 0,
>>>> +		Crop = (1 << 0),
>>>>    	};
>>>>
>>>>    	using Features = Flags<Feature>;
>>>> @@ -63,6 +65,9 @@ public:
>>>>    	virtual int queueBuffers(FrameBuffer *input,
>>>>    				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>>>>
>>>> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
>>> "crop" is loosely defined as a concept.
>>> A converter has an input and an output and I presume it scales.
>>>
>>> Where is the crop rectangle applied ?
>> A converter is a processing block - outside of ISP. Hence there is a input
>> and output, and the crop is applied at/w.r.t input. The resultant should be
>> obtained/visible on the output.
>>
> And where is the assumption that crop is applied to the input
> documented ? Aren't there converter that allow to crop on the output
> maybe -after- scaling ?
>
> Cropping before or after scaling (or, if you want to put it different,
> cropping on the input or on the output) are both possible options, and
> this interface doesn't allow you to specify that.

cropping after scaling means the application would get a different image 
output (if it tries to set crop on the output) ? Is that possible, a 
runtime crop ? Image output size differs everytime a crop is applied ?

I think what you are referring here is the compose rectangle, rather 
than the crop rectangle? Is that correct?

>
> You have wired the V4L2 converter to apply selection on the TGT_CROP
> target on the input queue (aka on the 'output' video device, oh funny
> names)
>
> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> {
>
> 	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> }
>
>
> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> {
> 	int ret = m2m_->output()->setSelection(target, rect);
> 	if (ret < 0)
> 		return ret;
>
> 	return 0;
> }
>
> Can't a converter expose a TGT_CROP target on the capture device to allow
> userspace to apply a crop on the "final" image before returning it to
> applications ?

So having read 
https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, 
it seems TGT_CROP can be target for video capture as well. There is no 
limitation on that front.

However, I can't wrap around my head around that kind of crop - applied 
at runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes 
more sense to me, rather than exposing TGT_CROP.

Certainly the interface is lacking such a distinction in this form - and 
I should clarify that the crop would be set on the input video signal. 
Would that makes things acceptable?

I am also a bit wary about how should we be exposing such a interface 
for converters, for output and video capture devices, have TGT_CROP and 
TGT_COMPOSE targets ... its seems hairy for now, to me..

>>> Does this need to be part of the base class interface ? Or should only
>>> derived classes that support it implement it ? (it's reasonable for a
>>> V4L2 M2M device to support this, once better specified the interface
>>> where the crop rectangle has to be applied).
>> I think so. Atleast that's what Laurent initally asked for. Converter
>> interface should dictate the features supported, the implementation left to
>> the derived converter classes. No implementation specific details should be
>> handled by the interface itself, only the "feature" part
>>
> Yes, but concepts like 'crop' and 'selection' might only apply to
> certain types of converter, like the v4l2 m2m ones. I wonder if it is
> better to provide a stub implementation in the whole hierarcy or only
> expose such methods in the classes derived from, in example,
> V4L2M2MConverter.
>
>>>> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
>>> not "get" in getters
>> ah yes
>>>> +
>>>>    	Signal<FrameBuffer *> inputBufferReady;
>>>>    	Signal<FrameBuffer *> outputBufferReady;
>>>>
>>>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>> index 91701dbe..2697eed9 100644
>>>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>> @@ -30,6 +30,7 @@ class Size;
>>>>    class SizeRange;
>>>>    class Stream;
>>>>    struct StreamConfiguration;
>>>> +class Rectangle;
>>>>    class V4L2M2MDevice;
>>>>
>>>>    class V4L2M2MConverter : public Converter
>>>> @@ -57,6 +58,9 @@ public:
>>>>    	int queueBuffers(FrameBuffer *input,
>>>>    			 const std::map<const Stream *, FrameBuffer *> &outputs);
>>>>
>>>> +	int setCrop(const Stream *stream, Rectangle *rect);
>>>> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
>>>> +
>>>>    private:
>>>>    	class V4L2M2MStream : protected Loggable
>>>>    	{
>>>> @@ -75,6 +79,8 @@ private:
>>>>
>>>>    		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>>>>
>>>> +		int setSelection(unsigned int target, Rectangle *rect);
>>>> +
>>>>    	protected:
>>>>    		std::string logPrefix() const override;
>>>>
>>>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>>>> index 2c3da6d4..d51e77a0 100644
>>>> --- a/src/libcamera/converter.cpp
>>>> +++ b/src/libcamera/converter.cpp
>>>> @@ -11,6 +11,8 @@
>>>>
>>>>    #include <libcamera/base/log.h>
>>>>
>>>> +#include <libcamera/stream.h>
>>>> +
>>>>    #include "libcamera/internal/media_device.h"
>>>>
>>>>    /**
>>>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>>>>     * \brief Specify the features supported by the converter
>>>>     * \var Converter::Feature::None
>>>>     * \brief No extra features supported by the converter
>>>> + * \var Converter::Feature::Crop
>>>> + * \brief Cropping capability is supported by the converter
>>>>     */
>>>>
>>>>    /**
>>>> @@ -161,6 +165,53 @@ Converter::~Converter()
>>>>     * \return 0 on success or a negative error code otherwise
>>>>     */
>>>>
>>>> +/**
>>>> + * \brief Set the crop rectangle \a rect for \a stream
>>>> + * \param[in] stream Pointer to output stream
>>>> + * \param[inout] rect The crop rectangle to be applied
>>>> + *
>>>> + * Set the crop rectangle \a rect for \a stream provided the converter supports
>>>> + * cropping. The converter should have the Feature::Crop flag in this case.
>>>> + *
>>>> + * \return 0 on success or a negative error code otherwise
>>>> + */
>>>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
>>>> +{
>>>> +	if (!(getFeatures() & Feature::Crop)) {
>>>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>>>> +		return -ENOTSUP;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Get the crop bounds \a stream
>>>> + * \param[in] stream Pointer to output stream
>>>> + *
>>>> + * Get the minimum and maximum crop bounds for \a stream. The converter
>>>> + * should supporting cropping (Feature::Crop).
>>>> + *
>>>> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
>>>> + * crop bound respectively.
>>>> + */
>>>> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
>>>> +{
>>>> +	const StreamConfiguration &config = stream->configuration();
>>>> +	Rectangle rect;
>>>> +
>>>> +	if (!(getFeatures() & Feature::Crop))
>>>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>>>> +
>>>> +	/*
>>>> +	 * This is base implementation for the Converter class, so just return
>>>> +	 * the stream configured size as minimum and maximum crop bounds.
>>>> +	 */
>>>> +	rect.size() = config.size;
>>>> +
>>>> +	return { rect, rect };
>>> I would rather not make it part of the base class interface
>> no? If the crop is a feature, getting the supported bounds would be
>> something callers would like to know. Do you think this should be left only
>> to derived converter classes ?
> Only to the hierarchy of derived classes that supports it, like the
> classes derived from V4L2M2MConverter (and the class itself). For some
> other "converters" cropping and scaling might not make sense at all.
>
>>>> +}
>>>> +
>>>>    /**
>>>>     * \var Converter::inputBufferReady
>>>>     * \brief A signal emitted when the input frame buffer completes
>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>> index 4aeb7dd9..eaae3528 100644
>>>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>>>>    	return 0;
>>>>    }
>>>>
>>>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
>>>> +{
>>>> +	int ret = m2m_->output()->setSelection(target, rect);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>>>>    {
>>>>    	return stream_->configuration().toString();
>>>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>>>>    	return iter->second->exportBuffers(count, buffers);
>>>>    }
>>>>
>>>> +/**
>>>> + * \copydoc libcamera::Converter::setCrop
>>>> + */
>>>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
>>>> +{
>>>> +	if (!(getFeatures() & Feature::Crop))
>>>> +		return -ENOTSUP;
>>>> +
>>>> +	auto iter = streams_.find(stream);
>>>> +	if (iter == streams_.end())
>>>> +		return -EINVAL;
>>>> +
>>>> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
>>>> +}
>>>> +
>>>> +/**
>>>> + * \copydoc libcamera::Converter::getCropBounds
>>>> + */
>>>> +std::pair<Rectangle, Rectangle>
>>>> +V4L2M2MConverter::getCropBounds(const Stream *stream)
>>>> +{
>>>> +	Rectangle minCrop;
>>>> +	Rectangle maxCrop;
>>>> +	int ret;
>>>> +
>>>> +	minCrop.width = 1;
>>>> +	minCrop.height = 1;
>>>> +	maxCrop.width = UINT_MAX;
>>>> +	maxCrop.height = UINT_MAX;
>>> You can move all of this after the check for the feature support
>>>
>>>> +
>>>> +	if (!(getFeatures() & Feature::Crop)) {
>>>> +		LOG(Converter, Error) << "Crop functionality is not supported";
>>>> +		return {};
>>>> +	}
>>>> +
>>>> +	auto iter = streams_.find(stream);
>>>> +	if (iter == streams_.end()) {
>>>> +		/*
>>>> +		 * No streams configured, return minimum and maximum crop
>>>> +		 * bounds at initialization.
>>>> +		 */
>>>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
>>>> +		if (ret) {
>>>> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
>>>> +			return {};
>>>> +		}
>>>> +
>>>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
>>>> +		if (ret) {
>>>> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
>>>> +			return {};
>>>> +		}
>>>> +
>>>> +		return { minCrop, maxCrop };
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * If the streams are configured, return bounds from according to
>>>> +	 * stream configuration.
>>>> +	 */
>>>> +	ret = setCrop(stream, &minCrop);
>>>> +	if (ret) {
>>>> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
>>>> +		return {};
>>>> +	}
>>>> +
>>>> +	ret = setCrop(stream, &maxCrop);
>>>> +	if (ret) {
>>>> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
>>>> +		return {};
>>>> +	}
>>>> +
>>>> +	return { minCrop, maxCrop };
>>>> +}
>>>> +
>>>>    /**
>>>>     * \copydoc libcamera::Converter::start
>>>>     */
>>>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>>>>    	return 0;
>>>>    }
>>>>
>>>> +/*
>>>> + * \todo: This should be extended to include Feature::Flag to denote
>>>> + * what each converter supports feature-wise.
>>>> + */
>>> That's another option.
>>>
>>> I'll send to the list the patches I have on top of your series that
>>> makes it possible to specify features at registration time for a
>>> comparison.
>> Right , this bit is something I have to develop once I get a testing setup.
>>
>> The compatibles below are currently used for simple pipeline handler only.
>>>>    static std::initializer_list<std::string> compatibles = {
>>>>    	"mtk-mdp",
>>>>    	"pxp",
>>>> --
>>>> 2.45.2
>>>>

Hi Umang

On Fri, Jul 19, 2024 at 03:31:48PM GMT, Umang Jain wrote:
> Hi Jacopo
>
> On 18/07/24 3:36 pm, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
> > > Hi Jacopo
> > >
> > > On 17/07/24 3:32 pm, Jacopo Mondi wrote:
> > > > Hi Umang
> > > >
> > > > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
> > > > > If the converter has cropping capability, the interface should support it
> > > > > by providing appropriate virtual functions. Provide Feature::Crop in
> > > > > Feature enumeration for the same.
> > > > >
> > > > > Provide virtual setCrop() and getCropBounds() interfaces so that
> > > > > the converter can implement their own cropping functionality.
> > > > >
> > > > > The V4L2M2MConverter implements these interfaces of the Converter
> > > > > interface. Not all V4L2M2M converters will have cropping capability,
> > > > > hence Feature::Crop should be set while construction for all those
> > > > > converters.
> > > > >
> > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > ---
> > > > >    include/libcamera/internal/converter.h        |  5 ++
> > > > >    .../internal/converter/converter_v4l2_m2m.h   |  6 ++
> > > > >    src/libcamera/converter.cpp                   | 51 +++++++++++
> > > > >    .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
> > > > >    4 files changed, 150 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > > > index 7e478356..9b1223fd 100644
> > > > > --- a/include/libcamera/internal/converter.h
> > > > > +++ b/include/libcamera/internal/converter.h
> > > > > @@ -14,6 +14,7 @@
> > > > >    #include <memory>
> > > > >    #include <string>
> > > > >    #include <tuple>
> > > > > +#include <utility>
> > > > >    #include <vector>
> > > > >
> > > > >    #include <libcamera/base/class.h>
> > > > > @@ -35,6 +36,7 @@ class Converter
> > > > >    public:
> > > > >    	enum class Feature {
> > > > >    		None = 0,
> > > > > +		Crop = (1 << 0),
> > > > >    	};
> > > > >
> > > > >    	using Features = Flags<Feature>;
> > > > > @@ -63,6 +65,9 @@ public:
> > > > >    	virtual int queueBuffers(FrameBuffer *input,
> > > > >    				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
> > > > >
> > > > > +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> > > > "crop" is loosely defined as a concept.
> > > > A converter has an input and an output and I presume it scales.
> > > >
> > > > Where is the crop rectangle applied ?
> > > A converter is a processing block - outside of ISP. Hence there is a input
> > > and output, and the crop is applied at/w.r.t input. The resultant should be
> > > obtained/visible on the output.
> > >
> > And where is the assumption that crop is applied to the input
> > documented ? Aren't there converter that allow to crop on the output
> > maybe -after- scaling ?
> >
> > Cropping before or after scaling (or, if you want to put it different,
> > cropping on the input or on the output) are both possible options, and
> > this interface doesn't allow you to specify that.
>
> cropping after scaling means the application would get a different image
> output (if it tries to set crop on the output) ? Is that possible, a runtime
> crop ? Image output size differs everytime a crop is applied ?

I don't think runtime crops are possible neither on the 'output video
device' or on the 'capture video device' as it would require changing
the buffer sizes at runtime. BUT my experience with m2m devices is
limited so..

What I meant is slightly different. Given an input buffer size of WxH
your converter will output a frame of W'xH'

To get from W to W' and from H to H' you can combine cropping and
scaling, theoretically freely. The canonical choice is to crop first
then scale, in order to maintain square pixels. In example if you have
an 1080p input frame (16:9) and you want to get to a 640x480 (4:3)
output, if you only scale you will change the image proportions.
Generally it is good practice to crop down 1080p to the closest 4:3
resolution, and then scale it down. So yes, "setCrop()" on the input
is the most reasonable choice. Due to my limited knowledge of
converters, I was wondering if other combinations of cropping/scaling
would be possible, or if multiple cropping points might be available
for a converter and if a generic "setCrop()" interface is
expressive enough to support more complex devices.

The setCrop() interface you have designed is "libcamera::Stream" oriented,
and a libcamera::Stream is associated in this class to a V4L2M2MStream
which contains an "input" and an "output". As the class interface
already expresses the "input" and "output" concepts (in example in
configure()) would it be an overkill to make it part of the setCrop()
interface as well ? (either with an INPUT/OUTPUT flag or by making
this a setInputCrop() function) ?

>
> I think what you are referring here is the compose rectangle, rather than
> the crop rectangle? Is that correct?
>
> >
> > You have wired the V4L2 converter to apply selection on the TGT_CROP
> > target on the input queue (aka on the 'output' video device, oh funny
> > names)
> >
> > int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> > {
> >
> > 	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> > }
> >
> >
> > int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> > {
> > 	int ret = m2m_->output()->setSelection(target, rect);
> > 	if (ret < 0)
> > 		return ret;
> >
> > 	return 0;
> > }
> >
> > Can't a converter expose a TGT_CROP target on the capture device to allow
> > userspace to apply a crop on the "final" image before returning it to
> > applications ?
>
> So having read
> https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, it
> seems TGT_CROP can be target for video capture as well. There is no
> limitation on that front.
>
> However, I can't wrap around my head around that kind of crop - applied at
> runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes more sense
> to me, rather than exposing TGT_CROP.

These are V4L2 specific concept. How a device expose selection targets
and how they should be used I presume will be handled by the dedicated
derived class.

What matters for me at the moment is the generic Converter interface
design.

>
> Certainly the interface is lacking such a distinction in this form - and I
> should clarify that the crop would be set on the input video signal. Would
> that makes things acceptable?
>
> I am also a bit wary about how should we be exposing such a interface for
> converters, for output and video capture devices, have TGT_CROP and
> TGT_COMPOSE targets ... its seems hairy for now, to me..
>
> > > > Does this need to be part of the base class interface ? Or should only
> > > > derived classes that support it implement it ? (it's reasonable for a
> > > > V4L2 M2M device to support this, once better specified the interface
> > > > where the crop rectangle has to be applied).
> > > I think so. Atleast that's what Laurent initally asked for. Converter
> > > interface should dictate the features supported, the implementation left to
> > > the derived converter classes. No implementation specific details should be
> > > handled by the interface itself, only the "feature" part
> > >
> > Yes, but concepts like 'crop' and 'selection' might only apply to
> > certain types of converter, like the v4l2 m2m ones. I wonder if it is
> > better to provide a stub implementation in the whole hierarcy or only
> > expose such methods in the classes derived from, in example,
> > V4L2M2MConverter.
> >
> > > > > +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > > not "get" in getters
> > > ah yes
> > > > > +
> > > > >    	Signal<FrameBuffer *> inputBufferReady;
> > > > >    	Signal<FrameBuffer *> outputBufferReady;
> > > > >
> > > > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > > index 91701dbe..2697eed9 100644
> > > > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > > > @@ -30,6 +30,7 @@ class Size;
> > > > >    class SizeRange;
> > > > >    class Stream;
> > > > >    struct StreamConfiguration;
> > > > > +class Rectangle;
> > > > >    class V4L2M2MDevice;
> > > > >
> > > > >    class V4L2M2MConverter : public Converter
> > > > > @@ -57,6 +58,9 @@ public:
> > > > >    	int queueBuffers(FrameBuffer *input,
> > > > >    			 const std::map<const Stream *, FrameBuffer *> &outputs);
> > > > >
> > > > > +	int setCrop(const Stream *stream, Rectangle *rect);
> > > > > +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > > > +
> > > > >    private:
> > > > >    	class V4L2M2MStream : protected Loggable
> > > > >    	{
> > > > > @@ -75,6 +79,8 @@ private:
> > > > >
> > > > >    		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > > >
> > > > > +		int setSelection(unsigned int target, Rectangle *rect);
> > > > > +
> > > > >    	protected:
> > > > >    		std::string logPrefix() const override;
> > > > >
> > > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > > > index 2c3da6d4..d51e77a0 100644
> > > > > --- a/src/libcamera/converter.cpp
> > > > > +++ b/src/libcamera/converter.cpp
> > > > > @@ -11,6 +11,8 @@
> > > > >
> > > > >    #include <libcamera/base/log.h>
> > > > >
> > > > > +#include <libcamera/stream.h>
> > > > > +
> > > > >    #include "libcamera/internal/media_device.h"
> > > > >
> > > > >    /**
> > > > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
> > > > >     * \brief Specify the features supported by the converter
> > > > >     * \var Converter::Feature::None
> > > > >     * \brief No extra features supported by the converter
> > > > > + * \var Converter::Feature::Crop
> > > > > + * \brief Cropping capability is supported by the converter
> > > > >     */
> > > > >
> > > > >    /**
> > > > > @@ -161,6 +165,53 @@ Converter::~Converter()
> > > > >     * \return 0 on success or a negative error code otherwise
> > > > >     */
> > > > >
> > > > > +/**
> > > > > + * \brief Set the crop rectangle \a rect for \a stream
> > > > > + * \param[in] stream Pointer to output stream
> > > > > + * \param[inout] rect The crop rectangle to be applied
> > > > > + *
> > > > > + * Set the crop rectangle \a rect for \a stream provided the converter supports
> > > > > + * cropping. The converter should have the Feature::Crop flag in this case.
> > > > > + *
> > > > > + * \return 0 on success or a negative error code otherwise
> > > > > + */
> > > > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> > > > > +{
> > > > > +	if (!(getFeatures() & Feature::Crop)) {
> > > > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > > > +		return -ENOTSUP;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \brief Get the crop bounds \a stream
> > > > > + * \param[in] stream Pointer to output stream
> > > > > + *
> > > > > + * Get the minimum and maximum crop bounds for \a stream. The converter
> > > > > + * should supporting cropping (Feature::Crop).
> > > > > + *
> > > > > + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> > > > > + * crop bound respectively.
> > > > > + */
> > > > > +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> > > > > +{
> > > > > +	const StreamConfiguration &config = stream->configuration();
> > > > > +	Rectangle rect;
> > > > > +
> > > > > +	if (!(getFeatures() & Feature::Crop))
> > > > > +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > > > +
> > > > > +	/*
> > > > > +	 * This is base implementation for the Converter class, so just return
> > > > > +	 * the stream configured size as minimum and maximum crop bounds.
> > > > > +	 */
> > > > > +	rect.size() = config.size;
> > > > > +
> > > > > +	return { rect, rect };
> > > > I would rather not make it part of the base class interface
> > > no? If the crop is a feature, getting the supported bounds would be
> > > something callers would like to know. Do you think this should be left only
> > > to derived converter classes ?
> > Only to the hierarchy of derived classes that supports it, like the
> > classes derived from V4L2M2MConverter (and the class itself). For some
> > other "converters" cropping and scaling might not make sense at all.
> >
> > > > > +}
> > > > > +
> > > > >    /**
> > > > >     * \var Converter::inputBufferReady
> > > > >     * \brief A signal emitted when the input frame buffer completes
> > > > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > > index 4aeb7dd9..eaae3528 100644
> > > > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> > > > >    	return 0;
> > > > >    }
> > > > >
> > > > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> > > > > +{
> > > > > +	int ret = m2m_->output()->setSelection(target, rect);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >    std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> > > > >    {
> > > > >    	return stream_->configuration().toString();
> > > > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> > > > >    	return iter->second->exportBuffers(count, buffers);
> > > > >    }
> > > > >
> > > > > +/**
> > > > > + * \copydoc libcamera::Converter::setCrop
> > > > > + */
> > > > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> > > > > +{
> > > > > +	if (!(getFeatures() & Feature::Crop))
> > > > > +		return -ENOTSUP;
> > > > > +
> > > > > +	auto iter = streams_.find(stream);
> > > > > +	if (iter == streams_.end())
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \copydoc libcamera::Converter::getCropBounds
> > > > > + */
> > > > > +std::pair<Rectangle, Rectangle>
> > > > > +V4L2M2MConverter::getCropBounds(const Stream *stream)
> > > > > +{
> > > > > +	Rectangle minCrop;
> > > > > +	Rectangle maxCrop;
> > > > > +	int ret;
> > > > > +
> > > > > +	minCrop.width = 1;
> > > > > +	minCrop.height = 1;
> > > > > +	maxCrop.width = UINT_MAX;
> > > > > +	maxCrop.height = UINT_MAX;
> > > > You can move all of this after the check for the feature support
> > > >
> > > > > +
> > > > > +	if (!(getFeatures() & Feature::Crop)) {
> > > > > +		LOG(Converter, Error) << "Crop functionality is not supported";
> > > > > +		return {};
> > > > > +	}
> > > > > +
> > > > > +	auto iter = streams_.find(stream);
> > > > > +	if (iter == streams_.end()) {
> > > > > +		/*
> > > > > +		 * No streams configured, return minimum and maximum crop
> > > > > +		 * bounds at initialization.
> > > > > +		 */
> > > > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> > > > > +		if (ret) {
> > > > > +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > > > +			return {};
> > > > > +		}
> > > > > +
> > > > > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > > > > +		if (ret) {
> > > > > +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > > +			return {};
> > > > > +		}
> > > > > +
> > > > > +		return { minCrop, maxCrop };
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * If the streams are configured, return bounds from according to
> > > > > +	 * stream configuration.
> > > > > +	 */
> > > > > +	ret = setCrop(stream, &minCrop);
> > > > > +	if (ret) {
> > > > > +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > > > +		return {};
> > > > > +	}
> > > > > +
> > > > > +	ret = setCrop(stream, &maxCrop);
> > > > > +	if (ret) {
> > > > > +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > > +		return {};
> > > > > +	}
> > > > > +
> > > > > +	return { minCrop, maxCrop };
> > > > > +}
> > > > > +
> > > > >    /**
> > > > >     * \copydoc libcamera::Converter::start
> > > > >     */
> > > > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> > > > >    	return 0;
> > > > >    }
> > > > >
> > > > > +/*
> > > > > + * \todo: This should be extended to include Feature::Flag to denote
> > > > > + * what each converter supports feature-wise.
> > > > > + */
> > > > That's another option.
> > > >
> > > > I'll send to the list the patches I have on top of your series that
> > > > makes it possible to specify features at registration time for a
> > > > comparison.
> > > Right , this bit is something I have to develop once I get a testing setup.
> > >
> > > The compatibles below are currently used for simple pipeline handler only.
> > > > >    static std::initializer_list<std::string> compatibles = {
> > > > >    	"mtk-mdp",
> > > > >    	"pxp",
> > > > > --
> > > > > 2.45.2
> > > > >
>

Hi Umang,
  one smaller nit

On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
> If the converter has cropping capability, the interface should support it
> by providing appropriate virtual functions. Provide Feature::Crop in
> Feature enumeration for the same.
>
> Provide virtual setCrop() and getCropBounds() interfaces so that
> the converter can implement their own cropping functionality.
>
> The V4L2M2MConverter implements these interfaces of the Converter
> interface. Not all V4L2M2M converters will have cropping capability,
> hence Feature::Crop should be set while construction for all those
> converters.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/internal/converter.h        |  5 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>  src/libcamera/converter.cpp                   | 51 +++++++++++
>  .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>  4 files changed, 150 insertions(+)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 7e478356..9b1223fd 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -14,6 +14,7 @@
>  #include <memory>
>  #include <string>
>  #include <tuple>
> +#include <utility>
>  #include <vector>
>
>  #include <libcamera/base/class.h>
> @@ -35,6 +36,7 @@ class Converter
>  public:
>  	enum class Feature {
>  		None = 0,
> +		Crop = (1 << 0),
>  	};
>
>  	using Features = Flags<Feature>;
> @@ -63,6 +65,9 @@ public:
>  	virtual int queueBuffers(FrameBuffer *input,
>  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>
> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 91701dbe..2697eed9 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -30,6 +30,7 @@ class Size;
>  class SizeRange;
>  class Stream;
>  struct StreamConfiguration;
> +class Rectangle;
>  class V4L2M2MDevice;
>
>  class V4L2M2MConverter : public Converter
> @@ -57,6 +58,9 @@ public:
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>
> +	int setCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> +
>  private:
>  	class V4L2M2MStream : protected Loggable
>  	{
> @@ -75,6 +79,8 @@ private:
>
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>
> +		int setSelection(unsigned int target, Rectangle *rect);
> +
>  	protected:
>  		std::string logPrefix() const override;
>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 2c3da6d4..d51e77a0 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -11,6 +11,8 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/media_device.h"
>
>  /**
> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief Specify the features supported by the converter
>   * \var Converter::Feature::None
>   * \brief No extra features supported by the converter
> + * \var Converter::Feature::Crop
> + * \brief Cropping capability is supported by the converter
>   */
>
>  /**
> @@ -161,6 +165,53 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>
> +/**
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to be applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::Crop flag in this case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream
> + * \param[in] stream Pointer to output stream
> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::Crop).
> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	Rectangle rect;
> +
> +	if (!(getFeatures() & Feature::Crop))
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +
> +	/*
> +	 * This is base implementation for the Converter class, so just return
> +	 * the stream configured size as minimum and maximum crop bounds.
> +	 */
> +	rect.size() = config.size;
> +
> +	return { rect, rect };
> +}
> +
>  /**
>   * \var Converter::inputBufferReady
>   * \brief A signal emitted when the input frame buffer completes
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 4aeb7dd9..eaae3528 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>  	return 0;
>  }
>
> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> +{
> +	int ret = m2m_->output()->setSelection(target, rect);

You can just
        return m2m_->output()->setSelection(target, rect);

as

 * \return 0 on success or a negative error code otherwise
 */
int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)


> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>  {
>  	return stream_->configuration().toString();
> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>  	return iter->second->exportBuffers(count, buffers);
>  }
>
> +/**
> + * \copydoc libcamera::Converter::setCrop
> + */
> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> +{
> +	if (!(getFeatures() & Feature::Crop))
> +		return -ENOTSUP;
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end())
> +		return -EINVAL;
> +
> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::getCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::getCropBounds(const Stream *stream)
> +{
> +	Rectangle minCrop;
> +	Rectangle maxCrop;
> +	int ret;
> +
> +	minCrop.width = 1;
> +	minCrop.height = 1;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;
> +
> +	if (!(getFeatures() & Feature::Crop)) {
> +		LOG(Converter, Error) << "Crop functionality is not supported";
> +		return {};
> +	}
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end()) {
> +		/*
> +		 * No streams configured, return minimum and maximum crop
> +		 * bounds at initialization.
> +		 */
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
> +			return {};
> +		}
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
> +			return {};
> +		}
> +
> +		return { minCrop, maxCrop };
> +	}
> +
> +	/*
> +	 * If the streams are configured, return bounds from according to
> +	 * stream configuration.
> +	 */
> +	ret = setCrop(stream, &minCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
> +		return {};
> +	}
> +
> +	ret = setCrop(stream, &maxCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
> +		return {};
> +	}
> +
> +	return { minCrop, maxCrop };
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>  	return 0;
>  }
>
> +/*
> + * \todo: This should be extended to include Feature::Flag to denote
> + * what each converter supports feature-wise.
> + */
>  static std::initializer_list<std::string> compatibles = {
>  	"mtk-mdp",
>  	"pxp",
> --
> 2.45.2
>

Hi Jacopo

On 22/07/24 3:02 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Fri, Jul 19, 2024 at 03:31:48PM GMT, Umang Jain wrote:
>> Hi Jacopo
>>
>> On 18/07/24 3:36 pm, Jacopo Mondi wrote:
>>> Hi Umang
>>>
>>> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
>>>> Hi Jacopo
>>>>
>>>> On 17/07/24 3:32 pm, Jacopo Mondi wrote:
>>>>> Hi Umang
>>>>>
>>>>> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:
>>>>>> If the converter has cropping capability, the interface should support it
>>>>>> by providing appropriate virtual functions. Provide Feature::Crop in
>>>>>> Feature enumeration for the same.
>>>>>>
>>>>>> Provide virtual setCrop() and getCropBounds() interfaces so that
>>>>>> the converter can implement their own cropping functionality.
>>>>>>
>>>>>> The V4L2M2MConverter implements these interfaces of the Converter
>>>>>> interface. Not all V4L2M2M converters will have cropping capability,
>>>>>> hence Feature::Crop should be set while construction for all those
>>>>>> converters.
>>>>>>
>>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>>>> ---
>>>>>>     include/libcamera/internal/converter.h        |  5 ++
>>>>>>     .../internal/converter/converter_v4l2_m2m.h   |  6 ++
>>>>>>     src/libcamera/converter.cpp                   | 51 +++++++++++
>>>>>>     .../converter/converter_v4l2_m2m.cpp          | 88 +++++++++++++++++++
>>>>>>     4 files changed, 150 insertions(+)
>>>>>>
>>>>>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
>>>>>> index 7e478356..9b1223fd 100644
>>>>>> --- a/include/libcamera/internal/converter.h
>>>>>> +++ b/include/libcamera/internal/converter.h
>>>>>> @@ -14,6 +14,7 @@
>>>>>>     #include <memory>
>>>>>>     #include <string>
>>>>>>     #include <tuple>
>>>>>> +#include <utility>
>>>>>>     #include <vector>
>>>>>>
>>>>>>     #include <libcamera/base/class.h>
>>>>>> @@ -35,6 +36,7 @@ class Converter
>>>>>>     public:
>>>>>>     	enum class Feature {
>>>>>>     		None = 0,
>>>>>> +		Crop = (1 << 0),
>>>>>>     	};
>>>>>>
>>>>>>     	using Features = Flags<Feature>;
>>>>>> @@ -63,6 +65,9 @@ public:
>>>>>>     	virtual int queueBuffers(FrameBuffer *input,
>>>>>>     				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>>>>>>
>>>>>> +	virtual int setCrop(const Stream *stream, Rectangle *rect);
>>>>> "crop" is loosely defined as a concept.
>>>>> A converter has an input and an output and I presume it scales.
>>>>>
>>>>> Where is the crop rectangle applied ?
>>>> A converter is a processing block - outside of ISP. Hence there is a input
>>>> and output, and the crop is applied at/w.r.t input. The resultant should be
>>>> obtained/visible on the output.
>>>>
>>> And where is the assumption that crop is applied to the input
>>> documented ? Aren't there converter that allow to crop on the output
>>> maybe -after- scaling ?
>>>
>>> Cropping before or after scaling (or, if you want to put it different,
>>> cropping on the input or on the output) are both possible options, and
>>> this interface doesn't allow you to specify that.
>> cropping after scaling means the application would get a different image
>> output (if it tries to set crop on the output) ? Is that possible, a runtime
>> crop ? Image output size differs everytime a crop is applied ?
> I don't think runtime crops are possible neither on the 'output video
> device' or on the 'capture video device' as it would require changing
> the buffer sizes at runtime. BUT my experience with m2m devices is
> limited so..
>
> What I meant is slightly different. Given an input buffer size of WxH
> your converter will output a frame of W'xH'
>
> To get from W to W' and from H to H' you can combine cropping and
> scaling, theoretically freely. The canonical choice is to crop first
> then scale, in order to maintain square pixels. In example if you have
> an 1080p input frame (16:9) and you want to get to a 640x480 (4:3)
> output, if you only scale you will change the image proportions.
> Generally it is good practice to crop down 1080p to the closest 4:3
> resolution, and then scale it down. So yes, "setCrop()" on the input
> is the most reasonable choice. Due to my limited knowledge of
> converters, I was wondering if other combinations of cropping/scaling
> would be possible, or if multiple cropping points might be available
> for a converter and if a generic "setCrop()" interface is
> expressive enough to support more complex devices.
>
> The setCrop() interface you have designed is "libcamera::Stream" oriented,
> and a libcamera::Stream is associated in this class to a V4L2M2MStream
> which contains an "input" and an "output". As the class interface
> already expresses the "input" and "output" concepts (in example in
> configure()) would it be an overkill to make it part of the setCrop()
> interface as well ? (either with an INPUT/OUTPUT flag or by making
> this a setInputCrop() function) ?
>
>> I think what you are referring here is the compose rectangle, rather than
>> the crop rectangle? Is that correct?
>>
>>> You have wired the V4L2 converter to apply selection on the TGT_CROP
>>> target on the input queue (aka on the 'output' video device, oh funny
>>> names)
>>>
>>> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
>>> {
>>>
>>> 	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
>>> }
>>>
>>>
>>> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
>>> {
>>> 	int ret = m2m_->output()->setSelection(target, rect);
>>> 	if (ret < 0)
>>> 		return ret;
>>>
>>> 	return 0;
>>> }
>>>
>>> Can't a converter expose a TGT_CROP target on the capture device to allow
>>> userspace to apply a crop on the "final" image before returning it to
>>> applications ?
>> So having read
>> https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, it
>> seems TGT_CROP can be target for video capture as well. There is no
>> limitation on that front.
>>
>> However, I can't wrap around my head around that kind of crop - applied at
>> runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes more sense
>> to me, rather than exposing TGT_CROP.
> These are V4L2 specific concept. How a device expose selection targets
> and how they should be used I presume will be handled by the dedicated
> derived class.
>
> What matters for me at the moment is the generic Converter interface
> design.

Makes sense.

I will make the setCrop() as setInputCrop() in the generic Converter 
interface.
And will also document as the Crop set on the input stream to the 
converter. Hopefully, this will help resolve ambiguity.


The configure() has a Input stream and no. of output streams.. So 
setInputCrop() will corroborate to the input stream, used while 
configuring the converter.

>
>> Certainly the interface is lacking such a distinction in this form - and I
>> should clarify that the crop would be set on the input video signal. Would
>> that makes things acceptable?
>>
>> I am also a bit wary about how should we be exposing such a interface for
>> converters, for output and video capture devices, have TGT_CROP and
>> TGT_COMPOSE targets ... its seems hairy for now, to me..
>>
>>>>> Does this need to be part of the base class interface ? Or should only
>>>>> derived classes that support it implement it ? (it's reasonable for a
>>>>> V4L2 M2M device to support this, once better specified the interface
>>>>> where the crop rectangle has to be applied).
>>>> I think so. Atleast that's what Laurent initally asked for. Converter
>>>> interface should dictate the features supported, the implementation left to
>>>> the derived converter classes. No implementation specific details should be
>>>> handled by the interface itself, only the "feature" part
>>>>
>>> Yes, but concepts like 'crop' and 'selection' might only apply to
>>> certain types of converter, like the v4l2 m2m ones. I wonder if it is
>>> better to provide a stub implementation in the whole hierarcy or only
>>> expose such methods in the classes derived from, in example,
>>> V4L2M2MConverter.
>>>
>>>>>> +	virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
>>>>> not "get" in getters
>>>> ah yes
>>>>>> +
>>>>>>     	Signal<FrameBuffer *> inputBufferReady;
>>>>>>     	Signal<FrameBuffer *> outputBufferReady;
>>>>>>
>>>>>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>>>> index 91701dbe..2697eed9 100644
>>>>>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>>>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>>>>>> @@ -30,6 +30,7 @@ class Size;
>>>>>>     class SizeRange;
>>>>>>     class Stream;
>>>>>>     struct StreamConfiguration;
>>>>>> +class Rectangle;
>>>>>>     class V4L2M2MDevice;
>>>>>>
>>>>>>     class V4L2M2MConverter : public Converter
>>>>>> @@ -57,6 +58,9 @@ public:
>>>>>>     	int queueBuffers(FrameBuffer *input,
>>>>>>     			 const std::map<const Stream *, FrameBuffer *> &outputs);
>>>>>>
>>>>>> +	int setCrop(const Stream *stream, Rectangle *rect);
>>>>>> +	std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
>>>>>> +
>>>>>>     private:
>>>>>>     	class V4L2M2MStream : protected Loggable
>>>>>>     	{
>>>>>> @@ -75,6 +79,8 @@ private:
>>>>>>
>>>>>>     		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>>>>>>
>>>>>> +		int setSelection(unsigned int target, Rectangle *rect);
>>>>>> +
>>>>>>     	protected:
>>>>>>     		std::string logPrefix() const override;
>>>>>>
>>>>>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>>>>>> index 2c3da6d4..d51e77a0 100644
>>>>>> --- a/src/libcamera/converter.cpp
>>>>>> +++ b/src/libcamera/converter.cpp
>>>>>> @@ -11,6 +11,8 @@
>>>>>>
>>>>>>     #include <libcamera/base/log.h>
>>>>>>
>>>>>> +#include <libcamera/stream.h>
>>>>>> +
>>>>>>     #include "libcamera/internal/media_device.h"
>>>>>>
>>>>>>     /**
>>>>>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
>>>>>>      * \brief Specify the features supported by the converter
>>>>>>      * \var Converter::Feature::None
>>>>>>      * \brief No extra features supported by the converter
>>>>>> + * \var Converter::Feature::Crop
>>>>>> + * \brief Cropping capability is supported by the converter
>>>>>>      */
>>>>>>
>>>>>>     /**
>>>>>> @@ -161,6 +165,53 @@ Converter::~Converter()
>>>>>>      * \return 0 on success or a negative error code otherwise
>>>>>>      */
>>>>>>
>>>>>> +/**
>>>>>> + * \brief Set the crop rectangle \a rect for \a stream
>>>>>> + * \param[in] stream Pointer to output stream
>>>>>> + * \param[inout] rect The crop rectangle to be applied
>>>>>> + *
>>>>>> + * Set the crop rectangle \a rect for \a stream provided the converter supports
>>>>>> + * cropping. The converter should have the Feature::Crop flag in this case.
>>>>>> + *
>>>>>> + * \return 0 on success or a negative error code otherwise
>>>>>> + */
>>>>>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
>>>>>> +{
>>>>>> +	if (!(getFeatures() & Feature::Crop)) {
>>>>>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>>>>>> +		return -ENOTSUP;
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * \brief Get the crop bounds \a stream
>>>>>> + * \param[in] stream Pointer to output stream
>>>>>> + *
>>>>>> + * Get the minimum and maximum crop bounds for \a stream. The converter
>>>>>> + * should supporting cropping (Feature::Crop).
>>>>>> + *
>>>>>> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
>>>>>> + * crop bound respectively.
>>>>>> + */
>>>>>> +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
>>>>>> +{
>>>>>> +	const StreamConfiguration &config = stream->configuration();
>>>>>> +	Rectangle rect;
>>>>>> +
>>>>>> +	if (!(getFeatures() & Feature::Crop))
>>>>>> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * This is base implementation for the Converter class, so just return
>>>>>> +	 * the stream configured size as minimum and maximum crop bounds.
>>>>>> +	 */
>>>>>> +	rect.size() = config.size;
>>>>>> +
>>>>>> +	return { rect, rect };
>>>>> I would rather not make it part of the base class interface
>>>> no? If the crop is a feature, getting the supported bounds would be
>>>> something callers would like to know. Do you think this should be left only
>>>> to derived converter classes ?
>>> Only to the hierarchy of derived classes that supports it, like the
>>> classes derived from V4L2M2MConverter (and the class itself). For some
>>> other "converters" cropping and scaling might not make sense at all.
>>>
>>>>>> +}
>>>>>> +
>>>>>>     /**
>>>>>>      * \var Converter::inputBufferReady
>>>>>>      * \brief A signal emitted when the input frame buffer completes
>>>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>>>> index 4aeb7dd9..eaae3528 100644
>>>>>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>>>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>>>>>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
>>>>>> +{
>>>>>> +	int ret = m2m_->output()->setSelection(target, rect);
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>     std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>>>>>>     {
>>>>>>     	return stream_->configuration().toString();
>>>>>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>>>>>>     	return iter->second->exportBuffers(count, buffers);
>>>>>>     }
>>>>>>
>>>>>> +/**
>>>>>> + * \copydoc libcamera::Converter::setCrop
>>>>>> + */
>>>>>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
>>>>>> +{
>>>>>> +	if (!(getFeatures() & Feature::Crop))
>>>>>> +		return -ENOTSUP;
>>>>>> +
>>>>>> +	auto iter = streams_.find(stream);
>>>>>> +	if (iter == streams_.end())
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * \copydoc libcamera::Converter::getCropBounds
>>>>>> + */
>>>>>> +std::pair<Rectangle, Rectangle>
>>>>>> +V4L2M2MConverter::getCropBounds(const Stream *stream)
>>>>>> +{
>>>>>> +	Rectangle minCrop;
>>>>>> +	Rectangle maxCrop;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	minCrop.width = 1;
>>>>>> +	minCrop.height = 1;
>>>>>> +	maxCrop.width = UINT_MAX;
>>>>>> +	maxCrop.height = UINT_MAX;
>>>>> You can move all of this after the check for the feature support
>>>>>
>>>>>> +
>>>>>> +	if (!(getFeatures() & Feature::Crop)) {
>>>>>> +		LOG(Converter, Error) << "Crop functionality is not supported";
>>>>>> +		return {};
>>>>>> +	}
>>>>>> +
>>>>>> +	auto iter = streams_.find(stream);
>>>>>> +	if (iter == streams_.end()) {
>>>>>> +		/*
>>>>>> +		 * No streams configured, return minimum and maximum crop
>>>>>> +		 * bounds at initialization.
>>>>>> +		 */
>>>>>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
>>>>>> +		if (ret) {
>>>>>> +			LOG(Converter, Error) << "Failed to query minimum crop bound";
>>>>>> +			return {};
>>>>>> +		}
>>>>>> +
>>>>>> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
>>>>>> +		if (ret) {
>>>>>> +			LOG(Converter, Error) << "Failed to query maximum crop bound";
>>>>>> +			return {};
>>>>>> +		}
>>>>>> +
>>>>>> +		return { minCrop, maxCrop };
>>>>>> +	}
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If the streams are configured, return bounds from according to
>>>>>> +	 * stream configuration.
>>>>>> +	 */
>>>>>> +	ret = setCrop(stream, &minCrop);
>>>>>> +	if (ret) {
>>>>>> +		LOG(Converter, Error) << "Failed to query minimum crop bound";
>>>>>> +		return {};
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = setCrop(stream, &maxCrop);
>>>>>> +	if (ret) {
>>>>>> +		LOG(Converter, Error) << "Failed to query maximum crop bound";
>>>>>> +		return {};
>>>>>> +	}
>>>>>> +
>>>>>> +	return { minCrop, maxCrop };
>>>>>> +}
>>>>>> +
>>>>>>     /**
>>>>>>      * \copydoc libcamera::Converter::start
>>>>>>      */
>>>>>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +/*
>>>>>> + * \todo: This should be extended to include Feature::Flag to denote
>>>>>> + * what each converter supports feature-wise.
>>>>>> + */
>>>>> That's another option.
>>>>>
>>>>> I'll send to the list the patches I have on top of your series that
>>>>> makes it possible to specify features at registration time for a
>>>>> comparison.
>>>> Right , this bit is something I have to develop once I get a testing setup.
>>>>
>>>> The compatibles below are currently used for simple pipeline handler only.
>>>>>>     static std::initializer_list<std::string> compatibles = {
>>>>>>     	"mtk-mdp",
>>>>>>     	"pxp",
>>>>>> --
>>>>>> 2.45.2
>>>>>>