[libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix incorrect bdsHeight calculation

[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 at ideasonboard.com>
> > >
> > > The fix looks correct to me.
> > > Reviewed-by: Hirokazu Honda <hiroh at 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;
> > > >                                 }
> > > >                         }

-- 
Regards,

Laurent Pinchart