[libcamera-devel] [PATCH v2] qcam: viewfinder_gl: Fix maybe-uninitialized warnings

[Laurent Pinchart]

Hi Paul,

On Fri, Sep 02, 2022 at 09:08:25PM -0400, paul.elder at ideasonboard.com wrote:
> On Sat, Sep 03, 2022 at 03:35:13AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Marco Felsch <m.felsch at 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 at 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 at pengutronix.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > Rewrote commit message, added a default case for the encoding switch.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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