| Message ID | 20210531020745.13815-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thank you for the patch. On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > When compiling with optimization, gcc 9 and newer throw an unitialized > variable warning: > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may > be used uninitialized in this function [-Werror=maybe-uninitialized] > 172 | unsigned int bdsIntHeight = static_cast<unsigned > int>(bdsHeight); > > Neither clang not gcc versions older than 9 complain. This seems to be > a false positive. > > However, there's an obvious error in the code. The second while () loop > in the first part of calculateBDSHeight() modifies the bdsHeight > variable set by the first loop even if the second loop doesn't find a > suitable height. This can result in an incorrect bdsHeight value. Fix > this, which also gets rid of the compiler warning. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > The fix looks correct to me. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp > b/src/libcamera/pipeline/ipu3/imgu.cpp > index 3e517ac67962..4eb3f7b730a9 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, > const Size &iif, const Size &gdc > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > ifHeight / bdsSF >= minBDSHeight) { > > - bdsHeight = ifHeight / bdsSF; > - if (std::fmod(bdsHeight, 1.0) == 0) { > - unsigned int bdsIntHeight = > static_cast<unsigned int>(bdsHeight); > + float height = ifHeight / bdsSF; > + if (std::fmod(height, 1.0) == 0) { > + unsigned int bdsIntHeight = > static_cast<unsigned int>(height); > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > foundIfHeight = ifHeight; > + bdsHeight = height; > break; > } > } > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, > const Size &iif, const Size &gdc > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > ifHeight / bdsSF >= minBDSHeight) { > > - bdsHeight = ifHeight / bdsSF; > - if (std::fmod(bdsHeight, 1.0) == 0) { > - unsigned int bdsIntHeight = > static_cast<unsigned int>(bdsHeight); > + float height = ifHeight / bdsSF; > + if (std::fmod(height, 1.0) == 0) { > + unsigned int bdsIntHeight = > static_cast<unsigned int>(height); > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > foundIfHeight = ifHeight; > + bdsHeight = height; > break; > } > } > -- > Regards, > > Laurent Pinchart > >
Hi Hiro, On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote: > On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote: > > > When compiling with optimization, gcc 9 and newer throw an unitialized > > variable warning: > > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void > > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, > > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: > > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may > > be used uninitialized in this function [-Werror=maybe-uninitialized] > > 172 | unsigned int bdsIntHeight = static_cast<unsigned > > int>(bdsHeight); > > > > Neither clang not gcc versions older than 9 complain. This seems to be > > a false positive. > > > > However, there's an obvious error in the code. The second while () loop > > in the first part of calculateBDSHeight() modifies the bdsHeight > > variable set by the first loop even if the second loop doesn't find a > > suitable height. This can result in an incorrect bdsHeight value. Fix > > this, which also gets rid of the compiler warning. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The fix looks correct to me. > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> As mentioned in the cover letter, which of 1/2 and 2/2 do you think we should merge ? > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp > > b/src/libcamera/pipeline/ipu3/imgu.cpp > > index 3e517ac67962..4eb3f7b730a9 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > ifHeight / bdsSF >= minBDSHeight) { > > > > - bdsHeight = ifHeight / bdsSF; > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > + float height = ifHeight / bdsSF; > > + if (std::fmod(height, 1.0) == 0) { > > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > foundIfHeight = ifHeight; > > + bdsHeight = height; > > break; > > } > > } > > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > ifHeight / bdsSF >= minBDSHeight) { > > > > - bdsHeight = ifHeight / bdsSF; > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > + float height = ifHeight / bdsSF; > > + if (std::fmod(height, 1.0) == 0) { > > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > foundIfHeight = ifHeight; > > + bdsHeight = height; > > break; > > } > > }
Hi Laurent, On Mon, May 31, 2021 at 5:37 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote: > > On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote: > > > > > When compiling with optimization, gcc 9 and newer throw an unitialized > > > variable warning: > > > > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void > > > > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, > > > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ > may > > > be used uninitialized in this function [-Werror=maybe-uninitialized] > > > 172 | unsigned int bdsIntHeight = static_cast<unsigned > > > int>(bdsHeight); > > > > > > Neither clang not gcc versions older than 9 complain. This seems to be > > > a false positive. > > > > > > However, there's an obvious error in the code. The second while () loop > > > in the first part of calculateBDSHeight() modifies the bdsHeight > > > variable set by the first loop even if the second loop doesn't find a > > > suitable height. This can result in an incorrect bdsHeight value. Fix > > > this, which also gets rid of the compiler warning. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The fix looks correct to me. > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > As mentioned in the cover letter, which of 1/2 and 2/2 do you think we > should merge ? > I prefer 2/2. -Hiro > > > > --- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp > > > b/src/libcamera/pipeline/ipu3/imgu.cpp > > > index 3e517ac67962..4eb3f7b730a9 100644 > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, > const Size &iif, const Size &gdc > > > while (ifHeight >= minIFHeight && ifHeight <= > iif.height && > > > ifHeight / bdsSF >= minBDSHeight) { > > > > > > - bdsHeight = ifHeight / bdsSF; > > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > > - unsigned int bdsIntHeight = > static_cast<unsigned int>(bdsHeight); > > > + float height = ifHeight / bdsSF; > > > + if (std::fmod(height, 1.0) == 0) { > > > + unsigned int bdsIntHeight = > static_cast<unsigned int>(height); > > > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > > foundIfHeight = ifHeight; > > > + bdsHeight = height; > > > break; > > > } > > > } > > > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, > const Size &iif, const Size &gdc > > > while (ifHeight >= minIFHeight && ifHeight <= > iif.height && > > > ifHeight / bdsSF >= minBDSHeight) { > > > > > > - bdsHeight = ifHeight / bdsSF; > > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > > - unsigned int bdsIntHeight = > static_cast<unsigned int>(bdsHeight); > > > + float height = ifHeight / bdsSF; > > > + if (std::fmod(height, 1.0) == 0) { > > > + unsigned int bdsIntHeight = > static_cast<unsigned int>(height); > > > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > > foundIfHeight = ifHeight; > > > + bdsHeight = height; > > > break; > > > } > > > } > > -- > Regards, > > Laurent Pinchart >
Hi Hiro, On Mon, May 31, 2021 at 05:39:07PM +0900, Hirokazu Honda wrote: > On Mon, May 31, 2021 at 5:37 PM Laurent Pinchart wrote: > > On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote: > > > On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote: > > > > > > > When compiling with optimization, gcc 9 and newer throw an unitialized > > > > variable warning: > > > > > > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void > > > > > > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, > > > > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: > > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ > > may > > > > be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > 172 | unsigned int bdsIntHeight = static_cast<unsigned > > > > int>(bdsHeight); > > > > > > > > Neither clang not gcc versions older than 9 complain. This seems to be > > > > a false positive. > > > > > > > > However, there's an obvious error in the code. The second while () loop > > > > in the first part of calculateBDSHeight() modifies the bdsHeight > > > > variable set by the first loop even if the second loop doesn't find a > > > > suitable height. This can result in an incorrect bdsHeight value. Fix > > > > this, which also gets rid of the compiler warning. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The fix looks correct to me. > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > As mentioned in the cover letter, which of 1/2 and 2/2 do you think we > > should merge ? > > I prefer 2/2. OK, I'll run a full CTS on that to check for regressions. > > > > --- > > > > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp > > > > b/src/libcamera/pipeline/ipu3/imgu.cpp > > > > index 3e517ac67962..4eb3f7b730a9 100644 > > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > > > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > > > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > > > ifHeight / bdsSF >= minBDSHeight) { > > > > > > > > - bdsHeight = ifHeight / bdsSF; > > > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > > > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > > > + float height = ifHeight / bdsSF; > > > > + if (std::fmod(height, 1.0) == 0) { > > > > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > > > > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > > > foundIfHeight = ifHeight; > > > > + bdsHeight = height; > > > > break; > > > > } > > > > } > > > > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > > > > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > > > > ifHeight / bdsSF >= minBDSHeight) { > > > > > > > > - bdsHeight = ifHeight / bdsSF; > > > > - if (std::fmod(bdsHeight, 1.0) == 0) { > > > > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > > > + float height = ifHeight / bdsSF; > > > > + if (std::fmod(height, 1.0) == 0) { > > > > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > > > > > > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > > > > foundIfHeight = ifHeight; > > > > + bdsHeight = height; > > > > break; > > > > } > > > > }
Hi Laurent, On 31/05/2021 04:07, Laurent Pinchart wrote: > When compiling with optimization, gcc 9 and newer throw an unitialized > variable warning: > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 172 | unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > > Neither clang not gcc versions older than 9 complain. This seems to be > a false positive. > > However, there's an obvious error in the code. The second while () loop > in the first part of calculateBDSHeight() modifies the bdsHeight > variable set by the first loop even if the second loop doesn't find a > suitable height. This can result in an incorrect bdsHeight value. Fix > this, which also gets rid of the compiler warning. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index 3e517ac67962..4eb3f7b730a9 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > ifHeight / bdsSF >= minBDSHeight) { > > - bdsHeight = ifHeight / bdsSF; > - if (std::fmod(bdsHeight, 1.0) == 0) { > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + float height = ifHeight / bdsSF; > + if (std::fmod(height, 1.0) == 0) { > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > foundIfHeight = ifHeight; > + bdsHeight = height; > break; > } > } > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc > while (ifHeight >= minIFHeight && ifHeight <= iif.height && > ifHeight / bdsSF >= minBDSHeight) { > > - bdsHeight = ifHeight / bdsSF; > - if (std::fmod(bdsHeight, 1.0) == 0) { > - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); > + float height = ifHeight / bdsSF; > + if (std::fmod(height, 1.0) == 0) { > + unsigned int bdsIntHeight = static_cast<unsigned int>(height); > > if (!(bdsIntHeight % BDS_ALIGN_H)) { > foundIfHeight = ifHeight; > + bdsHeight = height; > break; > } > } >
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index 3e517ac67962..4eb3f7b730a9 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc while (ifHeight >= minIFHeight && ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { - bdsHeight = ifHeight / bdsSF; - if (std::fmod(bdsHeight, 1.0) == 0) { - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); + float height = ifHeight / bdsSF; + if (std::fmod(height, 1.0) == 0) { + unsigned int bdsIntHeight = static_cast<unsigned int>(height); if (!(bdsIntHeight % BDS_ALIGN_H)) { foundIfHeight = ifHeight; + bdsHeight = height; break; } } @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc while (ifHeight >= minIFHeight && ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) { - bdsHeight = ifHeight / bdsSF; - if (std::fmod(bdsHeight, 1.0) == 0) { - unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); + float height = ifHeight / bdsSF; + if (std::fmod(height, 1.0) == 0) { + unsigned int bdsIntHeight = static_cast<unsigned int>(height); if (!(bdsIntHeight % BDS_ALIGN_H)) { foundIfHeight = ifHeight; + bdsHeight = height; break; } }
When compiling with optimization, gcc 9 and newer throw an unitialized variable warning: ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, const libcamera::Size&, const libcamera::Size&, unsigned int, float)’: ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 172 | unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight); Neither clang not gcc versions older than 9 complain. This seems to be a false positive. However, there's an obvious error in the code. The second while () loop in the first part of calculateBDSHeight() modifies the bdsHeight variable set by the first loop even if the second loop doesn't find a suitable height. This can result in an incorrect bdsHeight value. Fix this, which also gets rid of the compiler warning. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)