[v2] libcamera: pipeline: ipu3: Fix unsigned wrap in IF height search
diff mbox series

Message ID 20261024152743.89334-1-pkarnakov@gmail.com
State Accepted
Headers show
Series
  • [v2] libcamera: pipeline: ipu3: Fix unsigned wrap in IF height search
Related show

Commit Message

Petr Karnakov Oct. 24, 2026, 3:27 p.m. UTC 
calculatePipeConfig() tries IF heights down to in.height -
kIFMaxCropHeight, and calculateBDSHeight() then crops up to another
kIFMaxCropHeight lines from each of them. For an IF height below
kIFMaxCropHeight, the minimum IF height in calculateBDSHeight() wraps
around and becomes the lower bound of std::clamp() with lo > hi. With
_GLIBCXX_ASSERTIONS this aborts, e.g. on a Surface Go 3 (ov5693, 1296x972
input) when 640x360 is requested.

Don't try IF sizes below kIFMaxCropWidth x kIFMaxCropHeight. This also
stops the search loops from wrapping, and running practically forever,
for an input exactly kIFMaxCropHeight lines high.

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/366
Suggested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Petr Karnakov <pkarnakov@gmail.com>
---
Changes in v2:
- Bound the IF search in calculatePipeConfig() instead of returning
  early from calculateBDSHeight() (Jacopo)

 src/libcamera/pipeline/ipu3/imgu.cpp | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi Oct. 26, 2026, 8:26 a.m. UTC | #1 
Hi Petr
  one suggestion for next times: Do not send a new version of a patch
series as a reply to the previous one. it confuses patchwork (and
reviewers :)

On Sat, Oct 24, 2026 at 05:27:43PM +0200, Petr Karnakov wrote:
> calculatePipeConfig() tries IF heights down to in.height -
> kIFMaxCropHeight, and calculateBDSHeight() then crops up to another
> kIFMaxCropHeight lines from each of them. For an IF height below
> kIFMaxCropHeight, the minimum IF height in calculateBDSHeight() wraps
> around and becomes the lower bound of std::clamp() with lo > hi. With
> _GLIBCXX_ASSERTIONS this aborts, e.g. on a Surface Go 3 (ov5693, 1296x972
> input) when 640x360 is requested.
>
> Don't try IF sizes below kIFMaxCropWidth x kIFMaxCropHeight. This also
> stops the search loops from wrapping, and running practically forever,
> for an input exactly kIFMaxCropHeight lines high.
>
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/366
> Suggested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Oh don't worry, that suggestion come out from pretty regular review
process :)

> Signed-off-by: Petr Karnakov <pkarnakov@gmail.com>
> ---
> Changes in v2:
> - Bound the IF search in calculatePipeConfig() instead of returning
>   early from calculateBDSHeight() (Jacopo)
>
>  src/libcamera/pipeline/ipu3/imgu.cpp | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index bb36c06e8..bf5c33da5 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -407,11 +407,18 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
>  	float bdsSF = static_cast<float>(in.width) / gdc.width;
>  	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
>
> -	/* Populate the configurations vector by scaling width and height. */
> +	/*
> +	 * Populate the configurations vector by scaling width and height.
> +	 *
> +	 * calculateBDSHeight() crops up to kIFMaxCropHeight lines from the IF
> +	 * height, so don't go below that, or its minimum IF height wraps.
> +	 */
>  	unsigned int ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
>  	unsigned int ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
> -	unsigned int minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> -	unsigned int minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
> +	unsigned int minIfWidth = std::max(in.width - ImgUDevice::kIFMaxCropWidth,
> +					   ImgUDevice::kIFMaxCropWidth);
> +	unsigned int minIfHeight = std::max(in.height - ImgUDevice::kIFMaxCropHeight,
> +					    ImgUDevice::kIFMaxCropHeight);

That's even better than my original suggestion

>  	while (ifWidth >= minIfWidth) {
>  		while (ifHeight >= minIfHeight) {
>  			Size iif{ ifWidth, ifHeight };
> @@ -425,8 +432,6 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
>  	/* Repeat search by scaling width first. */
>  	ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
>  	ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
> -	minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> -	minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;

I wonder why minIfWidth and minIfHeight were re-initialized here...
Anyway, the change looks good to me, thanks!

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



>  	while (ifHeight >= minIfHeight) {
>  		/*
>  		 * \todo This procedure is probably broken:
> --
> 2.56.0
>
Dan Scally Oct. 27, 2026, 8:11 a.m. UTC | #2 
Morning Petr, thanks for the patch

On 24/10/2026 16:27, Petr Karnakov wrote:
> calculatePipeConfig() tries IF heights down to in.height -
> kIFMaxCropHeight, and calculateBDSHeight() then crops up to another
> kIFMaxCropHeight lines from each of them. For an IF height below
> kIFMaxCropHeight, the minimum IF height in calculateBDSHeight() wraps
> around and becomes the lower bound of std::clamp() with lo > hi. With
> _GLIBCXX_ASSERTIONS this aborts, e.g. on a Surface Go 3 (ov5693, 1296x972
> input) when 640x360 is requested.
> 
> Don't try IF sizes below kIFMaxCropWidth x kIFMaxCropHeight. This also
> stops the search loops from wrapping, and running practically forever,
> for an input exactly kIFMaxCropHeight lines high.
> 
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/366
> Suggested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Petr Karnakov <pkarnakov@gmail.com>
> ---

Looks good, and I tested it on my Surface Go2:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>> Changes in v2:
> - Bound the IF search in calculatePipeConfig() instead of returning
>    early from calculateBDSHeight() (Jacopo)
> 
>   src/libcamera/pipeline/ipu3/imgu.cpp | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index bb36c06e8..bf5c33da5 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -407,11 +407,18 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
>   	float bdsSF = static_cast<float>(in.width) / gdc.width;
>   	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
>   
> -	/* Populate the configurations vector by scaling width and height. */
> +	/*
> +	 * Populate the configurations vector by scaling width and height.
> +	 *
> +	 * calculateBDSHeight() crops up to kIFMaxCropHeight lines from the IF
> +	 * height, so don't go below that, or its minimum IF height wraps.
> +	 */
>   	unsigned int ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
>   	unsigned int ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
> -	unsigned int minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> -	unsigned int minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
> +	unsigned int minIfWidth = std::max(in.width - ImgUDevice::kIFMaxCropWidth,
> +					   ImgUDevice::kIFMaxCropWidth);
> +	unsigned int minIfHeight = std::max(in.height - ImgUDevice::kIFMaxCropHeight,
> +					    ImgUDevice::kIFMaxCropHeight);
>   	while (ifWidth >= minIfWidth) {
>   		while (ifHeight >= minIfHeight) {
>   			Size iif{ ifWidth, ifHeight };
> @@ -425,8 +432,6 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
>   	/* Repeat search by scaling width first. */
>   	ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
>   	ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
> -	minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> -	minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
>   	while (ifHeight >= minIfHeight) {
>   		/*
>   		 * \todo This procedure is probably broken:

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index bb36c06e8..bf5c33da5 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -407,11 +407,18 @@  ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
 	float bdsSF = static_cast<float>(in.width) / gdc.width;
 	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
 
-	/* Populate the configurations vector by scaling width and height. */
+	/*
+	 * Populate the configurations vector by scaling width and height.
+	 *
+	 * calculateBDSHeight() crops up to kIFMaxCropHeight lines from the IF
+	 * height, so don't go below that, or its minimum IF height wraps.
+	 */
 	unsigned int ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
 	unsigned int ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
-	unsigned int minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
-	unsigned int minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
+	unsigned int minIfWidth = std::max(in.width - ImgUDevice::kIFMaxCropWidth,
+					   ImgUDevice::kIFMaxCropWidth);
+	unsigned int minIfHeight = std::max(in.height - ImgUDevice::kIFMaxCropHeight,
+					    ImgUDevice::kIFMaxCropHeight);
 	while (ifWidth >= minIfWidth) {
 		while (ifHeight >= minIfHeight) {
 			Size iif{ ifWidth, ifHeight };
@@ -425,8 +432,6 @@  ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
 	/* Repeat search by scaling width first. */
 	ifWidth = utils::alignUp(in.width, ImgUDevice::kIFAlignWidth);
 	ifHeight = utils::alignUp(in.height, ImgUDevice::kIFAlignHeight);
-	minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
-	minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
 	while (ifHeight >= minIfHeight) {
 		/*
 		 * \todo This procedure is probably broken: