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

[paul.elder at ideasonboard.com]

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

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
>