[libcamera-devel,v3,4/8] libcamera: ipu3: imgu: Fix BDS height calculation
diff mbox series

Message ID 20210513152116.17666-5-jacopo@jmondi.org
State Accepted
Commit 630c83f82a6dfcac749469be62599b72c250b063
Delegated to: Jacopo Mondi
Headers show
Series
  • ipu3: imgu: Improve ImgU calculation procedure
Related show

Commit Message

Jacopo Mondi May 13, 2021, 3:21 p.m. UTC 
The IF rectangle height is iteratively computed by first subtracting
the scaling factor to the estimated height, then in a successive loop
by adding the same scaling factor until the maximum IF size is not
reached.

As the computed IF height is not cached in any variable, the second
loop over-writes the result of the first one, even if the BDS alignment
condition is not satisfied.

Fix this by caching the result of the two iterations, and use the one
that produced any result, with a preference for the one produced by the
second loop, as implemented in the reference python script.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/imgu.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Hirokazu Honda May 17, 2021, 3:58 a.m. UTC | #1 
Hi Jacopo, thank you for the patch.

On Fri, May 14, 2021 at 12:20 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> The IF rectangle height is iteratively computed by first subtracting
> the scaling factor to the estimated height, then in a successive loop
> by adding the same scaling factor until the maximum IF size is not
> reached.
>
> As the computed IF height is not cached in any variable, the second
> loop over-writes the result of the first one, even if the BDS alignment
> condition is not satisfied.
>
> Fix this by caching the result of the two iterations, and use the one
> that produced any result, with a preference for the one produced by the
> second loop, as implemented in the reference python script.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

>

> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp
> b/src/libcamera/pipeline/ipu3/imgu.cpp
> index d878a029ad84..a325b787d916 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> const Size &iif, const Size &gdc
>         float bdsHeight;
>
>         if (!isSameRatio(pipe->input, gdc)) {
> +               unsigned int foundIfHeight = 0;
>                 float estIFHeight = (iif.width * gdc.height) /
>                                     static_cast<float>(gdc.width);
>                 estIFHeight = std::clamp<float>(estIFHeight, minIFHeight,
> iif.height);
> -               bool found = false;
>
>                 ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
>                 while (ifHeight >= minIFHeight && ifHeight / bdsSF >=
> minBDSHeight) {
> @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const
> Size &iif, const Size &gdc
>                                 unsigned int bdsIntHeight =
> static_cast<unsigned int>(bdsHeight);
>
>                                 if (!(bdsIntHeight % BDS_ALIGN_H)) {
> -                                       found = true;
> +                                       foundIfHeight = ifHeight;
>                                         break;
>                                 }
>                         }
> @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const
> Size &iif, const Size &gdc
>                                 unsigned int bdsIntHeight =
> static_cast<unsigned int>(bdsHeight);
>
>                                 if (!(bdsIntHeight % BDS_ALIGN_H)) {
> -                                       found = true;
> +                                       foundIfHeight = ifHeight;
>                                         break;
>                                 }
>                         }
> @@ -166,10 +166,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> const Size &iif, const Size &gdc
>                         ifHeight += IF_ALIGN_H;
>                 }
>
> -               if (found) {
> +               if (foundIfHeight) {
>                         unsigned int bdsIntHeight = static_cast<unsigned
> int>(bdsHeight);
>
> -                       pipeConfigs.push_back({ bdsSF, { iif.width,
> ifHeight },
> +                       pipeConfigs.push_back({ bdsSF, { iif.width,
> foundIfHeight },
>                                                 { bdsWidth, bdsIntHeight
> }, gdc });
>                         return;
>                 }
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index d878a029ad84..a325b787d916 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -129,10 +129,10 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 	float bdsHeight;
 
 	if (!isSameRatio(pipe->input, gdc)) {
+		unsigned int foundIfHeight = 0;
 		float estIFHeight = (iif.width * gdc.height) /
 				    static_cast<float>(gdc.width);
 		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
-		bool found = false;
 
 		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
 		while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
@@ -142,7 +142,7 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
 
 				if (!(bdsIntHeight % BDS_ALIGN_H)) {
-					found = true;
+					foundIfHeight = ifHeight;
 					break;
 				}
 			}
@@ -158,7 +158,7 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
 
 				if (!(bdsIntHeight % BDS_ALIGN_H)) {
-					found = true;
+					foundIfHeight = ifHeight;
 					break;
 				}
 			}
@@ -166,10 +166,10 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 			ifHeight += IF_ALIGN_H;
 		}
 
-		if (found) {
+		if (foundIfHeight) {
 			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
 
-			pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
+			pipeConfigs.push_back({ bdsSF, { iif.width, foundIfHeight },
 						{ bdsWidth, bdsIntHeight }, gdc });
 			return;
 		}