[v7,2/4] libcamera: converter: Add interface to support cropping capability

21170 diff mbox series

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

Provide virtual setInputCrop() and inputCropBounds() 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 ability
on its input, hence it needs to discovered during construction time.
If the capability to crop to identified successfully, the cropping
bounds are determined during configure() time.

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

Hi Umang,

Thank you for the patch. 

On Fri, Sep 06, 2024 at 12:04:42PM +0530, Umang Jain wrote:
> If the converter has cropping capability on its input, the interface
> should support it by providing appropriate virtual functions. Provide
> Feature::InputCrop in Feature enumeration for the same.
> 
> Provide virtual setInputCrop() and inputCropBounds() 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 ability
> on its input, hence it needs to discovered during construction time.
> If the capability to crop to identified successfully, the cropping
> bounds are determined during configure() time.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/internal/converter.h        |   5 +
>  .../internal/converter/converter_v4l2_m2m.h   |  13 ++-
>  src/libcamera/converter.cpp                   |  38 +++++++
>  .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++++++++++-
>  4 files changed, 158 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 6623de4d..ffbb6f34 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,
> +		InputCrop = (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 setInputCrop(const Stream *stream, Rectangle *rect) = 0;
> +	virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;
> +
>  	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 b9e59899..7d321c85 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,11 +58,15 @@ public:
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>  
> +	int setInputCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream);
> +
>  private:
>  	class V4L2M2MStream : protected Loggable
>  	{
>  	public:
> -		V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);
> +		V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> +			      Features features);
>  
>  		bool isValid() const { return m2m_ != nullptr; }
>  
> @@ -75,6 +80,9 @@ private:
>  
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> +		int setInputSelection(unsigned int target, Rectangle *rect);
> +		std::pair<Rectangle, Rectangle> inputCropBounds();
> +
>  	protected:
>  		std::string logPrefix() const override;
>  
> @@ -88,6 +96,9 @@ private:
>  
>  		unsigned int inputBufferCount_;
>  		unsigned int outputBufferCount_;
> +
> +		std::pair<Rectangle, Rectangle> inputCropBounds_;
> +		Features features_;
>  	};
>  
>  	std::unique_ptr<V4L2M2MDevice> m2m_;
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index d7bb7273..7cb6532a 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::InputCrop
> + * \brief Cropping capability at input is supported by the converter
>   */
>  
>  /**
> @@ -161,6 +165,40 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>  
> +/**
> + * \fn Converter::setInputCrop()
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to apply and return the rectangle
> + * that is actually applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter has the Feature::InputCrop flag in this case.
> + *
> + * The underlying hardware can adjust the rectangle supplied by the user
> + * due to hardware constraints. Caller can inspect \a rect to determine the
> + * actual rectangle that has been applied by the converter, after this function
> + * returns.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn Converter::inputCropBounds()
> + * \brief Retrieve the crop bounds for \a stream
> + * \param[in] stream The output stream
> + *
> + * Retrieve the minimum and maximum crop bounds for \a stream. The converter
> + * should support cropping (Feature::InputCrop).
> + *
> + * The crop bounds depend on the configuration of the output stream hence, this
> + * function should be called after the \a stream has been configured using
> + * configure().
> + *
> + * \return A std::pair<Rectangle, Rectangle> containing minimum and maximum
> + * crop bound respectively.
> + */
> +
>  /**
>   * \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 4f3e8ce4..1bf47ff9 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -34,8 +34,9 @@ LOG_DECLARE_CATEGORY(Converter)
>   * V4L2M2MConverter::V4L2M2MStream
>   */
>  
> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)
> -	: converter_(converter), stream_(stream)
> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> +					       Features features)
> +	: converter_(converter), stream_(stream), features_(features)
>  {
>  	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());
>  
> @@ -97,6 +98,33 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC
>  	inputBufferCount_ = inputCfg.bufferCount;
>  	outputBufferCount_ = outputCfg.bufferCount;
>  
> +	if (features_ & Feature::InputCrop) {

I don't fully understand why the stream has it's own features_ member.
Wouldn't it be sufficient to do

if(converter_->features() & Feature::InputCrop) {

or did I miss something?

> +		Rectangle minCrop;
> +		Rectangle maxCrop;
> +
> +		/* Find crop bounds */
> +		minCrop.width = 1;
> +		minCrop.height = 1;
> +		maxCrop.width = UINT_MAX;
> +		maxCrop.height = UINT_MAX;
> +
> +		ret = setInputSelection(V4L2_SEL_TGT_CROP, &minCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Could not query minimum selection crop"
> +					      << strerror(-ret);
> +			return ret;
> +		}
> +
> +		ret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Could not query maximum selection crop"
> +					      << strerror(-ret);
> +			return ret;
> +		}
> +
> +		inputCropBounds_ = { minCrop, maxCrop };
> +	}
> +
>  	return 0;
>  }
>  
> @@ -154,6 +182,20 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>  	return 0;
>  }
>  
> +int V4L2M2MConverter::V4L2M2MStream::setInputSelection(unsigned int target, Rectangle *rect)
> +{
> +	int ret = m2m_->output()->setSelection(target, rect);
> +	if (ret < 0)

Why the explicit check for < 0 and not if(ret)... that is used elsewhere?



> +		return ret;
> +
> +	return 0;
> +}
> +
> +std::pair<Rectangle, Rectangle> V4L2M2MConverter::V4L2M2MStream::inputCropBounds()
> +{
> +	return inputCropBounds_;
> +}
> +
>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
>  {
>  	return stream_->configuration().toString();
> @@ -204,6 +246,34 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
>  		m2m_.reset();
>  		return;
>  	}
> +
> +	/* Discover Feature::InputCrop */
> +	Rectangle maxCrop;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;
> +
> +	ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +	if (!ret) {

You could early out and save an indentation with 

if (ret)
	return;

With or without the changes:
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan

> +		/*
> +		 * Rectangles for cropping targets are defined even if the device
> +		 * does not support cropping. Their sizes and positions will be
> +		 * fixed in such cases.
> +		 *
> +		 * Set and inspect a crop equivalent to half of the maximum crop
> +		 * returned earlier. Use this to determine whether the crop on
> +		 * input is really supported.
> +		 */
> +		Rectangle halfCrop(maxCrop.size() / 2);
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);
> +		if (!ret && halfCrop.width != maxCrop.width &&
> +		    halfCrop.height != maxCrop.height) {
> +			features_ |= Feature::InputCrop;
> +
> +			LOG(Converter, Info)
> +				<< "Converter supports cropping on its input";
> +		}
> +	}
>  }
>  
>  /**
> @@ -336,7 +406,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
>  	for (unsigned int i = 0; i < outputCfgs.size(); ++i) {
>  		const StreamConfiguration &cfg = outputCfgs[i];
>  		std::unique_ptr<V4L2M2MStream> stream =
> -			std::make_unique<V4L2M2MStream>(this, cfg.stream());
> +			std::make_unique<V4L2M2MStream>(this, cfg.stream(), features_);
>  
>  		if (!stream->isValid()) {
>  			LOG(Converter, Error)
> @@ -373,6 +443,36 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
>  	return iter->second->exportBuffers(count, buffers);
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::setInputCrop
> + */
> +int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)
> +{
> +	if (!(features_ & Feature::InputCrop))
> +		return -ENOTSUP;
> +
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end()) {
> +		LOG(Converter, Error) << "Invalid output stream";
> +		return -EINVAL;
> +	}
> +
> +	return iter->second->setInputSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::inputCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::inputCropBounds(const Stream *stream)
> +{
> +	auto iter = streams_.find(stream);
> +	if (iter == streams_.end())
> +		return {};
> +
> +	return iter->second->inputCropBounds();
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> -- 
> 2.45.0
>

On Tue, Sep 24, 2024 at 09:50:31PM +0200, Stefan Klug wrote:
> Hi Umang,
> 
> Thank you for the patch. 
> 
> On Fri, Sep 06, 2024 at 12:04:42PM +0530, Umang Jain wrote:
> > If the converter has cropping capability on its input, the interface
> > should support it by providing appropriate virtual functions. Provide
> > Feature::InputCrop in Feature enumeration for the same.
> > 
> > Provide virtual setInputCrop() and inputCropBounds() interfaces so that
> > the converter can implement their own cropping functionality.

s/their/its/

> > 
> > The V4L2M2MConverter implements these interfaces of the Converter
> > interface. Not all V4L2M2M converters will have cropping ability
> > on its input, hence it needs to discovered during construction time.

s/to discovered during/to be discovered at/

> > If the capability to crop to identified successfully, the cropping

s/to identified/is identified/

> > bounds are determined during configure() time.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  include/libcamera/internal/converter.h        |   5 +
> >  .../internal/converter/converter_v4l2_m2m.h   |  13 ++-
> >  src/libcamera/converter.cpp                   |  38 +++++++
> >  .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++++++++++-
> >  4 files changed, 158 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > index 6623de4d..ffbb6f34 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,
> > +		InputCrop = (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 setInputCrop(const Stream *stream, Rectangle *rect) = 0;
> > +	virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;
> > +
> >  	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 b9e59899..7d321c85 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,11 +58,15 @@ public:
> >  	int queueBuffers(FrameBuffer *input,
> >  			 const std::map<const Stream *, FrameBuffer *> &outputs);
> >  
> > +	int setInputCrop(const Stream *stream, Rectangle *rect);
> > +	std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream);
> > +
> >  private:
> >  	class V4L2M2MStream : protected Loggable
> >  	{
> >  	public:
> > -		V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);
> > +		V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> > +			      Features features);
> >  
> >  		bool isValid() const { return m2m_ != nullptr; }
> >  
> > @@ -75,6 +80,9 @@ private:
> >  
> >  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> >  
> > +		int setInputSelection(unsigned int target, Rectangle *rect);
> > +		std::pair<Rectangle, Rectangle> inputCropBounds();
> > +
> >  	protected:
> >  		std::string logPrefix() const override;
> >  
> > @@ -88,6 +96,9 @@ private:
> >  
> >  		unsigned int inputBufferCount_;
> >  		unsigned int outputBufferCount_;
> > +
> > +		std::pair<Rectangle, Rectangle> inputCropBounds_;
> > +		Features features_;
> >  	};
> >  
> >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index d7bb7273..7cb6532a 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::InputCrop
> > + * \brief Cropping capability at input is supported by the converter
> >   */
> >  
> >  /**
> > @@ -161,6 +165,40 @@ Converter::~Converter()
> >   * \return 0 on success or a negative error code otherwise
> >   */
> >  
> > +/**
> > + * \fn Converter::setInputCrop()
> > + * \brief Set the crop rectangle \a rect for \a stream
> > + * \param[in] stream Pointer to output stream

s/Pointer to/The/

(consistent with the documentation below)

> > + * \param[inout] rect The crop rectangle to apply and return the rectangle
> > + * that is actually applied
> > + *
> > + * Set the crop rectangle \a rect for \a stream provided the converter supports
> > + * cropping. The converter has the Feature::InputCrop flag in this case.
> > + *
> > + * The underlying hardware can adjust the rectangle supplied by the user
> > + * due to hardware constraints. Caller can inspect \a rect to determine the

s/Caller/The caller/

> > + * actual rectangle that has been applied by the converter, after this function
> > + * returns.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn Converter::inputCropBounds()
> > + * \brief Retrieve the crop bounds for \a stream
> > + * \param[in] stream The output stream
> > + *
> > + * Retrieve the minimum and maximum crop bounds for \a stream. The converter
> > + * should support cropping (Feature::InputCrop).
> > + *
> > + * The crop bounds depend on the configuration of the output stream hence, this

s/hence, this/and hence this/

> > + * function should be called after the \a stream has been configured using
> > + * configure().
> > + *
> > + * \return A std::pair<Rectangle, Rectangle> containing minimum and maximum

s/std::pair<Rectangle, Rectangle>/pair/ (the type is already indicated
by the function prototype).

s/containing/containing the/

> > + * crop bound respectively.

s/respectively/in that order/

> > + */
> > +
> >  /**
> >   * \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 4f3e8ce4..1bf47ff9 100644
> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > @@ -34,8 +34,9 @@ LOG_DECLARE_CATEGORY(Converter)
> >   * V4L2M2MConverter::V4L2M2MStream
> >   */
> >  
> > -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)
> > -	: converter_(converter), stream_(stream)
> > +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> > +					       Features features)
> > +	: converter_(converter), stream_(stream), features_(features)
> >  {
> >  	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());
> >  
> > @@ -97,6 +98,33 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC
> >  	inputBufferCount_ = inputCfg.bufferCount;
> >  	outputBufferCount_ = outputCfg.bufferCount;
> >  
> > +	if (features_ & Feature::InputCrop) {
> 
> I don't fully understand why the stream has it's own features_ member.
> Wouldn't it be sufficient to do
> 
> if(converter_->features() & Feature::InputCrop) {
> 
> or did I miss something?

I would also drop the member variable.

> > +		Rectangle minCrop;
> > +		Rectangle maxCrop;
> > +
> > +		/* Find crop bounds */
> > +		minCrop.width = 1;
> > +		minCrop.height = 1;
> > +		maxCrop.width = UINT_MAX;
> > +		maxCrop.height = UINT_MAX;
> > +
> > +		ret = setInputSelection(V4L2_SEL_TGT_CROP, &minCrop);
> > +		if (ret) {
> > +			LOG(Converter, Error) << "Could not query minimum selection crop"

s/crop"/crop: "/

But it should be formatted as

			LOG(Converter, Error)
				<< "Could not query minimum selection crop: "
				<< strerror(-ret);

> > +					      << strerror(-ret);
> > +			return ret;
> > +		}
> > +
> > +		ret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);

Shouldn't this be instead implemented by getting
V4L2_SEL_TGT_CROP_BOUNDS ? I've sent a patch that adds support for
V4L2VideoDevice::getSelection() ("[PATCH] libcamera: v4l2_videodevice:
Add getSelection() function").

> > +		if (ret) {
> > +			LOG(Converter, Error) << "Could not query maximum selection crop"

Same here.

> > +					      << strerror(-ret);
> > +			return ret;
> > +		}
> > +
> > +		inputCropBounds_ = { minCrop, maxCrop };
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -154,6 +182,20 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> >  	return 0;
> >  }
> >  
> > +int V4L2M2MConverter::V4L2M2MStream::setInputSelection(unsigned int target, Rectangle *rect)
> > +{
> > +	int ret = m2m_->output()->setSelection(target, rect);
> > +	if (ret < 0)
> 
> Why the explicit check for < 0 and not if(ret)... that is used elsewhere?

You could simply

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

> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +std::pair<Rectangle, Rectangle> V4L2M2MConverter::V4L2M2MStream::inputCropBounds()
> > +{
> > +	return inputCropBounds_;
> > +}
> > +
> >  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> >  {
> >  	return stream_->configuration().toString();
> > @@ -204,6 +246,34 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> >  		m2m_.reset();
> >  		return;
> >  	}
> > +
> > +	/* Discover Feature::InputCrop */
> > +	Rectangle maxCrop;
> > +	maxCrop.width = UINT_MAX;
> > +	maxCrop.height = UINT_MAX;
> > +
> > +	ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > +	if (!ret) {
> 
> You could early out and save an indentation with 
> 
> if (ret)
> 	return;
> 
> With or without the changes:
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 
> 
> Cheers,
> Stefan
> 
> > +		/*
> > +		 * Rectangles for cropping targets are defined even if the device
> > +		 * does not support cropping. Their sizes and positions will be
> > +		 * fixed in such cases.
> > +		 *
> > +		 * Set and inspect a crop equivalent to half of the maximum crop
> > +		 * returned earlier. Use this to determine whether the crop on
> > +		 * input is really supported.
> > +		 */
> > +		Rectangle halfCrop(maxCrop.size() / 2);
> > +
> > +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);
> > +		if (!ret && halfCrop.width != maxCrop.width &&
> > +		    halfCrop.height != maxCrop.height) {

How about

		if (!ret && halfCrop != maxCrop) {

> > +			features_ |= Feature::InputCrop;
> > +
> > +			LOG(Converter, Info)
> > +				<< "Converter supports cropping on its input";
> > +		}
> > +	}
> >  }
> >  
> >  /**
> > @@ -336,7 +406,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
> >  	for (unsigned int i = 0; i < outputCfgs.size(); ++i) {
> >  		const StreamConfiguration &cfg = outputCfgs[i];
> >  		std::unique_ptr<V4L2M2MStream> stream =
> > -			std::make_unique<V4L2M2MStream>(this, cfg.stream());
> > +			std::make_unique<V4L2M2MStream>(this, cfg.stream(), features_);
> >  
> >  		if (!stream->isValid()) {
> >  			LOG(Converter, Error)
> > @@ -373,6 +443,36 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> >  	return iter->second->exportBuffers(count, buffers);
> >  }
> >  
> > +/**
> > + * \copydoc libcamera::Converter::setInputCrop
> > + */
> > +int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)
> > +{
> > +	if (!(features_ & Feature::InputCrop))
> > +		return -ENOTSUP;
> > +
> > +	auto iter = streams_.find(stream);
> > +	if (iter == streams_.end()) {
> > +		LOG(Converter, Error) << "Invalid output stream";
> > +		return -EINVAL;
> > +	}
> > +
> > +	return iter->second->setInputSelection(V4L2_SEL_TGT_CROP, rect);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::Converter::inputCropBounds
> > + */
> > +std::pair<Rectangle, Rectangle>
> > +V4L2M2MConverter::inputCropBounds(const Stream *stream)
> > +{
> > +	auto iter = streams_.find(stream);
> > +	if (iter == streams_.end())
> > +		return {};
> > +
> > +	return iter->second->inputCropBounds();
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::start
> >   */