<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 31, 2021 at 5:37 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote:<br>
> On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote:<br>
> <br>
> > When compiling with optimization, gcc 9 and newer throw an unitialized<br>
> > variable warning:<br>
> ><br>
> > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void<br>
> > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*,<br>
> > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:<br>
> > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may<br>
> > be used uninitialized in this function [-Werror=maybe-uninitialized]<br>
> >   172 |    unsigned int bdsIntHeight = static_cast<unsigned<br>
> > int>(bdsHeight);<br>
> ><br>
> > Neither clang not gcc versions older than 9 complain. This seems to be<br>
> > a false positive.<br>
> ><br>
> > However, there's an obvious error in the code. The second while () loop<br>
> > in the first part of calculateBDSHeight() modifies the bdsHeight<br>
> > variable set by the first loop even if the second loop doesn't find a<br>
> > suitable height. This can result in an incorrect bdsHeight value. Fix<br>
> > this, which also gets rid of the compiler warning.<br>
> ><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> <br>
> The fix looks correct to me.<br>
> Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
<br>
As mentioned in the cover letter, which of 1/2 and 2/2 do you think we<br>
should merge ?<br></blockquote><div><br></div><div>I prefer 2/2.</div><div><br></div><div>-Hiro </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > ---<br>
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------<br>
> >  1 file changed, 8 insertions(+), 6 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > b/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > index 3e517ac67962..4eb3f7b730a9 100644<br>
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp<br>
> > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc<br>
> >                 while (ifHeight >= minIFHeight && ifHeight <= iif.height &&<br>
> >                        ifHeight / bdsSF >= minBDSHeight) {<br>
> ><br>
> > -                       bdsHeight = ifHeight / bdsSF;<br>
> > -                       if (std::fmod(bdsHeight, 1.0) == 0) {<br>
> > -                               unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);<br>
> > +                       float height = ifHeight / bdsSF;<br>
> > +                       if (std::fmod(height, 1.0) == 0) {<br>
> > +                               unsigned int bdsIntHeight = static_cast<unsigned int>(height);<br>
> ><br>
> >                                 if (!(bdsIntHeight % BDS_ALIGN_H)) {<br>
> >                                         foundIfHeight = ifHeight;<br>
> > +                                       bdsHeight = height;<br>
> >                                         break;<br>
> >                                 }<br>
> >                         }<br>
> > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc<br>
> >                 while (ifHeight >= minIFHeight && ifHeight <= iif.height &&<br>
> >                        ifHeight / bdsSF >= minBDSHeight) {<br>
> ><br>
> > -                       bdsHeight = ifHeight / bdsSF;<br>
> > -                       if (std::fmod(bdsHeight, 1.0) == 0) {<br>
> > -                               unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);<br>
> > +                       float height = ifHeight / bdsSF;<br>
> > +                       if (std::fmod(height, 1.0) == 0) {<br>
> > +                               unsigned int bdsIntHeight = static_cast<unsigned int>(height);<br>
> ><br>
> >                                 if (!(bdsIntHeight % BDS_ALIGN_H)) {<br>
> >                                         foundIfHeight = ifHeight;<br>
> > +                                       bdsHeight = height;<br>
> >                                         break;<br>
> >                                 }<br>
> >                         }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>