[PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop bounds support

[Stefan Klug]

On Mon, Nov 25, 2024 at 07:36:50PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 25, 2024 at 04:14:15PM +0100, Stefan Klug wrote:
> > When a converter (dw100 on imx8mp in this case) is used in a pipeline,
> > the ScalerCrop control get's created at createCamera() time. As this is
> 
> Kind of unrelated to this patch, right ? :)

A bit maybe. Without it, I would ask myself: Why would anyone ask for
cropBounds before configuring the coverter?

How would you phrase that?

> 
> > before configure(), no streams were configured on the converter and the
> > stream specific crop bounds are not known. Extend the converter class to
> > report the default crop bounds in case the provided stream is not found.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> > Changes in v2:
> > - Added this patch
> > ---
> >  .../internal/converter/converter_v4l2_m2m.h   |   1 +
> >  src/libcamera/converter.cpp                   |   3 +
> >  .../converter/converter_v4l2_m2m.cpp          | 113 +++++++++---------
> >  3 files changed, 59 insertions(+), 58 deletions(-)
> >
> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > index 0bc0d053e2c4..a5286166f574 100644
> > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > @@ -105,6 +105,7 @@ private:
> >
> >  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> >  	std::map<FrameBuffer *, unsigned int> queue_;
> > +	std::pair<Rectangle, Rectangle> inputCropBounds_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index 945f2527b96a..d4b5e5260e02 100644
> > --- a/src/libcamera/converter.cpp
> > +++ b/src/libcamera/converter.cpp
> > @@ -195,6 +195,9 @@ Converter::~Converter()
> >   * this function should be called after the \a stream has been configured using
> >   * configure().
> >   *
> > + * When called with an invalid \a stream, the function returns the default crop
> > + * bounds of the converter.
> > + *
> >   * \return A pair containing the minimum and maximum crop bound in that order
> >   */
> >
> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index d63ef2f8028f..bd7e5cce600d 100644
> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > @@ -30,6 +30,52 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(Converter)
> >
> > +namespace {
> > +
> > +int getCropBounds(V4L2VideoDevice *device, Rectangle &minCrop,
> > +		  Rectangle &maxCrop)
> > +{
> > +	Rectangle minC;
> > +	Rectangle maxC;
> > +
> > +	/* Find crop bounds */
> > +	minC.width = 1;
> > +	minC.height = 1;
> > +	maxC.width = UINT_MAX;
> > +	maxC.height = UINT_MAX;
> > +
> > +	int ret = device->setSelection(V4L2_SEL_TGT_CROP, &minC);
> > +	if (ret) {
> > +		LOG(Converter, Error)
> > +			<< "Could not query minimum selection crop: "
> > +			<< strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = device->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxC);
> > +	if (ret) {
> > +		LOG(Converter, Error)
> > +			<< "Could not query maximum selection crop: "
> > +			<< strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Reset the crop to its maximum */
> > +	ret = device->setSelection(V4L2_SEL_TGT_CROP, &maxC);
> > +	if (ret) {
> > +		LOG(Converter, Error)
> > +			<< "Could not reset selection crop: "
> > +			<< strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	minCrop = minC;
> > +	maxCrop = maxC;
> > +	return 0;
> > +}
> > +
> > +} /* namespace */
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2M2MConverter::V4L2M2MStream
> >   */
> > @@ -98,41 +144,10 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC
> >  	outputBufferCount_ = outputCfg.bufferCount;
> >
> >  	if (converter_->features() & Feature::InputCrop) {
> > -		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 = getInputSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);
> > -		if (ret) {
> > -			LOG(Converter, Error)
> > -				<< "Could not query maximum selection crop: "
> > -				<< strerror(-ret);
> > -			return ret;
> > -		}
> > -
> > -		/* Reset the crop to its maximum */
> > -		ret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > -		if (ret) {
> > -			LOG(Converter, Error)
> > -				<< "Could not reset selection crop: "
> > -				<< strerror(-ret);
> 
> Maybe I don't understand M2M enough, but is there of Stream-specific
> here ?
> 
> There's an output and an input queue, right ? Do they have per-stream
> crop bounds ??

I think I don't get the question here. Yes, every context/stream has own
crop bounds. What is wrong with that? Or did we settle that in the
discussions?

Cheers,
Stefan

> 
> > +		ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> > +				    inputCropBounds_.second);
> > +		if (ret)
> >  			return ret;
> > -		}
> > -
> > -		inputCropBounds_ = { minCrop, maxCrop };
> >  	}
> >
> >  	return 0;
> > @@ -258,27 +273,9 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> >  		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)
> > -		return;
> > -
> > -	/*
> > -	 * 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 != maxCrop) {
> > +	ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> > +			    inputCropBounds_.second);
> > +	if (!ret && inputCropBounds_.first != inputCropBounds_.second) {
> >  		features_ |= Feature::InputCrop;
> >
> >  		LOG(Converter, Info)
> > @@ -477,10 +474,10 @@ std::pair<Rectangle, Rectangle>
> >  V4L2M2MConverter::inputCropBounds(const Stream *stream)
> >  {
> >  	auto iter = streams_.find(stream);
> > -	if (iter == streams_.end())
> > -		return {};
> > +	if (iter != streams_.end())
> > +		return iter->second->inputCropBounds();
> >
> > -	return iter->second->inputCropBounds();
> > +	return inputCropBounds_;
> >  }
> >
> >  /**
> > --
> > 2.43.0
> >