| Message ID | 20220903003513.2516-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 5771125bfa4ae6c51a214d42933c393352395a89 |
| Headers | show |
| Series |
|
| Related | show |
On Sat, Sep 03, 2022 at 03:35:13AM +0300, Laurent Pinchart via libcamera-devel wrote: > From: Marco Felsch <m.felsch@pengutronix.de> > > Commit 251f0534b74b ("qcam: viewfinder_gl: Take color space into account > for YUV rendering") introduced maybe-uninitialized warnings with gcc 11 > and 12 when compiling with -O3. Both compilers warn that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:392:21: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 391 | fragmentShaderDefines_.append(QString("#define YUV2RGB_Y_OFFSET %1") > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 392 | .arg(offset, 0, 'f', 1)); > | ~~~~^~~~~~~~~~~~~~~~~~~ > > Additionally, gcc 12 warns that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:379:36: error: ‘yuv2rgb’ may be used uninitialized [-Werror=maybe-uninitialized] > 379 | yuv2rgb[i] *= 255.0 / 219.0; > ../../src/qcam/viewfinder_gl.cpp:330:31: note: ‘yuv2rgb’ declared here > 330 | std::array<double, 9> yuv2rgb; > | > > While this should never happen here, the compiler isn't necessarily > wrong, as C++17 allows initializing a scoped enum from an integer using > direct-list-initialization, even if the integer value doesn't match any > of the enumerators for the scoped enum ([1]). Whether this is valid or > borderline paranoia from gcc may be debatable, but in any case it can't > be classified as blatantly wrong. Fix the warnings by adding default > cases to the switch statements in ViewFinderGL::selectColorSpace(). > Which case is selected as the default doesn't matter, as this is not > meant to happen. If it's not meant to happen then maybe we should Fatal :D jk imo indeed the "sensible defaults" that you chose below are sufficient. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > [1] https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > Fixes: 251f0534b74b ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Rewrote commit message, added a default case for the encoding switch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/qcam/viewfinder_gl.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index e2aa24703ff0..38ddad58e09e 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -332,6 +332,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > /* OpenGL stores arrays in column-major order. */ > switch (colorSpace.ycbcrEncoding) { > case libcamera::ColorSpace::YcbcrEncoding::None: > + default: > yuv2rgb = { > 1.0000, 0.0000, 0.0000, > 0.0000, 1.0000, 0.0000, > @@ -368,6 +369,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > switch (colorSpace.range) { > case libcamera::ColorSpace::Range::Full: > + default: > offset = 0.0; > break; > > > base-commit: 251f0534b74bcb46c777aa0df34b1b4142b664f5 > -- > Regards, > > Laurent Pinchart >
Hi Paul, On Fri, Sep 02, 2022 at 09:08:25PM -0400, paul.elder@ideasonboard.com wrote: > On Sat, Sep 03, 2022 at 03:35:13AM +0300, Laurent Pinchart via libcamera-devel wrote: > > From: Marco Felsch <m.felsch@pengutronix.de> > > > > Commit 251f0534b74b ("qcam: viewfinder_gl: Take color space into account > > for YUV rendering") introduced maybe-uninitialized warnings with gcc 11 > > and 12 when compiling with -O3. Both compilers warn that > > > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > > ../../src/qcam/viewfinder_gl.cpp:392:21: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > 391 | fragmentShaderDefines_.append(QString("#define YUV2RGB_Y_OFFSET %1") > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 392 | .arg(offset, 0, 'f', 1)); > > | ~~~~^~~~~~~~~~~~~~~~~~~ > > > > Additionally, gcc 12 warns that > > > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > > ../../src/qcam/viewfinder_gl.cpp:379:36: error: ‘yuv2rgb’ may be used uninitialized [-Werror=maybe-uninitialized] > > 379 | yuv2rgb[i] *= 255.0 / 219.0; > > ../../src/qcam/viewfinder_gl.cpp:330:31: note: ‘yuv2rgb’ declared here > > 330 | std::array<double, 9> yuv2rgb; > > | > > > > While this should never happen here, the compiler isn't necessarily > > wrong, as C++17 allows initializing a scoped enum from an integer using > > direct-list-initialization, even if the integer value doesn't match any > > of the enumerators for the scoped enum ([1]). Whether this is valid or > > borderline paranoia from gcc may be debatable, but in any case it can't > > be classified as blatantly wrong. Fix the warnings by adding default > > cases to the switch statements in ViewFinderGL::selectColorSpace(). > > Which case is selected as the default doesn't matter, as this is not > > meant to happen. > > If it's not meant to happen then maybe we should Fatal :D Within libcamera I would have been tempted to do so, but in qcam, I think I don't care that much (especially given that we have no LOG() there). > jk imo indeed the "sensible defaults" that you chose below are > sufficient. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > [1] https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > > > Fixes: 251f0534b74b ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Rewrote commit message, added a default case for the encoding switch. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/qcam/viewfinder_gl.cpp | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > > index e2aa24703ff0..38ddad58e09e 100644 > > --- a/src/qcam/viewfinder_gl.cpp > > +++ b/src/qcam/viewfinder_gl.cpp > > @@ -332,6 +332,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > /* OpenGL stores arrays in column-major order. */ > > switch (colorSpace.ycbcrEncoding) { > > case libcamera::ColorSpace::YcbcrEncoding::None: > > + default: > > yuv2rgb = { > > 1.0000, 0.0000, 0.0000, > > 0.0000, 1.0000, 0.0000, > > @@ -368,6 +369,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > > > switch (colorSpace.range) { > > case libcamera::ColorSpace::Range::Full: > > + default: > > offset = 0.0; > > break; > > > > > > base-commit: 251f0534b74bcb46c777aa0df34b1b4142b664f5
Hi Laurent, Thank you for the patch. On 9/3/22 6:05 AM, Laurent Pinchart via libcamera-devel wrote: > From: Marco Felsch <m.felsch@pengutronix.de> > > Commit 251f0534b74b ("qcam: viewfinder_gl: Take color space into account > for YUV rendering") introduced maybe-uninitialized warnings with gcc 11 > and 12 when compiling with -O3. Both compilers warn that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:392:21: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 391 | fragmentShaderDefines_.append(QString("#define YUV2RGB_Y_OFFSET %1") > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 392 | .arg(offset, 0, 'f', 1)); > | ~~~~^~~~~~~~~~~~~~~~~~~ > > Additionally, gcc 12 warns that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:379:36: error: ‘yuv2rgb’ may be used uninitialized [-Werror=maybe-uninitialized] > 379 | yuv2rgb[i] *= 255.0 / 219.0; > ../../src/qcam/viewfinder_gl.cpp:330:31: note: ‘yuv2rgb’ declared here > 330 | std::array<double, 9> yuv2rgb; > | > > While this should never happen here, the compiler isn't necessarily > wrong, as C++17 allows initializing a scoped enum from an integer using > direct-list-initialization, even if the integer value doesn't match any > of the enumerators for the scoped enum ([1]). Whether this is valid or > borderline paranoia from gcc may be debatable, but in any case it can't > be classified as blatantly wrong. Fix the warnings by adding default > cases to the switch statements in ViewFinderGL::selectColorSpace(). > Which case is selected as the default doesn't matter, as this is not > meant to happen. > > [1] https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > Fixes: 251f0534b74b ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Rewrote commit message, added a default case for the encoding switch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/qcam/viewfinder_gl.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index e2aa24703ff0..38ddad58e09e 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -332,6 +332,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > /* OpenGL stores arrays in column-major order. */ > switch (colorSpace.ycbcrEncoding) { > case libcamera::ColorSpace::YcbcrEncoding::None: > + default: > yuv2rgb = { > 1.0000, 0.0000, 0.0000, > 0.0000, 1.0000, 0.0000, > @@ -368,6 +369,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > switch (colorSpace.range) { > case libcamera::ColorSpace::Range::Full: > + default: > offset = 0.0; > break; > > > base-commit: 251f0534b74bcb46c777aa0df34b1b4142b664f5
Hi Laurent, thanks for the more detailed patch description and the rework. Regards, Marco On 22-09-03, Laurent Pinchart wrote: > From: Marco Felsch <m.felsch@pengutronix.de> > > Commit 251f0534b74b ("qcam: viewfinder_gl: Take color space into account > for YUV rendering") introduced maybe-uninitialized warnings with gcc 11 > and 12 when compiling with -O3. Both compilers warn that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:392:21: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 391 | fragmentShaderDefines_.append(QString("#define YUV2RGB_Y_OFFSET %1") > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 392 | .arg(offset, 0, 'f', 1)); > | ~~~~^~~~~~~~~~~~~~~~~~~ > > Additionally, gcc 12 warns that > > ../../src/qcam/viewfinder_gl.cpp: In member function ‘void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace&)’: > ../../src/qcam/viewfinder_gl.cpp:379:36: error: ‘yuv2rgb’ may be used uninitialized [-Werror=maybe-uninitialized] > 379 | yuv2rgb[i] *= 255.0 / 219.0; > ../../src/qcam/viewfinder_gl.cpp:330:31: note: ‘yuv2rgb’ declared here > 330 | std::array<double, 9> yuv2rgb; > | > > While this should never happen here, the compiler isn't necessarily > wrong, as C++17 allows initializing a scoped enum from an integer using > direct-list-initialization, even if the integer value doesn't match any > of the enumerators for the scoped enum ([1]). Whether this is valid or > borderline paranoia from gcc may be debatable, but in any case it can't > be classified as blatantly wrong. Fix the warnings by adding default > cases to the switch statements in ViewFinderGL::selectColorSpace(). > Which case is selected as the default doesn't matter, as this is not > meant to happen. > > [1] https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > Fixes: 251f0534b74b ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Rewrote commit message, added a default case for the encoding switch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/qcam/viewfinder_gl.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index e2aa24703ff0..38ddad58e09e 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -332,6 +332,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > /* OpenGL stores arrays in column-major order. */ > switch (colorSpace.ycbcrEncoding) { > case libcamera::ColorSpace::YcbcrEncoding::None: > + default: > yuv2rgb = { > 1.0000, 0.0000, 0.0000, > 0.0000, 1.0000, 0.0000, > @@ -368,6 +369,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > switch (colorSpace.range) { > case libcamera::ColorSpace::Range::Full: > + default: > offset = 0.0; > break; > > > base-commit: 251f0534b74bcb46c777aa0df34b1b4142b664f5 > -- > Regards, > > Laurent Pinchart > >
diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp index e2aa24703ff0..38ddad58e09e 100644 --- a/src/qcam/viewfinder_gl.cpp +++ b/src/qcam/viewfinder_gl.cpp @@ -332,6 +332,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) /* OpenGL stores arrays in column-major order. */ switch (colorSpace.ycbcrEncoding) { case libcamera::ColorSpace::YcbcrEncoding::None: + default: yuv2rgb = { 1.0000, 0.0000, 0.0000, 0.0000, 1.0000, 0.0000, @@ -368,6 +369,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) switch (colorSpace.range) { case libcamera::ColorSpace::Range::Full: + default: offset = 0.0; break;