[v4,07/20] libcamera: converter_v4l2_m2m: Refactor get crop bounds code
diff mbox series

Message ID 20241216154124.203650-8-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • rkisp1: Fix aspect ratio and ScalerCrop
Related show

Commit Message

Stefan Klug Dec. 16, 2024, 3:40 p.m. UTC 
In an upcoming patch it is necessary to get the crop bounds on the
converter itself. The V4L2M2MConverter contains code that is very
similar to the get crop bounds code in the V4L2M2MStream. Merge these
code blocks into a static function to be used from both classes.  This
patch contains no functional changes.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v4:
- Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds
  support
---
 .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++---------
 1 file changed, 52 insertions(+), 54 deletions(-)

Comments

Jacopo Mondi Dec. 16, 2024, 6:03 p.m. UTC | #1 
Hi Stefan

On Mon, Dec 16, 2024 at 04:40:47PM +0100, Stefan Klug wrote:
> In an upcoming patch it is necessary to get the crop bounds on the
> converter itself. The V4L2M2MConverter contains code that is very
> similar to the get crop bounds code in the V4L2M2MStream. Merge these
> code blocks into a static function to be used from both classes.  This
> patch contains no functional changes.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>
> ---
>
> Changes in v4:
> - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds
>   support
> ---
>  .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++---------
>  1 file changed, 52 insertions(+), 54 deletions(-)
>
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index d63ef2f8028f..8c341fe199f6 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)

Not thrilled by static functions, but we discussed it already :)

> +{
> +	Rectangle minC;
> +	Rectangle maxC;

You could use minCrop and maxCrop maybe

for this patch

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


> +
> +	/* 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);
> +		ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> +				    inputCropBounds_.second);
> +		if (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);
> -			return ret;
> -		}
> -
> -		inputCropBounds_ = { minCrop, maxCrop };
>  	}
>
>  	return 0;
> @@ -258,27 +273,10 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
>  		return;
>  	}
>
> -	/* Discover Feature::InputCrop */
> +	Rectangle minCrop;
>  	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(), minCrop, maxCrop);
> +	if (!ret && minCrop != maxCrop) {
>  		features_ |= Feature::InputCrop;
>
>  		LOG(Converter, Info)
> --
> 2.43.0
>
Paul Elder Dec. 17, 2024, 3:37 a.m. UTC | #2 
On Mon, Dec 16, 2024 at 04:40:47PM +0100, Stefan Klug wrote:
> In an upcoming patch it is necessary to get the crop bounds on the
> converter itself. The V4L2M2MConverter contains code that is very
> similar to the get crop bounds code in the V4L2M2MStream. Merge these
> code blocks into a static function to be used from both classes.  This
> patch contains no functional changes.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

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

> 
> ---
> 
> Changes in v4:
> - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds
>   support
> ---
>  .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++---------
>  1 file changed, 52 insertions(+), 54 deletions(-)
> 
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index d63ef2f8028f..8c341fe199f6 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);
> +		ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> +				    inputCropBounds_.second);
> +		if (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);
> -			return ret;
> -		}
> -
> -		inputCropBounds_ = { minCrop, maxCrop };
>  	}
>  
>  	return 0;
> @@ -258,27 +273,10 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
>  		return;
>  	}
>  
> -	/* Discover Feature::InputCrop */
> +	Rectangle minCrop;
>  	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(), minCrop, maxCrop);
> +	if (!ret && minCrop != maxCrop) {
>  		features_ |= Feature::InputCrop;
>  
>  		LOG(Converter, Info)
> -- 
> 2.43.0
>
Paul Elder Dec. 17, 2024, 3:48 a.m. UTC | #3 
On Mon, Dec 16, 2024 at 07:03:44PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Dec 16, 2024 at 04:40:47PM +0100, Stefan Klug wrote:
> > In an upcoming patch it is necessary to get the crop bounds on the
> > converter itself. The V4L2M2MConverter contains code that is very
> > similar to the get crop bounds code in the V4L2M2MStream. Merge these
> > code blocks into a static function to be used from both classes.  This
> > patch contains no functional changes.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> >
> > ---
> >
> > Changes in v4:
> > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds
> >   support
> > ---
> >  .../converter/converter_v4l2_m2m.cpp          | 106 +++++++++---------
> >  1 file changed, 52 insertions(+), 54 deletions(-)
> >
> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index d63ef2f8028f..8c341fe199f6 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)
> 
> Not thrilled by static functions, but we discussed it already :)
> 
> > +{
> > +	Rectangle minC;
> > +	Rectangle maxC;
> 
> You could use minCrop and maxCrop maybe

That would shadow the output parameters which you would only want to
write to on success, I think.


Paul

> 
> for this patch
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> 
> > +
> > +	/* 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);
> > +		ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> > +				    inputCropBounds_.second);
> > +		if (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);
> > -			return ret;
> > -		}
> > -
> > -		inputCropBounds_ = { minCrop, maxCrop };
> >  	}
> >
> >  	return 0;
> > @@ -258,27 +273,10 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> >  		return;
> >  	}
> >
> > -	/* Discover Feature::InputCrop */
> > +	Rectangle minCrop;
> >  	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(), minCrop, maxCrop);
> > +	if (!ret && minCrop != maxCrop) {
> >  		features_ |= Feature::InputCrop;
> >
> >  		LOG(Converter, Info)
> > --
> > 2.43.0
> >

Patch
diff mbox series 

diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
index d63ef2f8028f..8c341fe199f6 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);
+		ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
+				    inputCropBounds_.second);
+		if (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);
-			return ret;
-		}
-
-		inputCropBounds_ = { minCrop, maxCrop };
 	}
 
 	return 0;
@@ -258,27 +273,10 @@  V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
 		return;
 	}
 
-	/* Discover Feature::InputCrop */
+	Rectangle minCrop;
 	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(), minCrop, maxCrop);
+	if (!ret && minCrop != maxCrop) {
 		features_ |= Feature::InputCrop;
 
 		LOG(Converter, Info)