[libcamera-devel] [PATCH] qcam: viewfinder_gl: selectColorSpace: fix maybe uninitialized warning

[Laurent Pinchart]

On Sat, Sep 03, 2022 at 12:48:43AM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, Sep 02, 2022 at 04:42:55PM +0200, Marco Felsch wrote:
> > On 22-09-02, Laurent Pinchart wrote:
> > > On Fri, Sep 02, 2022 at 04:11:36PM +0200, Marco Felsch via libcamera-devel wrote:
> > > > Fix gcc11 warning introduced in commit 251f0534 ("qcam: viewfinder_gl:
> > > > Take color space into account for YUV rendering"). Fix it by making the
> > > > full range as default.
> > > 
> > > What exact compiler version are you using ? gcc 11.3.0 compiles the code
> > > fine here. It seems to be a false positive, but if it causes breakages,
> > > we of course want to fix/work around the problem.
> > 
> > Arg.. I'm using 11.1.1 from the official OSELAS.Toolchain-2021.07.0
> > cross-toolchain. So it seems my gcc-11 is to old.
> 
> I think this is a false positive caused by a bug in gcc, which seems to
> have been fixed in later releases. gcc 10.4.0, 11.3.0 and 12.2.0 all
> compile libcamera without warning, in both debug and release modes. I
> have however been able to reproduce the issue with gcc 12.1.0 (along
> with a second warning that complains about the yuv2rgb matrix being used
> uninitialized).

Another data point: gcc 10.4.0 compiles fine with both -O0 and -O3. gcc
11.3.0 abd 12.2.0 compiles fine with -O0 but produces the warning with
-O3, which I missed when testing the offending commit.

https://gcc.gnu.org/wiki/VerboseDiagnostics#enum_switch is related. I've
read mode on the topic, and C++17 has introduced an "interesting"
feature:

An enumeration can be initialized from an integer without a cast, using
list initialization, if all of the following are true:

    the initialization is direct-list-initialization
    the initializer list has only a single element
    the enumeration is either scoped or unscoped with underlying type fixed
    the conversion is non-narrowing

This makes it possible to introduce new integer types (e.g. SafeInt)
that enjoy the same existing calling conventions as their underlying
integer types, even on ABIs that penalize passing/returning structures
by value.

[...]

enum class Handle : std::uint32_t { Invalid = 0 };
Handle h{42}; // OK as of C++17

(source: https://en.cppreference.com/w/cpp/language/enum)

gcc thus seems to be right to consider that colorSpace.range could have
other values than Full or Limited, even if it's a scoped enumeration.
I'm not sure I like what C++17 is doing there, but I can't blame the
compiler :-S

> This being said, as the problem affects real use cases, we'll fix it.

-- 
Regards,

Laurent Pinchart