| Message ID | 20210318103941.18837-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for the patch, On 18/03/2021 11:39, Jacopo Mondi 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> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d9296ea3e674..11bb7cb95084 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 foundIfHeights[2] = { 0, 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; > + foundIfHeights[0] = 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; > + foundIfHeights[1] = ifHeight; > break; > } > } > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > ifHeight += IF_ALIGN_H; > } > > - if (found) { > + if (foundIfHeights[0]) > + ifHeight = foundIfHeights[0]; > + if (foundIfHeights[1]) > + ifHeight = foundIfHeights[1]; > + > + if (foundIfHeights[0] || foundIfHeights[1]) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, >
Hi Jacopo, Thank you for the patch. On Thu, Mar 18, 2021 at 11:39:37AM +0100, Jacopo Mondi 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> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index d9296ea3e674..11bb7cb95084 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 foundIfHeights[2] = { 0, 0 }; You don't need two values, a single one will do, it can be overwritten in the second loop. With an updated commit message, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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; > + foundIfHeights[0] = 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; > + foundIfHeights[1] = ifHeight; > break; > } > } > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > ifHeight += IF_ALIGN_H; > } > > - if (found) { > + if (foundIfHeights[0]) > + ifHeight = foundIfHeights[0]; > + if (foundIfHeights[1]) > + ifHeight = foundIfHeights[1]; > + > + if (foundIfHeights[0] || foundIfHeights[1]) { > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
Hi Laurent, On Tue, Apr 13, 2021 at 03:18:02AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:37AM +0100, Jacopo Mondi 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> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index d9296ea3e674..11bb7cb95084 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 foundIfHeights[2] = { 0, 0 }; > > You don't need two values, a single one will do, it can be overwritten > in the second loop. Correct, I got the same understanding but I would prefer to use the same "style" as the python script, not to diverge... What do you think ? > > With an updated commit message, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks j > > > 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; > > + foundIfHeights[0] = 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; > > + foundIfHeights[1] = ifHeight; > > break; > > } > > } > > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > ifHeight += IF_ALIGN_H; > > } > > > > - if (found) { > > + if (foundIfHeights[0]) > > + ifHeight = foundIfHeights[0]; > > + if (foundIfHeights[1]) > > + ifHeight = foundIfHeights[1]; > > + > > + if (foundIfHeights[0] || foundIfHeights[1]) { > > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > > > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Apr 21, 2021 at 02:57:56PM +0200, Jacopo Mondi wrote: > On Tue, Apr 13, 2021 at 03:18:02AM +0300, Laurent Pinchart wrote: > > On Thu, Mar 18, 2021 at 11:39:37AM +0100, Jacopo Mondi 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> > > > --- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > > index d9296ea3e674..11bb7cb95084 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 foundIfHeights[2] = { 0, 0 }; > > > > You don't need two values, a single one will do, it can be overwritten > > in the second loop. > > Correct, I got the same understanding but I would prefer to use the > same "style" as the python script, not to diverge... > > What do you think ? I don't think diverging is an issue, as I believe this code should be completely rewritten based on an actual understanding of the procedures, instead of blindly mimicking python code that has known issues. I'm OK with this series as a short term, stop-gap measure, but it's clearly not what we need longer term. I will likely insist more strongly to have this rewritten the next time code from python is integrated. > > With an updated commit message, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > 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; > > > + foundIfHeights[0] = 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; > > > + foundIfHeights[1] = ifHeight; > > > break; > > > } > > > } > > > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > > ifHeight += IF_ALIGN_H; > > > } > > > > > > - if (found) { > > > + if (foundIfHeights[0]) > > > + ifHeight = foundIfHeights[0]; > > > + if (foundIfHeights[1]) > > > + ifHeight = foundIfHeights[1]; > > > + > > > + if (foundIfHeights[0] || foundIfHeights[1]) { > > > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > > > > > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index d9296ea3e674..11bb7cb95084 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 foundIfHeights[2] = { 0, 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; + foundIfHeights[0] = 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; + foundIfHeights[1] = ifHeight; break; } } @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc ifHeight += IF_ALIGN_H; } - if (found) { + if (foundIfHeights[0]) + ifHeight = foundIfHeights[0]; + if (foundIfHeights[1]) + ifHeight = foundIfHeights[1]; + + if (foundIfHeights[0] || foundIfHeights[1]) { unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
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> --- src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)