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

20711 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>
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                   | 52 +++++++++++
 .../converter/converter_v4l2_m2m.cpp          | 92 +++++++++++++++++++
 4 files changed, 155 insertions(+)

Hi Umang,

Thank you for the patch.

On Fri, Jul 26, 2024 at 05:17:12PM +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>
> 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                   | 52 +++++++++++
>  .../converter/converter_v4l2_m2m.cpp          | 92 +++++++++++++++++++
>  4 files changed, 155 insertions(+)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 652ff519..853888f4 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);
> +	virtual std::pair<Rectangle, Rectangle> inputCropBounds(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..5b69b14e 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 setInputCrop(const Stream *stream, Rectangle *rect);
> +	std::pair<Rectangle, Rectangle> inputCropBounds(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 e2dbf5e0..7ab56b4c 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,54 @@ 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

There's no explanation of the "out" side of "inout" in the text here.

> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::InputCrop flag in this

"should" is not a mandatory requirement.

> + * case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setInputCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> +	if (!(features() & Feature::InputCrop)) {
> +		LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;

What's the purpose of this default implementation ?

> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream

s/Get/Retrieve/
s/bounds/bounds for/

> + * \param[in] stream Pointer to output stream

s/Pointer to output stream/The output stream/

> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::InputCrop).

s/supporting/support/

> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::inputCropBounds([[maybe_unused]] const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	Rectangle rect;
> +
> +	if (!(features() & Feature::InputCrop))
> +		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.
> +	 */

Why ?

> +	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..3295b988 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,85 @@ 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())
> +		return -EINVAL;
> +
> +	return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::inputCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::inputCropBounds(const Stream *stream)
> +{
> +	Rectangle minCrop;
> +	Rectangle maxCrop;
> +	int ret;
> +
> +	if (!(features() & Feature::InputCrop)) {
> +		LOG(Converter, Error) << "Input Crop functionality is not supported";
> +		return {};
> +	}
> +
> +	minCrop.width = 1;
> +	minCrop.height = 1;
> +	maxCrop.width = UINT_MAX;
> +	maxCrop.height = UINT_MAX;
> +
> +	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"
> +					      << strerror(-ret);
> +			return {};
> +		}
> +
> +		ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> +		if (ret) {
> +			LOG(Converter, Error) << "Failed to query maximum crop bound"
> +					      << strerror(-ret);
> +			return {};
> +		}
> +
> +		return { minCrop, maxCrop };
> +	}
> +
> +	/*
> +	 * If the streams are configured, return bounds from according to

"from according to" ?

> +	 * stream configuration.
> +	 */

The behaviour is ill-defined. The documentation of the inputCropBounds()
function makes no mention of the fact that the crop bounds will vary
depending on the stream configuration.

> +	ret = setInputCrop(stream, &minCrop);

Modifying the active crop rectangle when querying the bounds isn't a
good idea. It's an unexpected side effect when the converter isn't
streaming yet, and will mess things up when it is.

> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query minimum crop bound"
> +				      << strerror(-ret);
> +		return {};
> +	}
> +
> +	ret = setInputCrop(stream, &maxCrop);
> +	if (ret) {
> +		LOG(Converter, Error) << "Failed to query maximum crop bound"
> +				      << strerror(-ret);
> +		return {};
> +	}
> +
> +	return { minCrop, maxCrop };
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::start
>   */
> @@ -448,6 +536,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.
> + */

Does this belong to patch 1/5 ?

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