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

[Umang Jain]

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 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.
>
> [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>

Reviewed-by: Umang Jain <umang.jain 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