| Message ID | 20210503092705.15562-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2021-05-03 11:27:01 +0200, 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. > > 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> > 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 36fee31c1962..ea654e57b0e7 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]) { nit: This could be simplified by turning 'found' into an unsigned int initialized to 0 and storing ifHeight inside found instead of true. But I think this maybe a matter of taste so with or without this, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight }, > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thank you for the patch, On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund < niklas.soderlund@ragnatech.se> wrote: > Hi Jacopo, > > Thanks for your work. > > On 2021-05-03 11:27:01 +0200, 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. > > > > 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> > > 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 36fee31c1962..ea654e57b0e7 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]) { > > nit: This could be simplified by turning 'found' into an unsigned int > initialized to 0 and storing ifHeight inside found instead of true. But > I think this maybe a matter of taste so with or without this, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Is it necessary to do the second-loop when a height is found in the first loop? -Hiro > > unsigned int bdsIntHeight = static_cast<unsigned > int>(bdsHeight); > > > > pipeConfigs.push_back({ bdsSF, { iif.width, > ifHeight }, > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hello, time to time I dig this series out of the dead patches cemetery. I'm kind of the Lara Croft of libcamera On Thu, May 06, 2021 at 02:51:03PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch, > > On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund < > niklas.soderlund@ragnatech.se> wrote: > > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2021-05-03 11:27:01 +0200, 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. > > > > > > 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> > > > 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 36fee31c1962..ea654e57b0e7 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]) { > > > > nit: This could be simplified by turning 'found' into an unsigned int > > initialized to 0 and storing ifHeight inside found instead of true. But > > I think this maybe a matter of taste so with or without this, > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > Is it necessary to do the second-loop when a height is found in the first > loop? Yes, as if (foundIfHeights[0]) ifHeight = foundIfHeights[0]; if (foundIfHeights[1]) ifHeight = foundIfHeights[1]; overwrites the first found result with the second found one. Actually as Niklas and Laurent pointed out, this can be handled with a single variable that gets overwritten by the second loop, if found. Let's deviate from the script, I'll take this change in. Thanks j > > -Hiro > > > > unsigned int bdsIntHeight = static_cast<unsigned > > int>(bdsHeight); > > > > > > pipeConfigs.push_back({ bdsSF, { iif.width, > > ifHeight }, > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Tue, May 11, 2021 at 11:36 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hello, > time to time I dig this series out of the dead patches cemetery. > I'm kind of the Lara Croft of libcamera > > On Thu, May 06, 2021 at 02:51:03PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for the patch, > > > > On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund < > > niklas.soderlund@ragnatech.se> wrote: > > > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > On 2021-05-03 11:27:01 +0200, 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. > > > > > > > > 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> > > > > 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 36fee31c1962..ea654e57b0e7 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]) { > > > > > > nit: This could be simplified by turning 'found' into an unsigned int > > > initialized to 0 and storing ifHeight inside found instead of true. But > > > I think this maybe a matter of taste so with or without this, > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > Is it necessary to do the second-loop when a height is found in the first > > loop? > > Yes, as > > if (foundIfHeights[0]) > ifHeight = foundIfHeights[0]; > if (foundIfHeights[1]) > ifHeight = foundIfHeights[1]; > > overwrites the first found result with the second found one. > Actually as Niklas and Laurent pointed out, this can be handled with a > single variable that gets overwritten by the second loop, if found. > > Let's deviate from the script, I'll take this change in. > > Ack. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > Thanks > j > > > > > -Hiro > > > > > > unsigned int bdsIntHeight = > static_cast<unsigned > > > int>(bdsHeight); > > > > > > > > pipeConfigs.push_back({ bdsSF, { iif.width, > > > ifHeight }, > > > > -- > > > > 2.31.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index 36fee31c1962..ea654e57b0e7 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 },