| Message ID | 20220902141136.1342352-1-m.felsch@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Marco, Thank you for the patch. 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. > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > src/qcam/viewfinder_gl.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index e2aa2470..501c5bae 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > double offset; > > switch (colorSpace.range) { > + default: /* Avoid gcc11 maybe-uninitialized warning */ > case libcamera::ColorSpace::Range::Full: > offset = 0.0; > break;
Hi Laurent, On 22-09-02, Laurent Pinchart wrote: > Hi Marco, > > Thank you for the patch. > > 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. Regards, Marco
Hi Marco, 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). This being said, as the problem affects real use cases, we'll fix it.
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.
Hi Marco, Going back to the patch now. 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. I'd like to mention the result of the investigation I detailed in a separate reply in this mail thread. Would you be fine with the following commit message ? Commit 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") introduced a maybe-uninitialized warning with gcc 11 and 12. The compiler is correct, 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]). Fix the warning by making the full range the default. https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > src/qcam/viewfinder_gl.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index e2aa2470..501c5bae 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > double offset; > > switch (colorSpace.range) { > + default: /* Avoid gcc11 maybe-uninitialized warning */ Given that this is a valid behaviour for the compiler, and not a false positive as I initially thought, I think we can drop the comment. If you don't mind I'll also move the default one line down, just right after the Full case. There's no need to submit a v2 if you're fine with these changes, I'll apply them locally. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > case libcamera::ColorSpace::Range::Full: > offset = 0.0; > break;
On Sat, Sep 03, 2022 at 02:31:39AM +0300, Laurent Pinchart wrote: > Hi Marco, > > Going back to the patch now. > > 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. > > I'd like to mention the result of the investigation I detailed in a > separate reply in this mail thread. Would you be fine with the following > commit message ? > > Commit 251f0534 ("qcam: viewfinder_gl: Take color space into account for > YUV rendering") introduced a maybe-uninitialized warning with gcc 11 and > 12. The compiler is correct, 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]). > > Fix the warning by making the full range the default. > > https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") I also forgot to mention, Fixes tags should use 12 digits for the commit ID. I can also address this locally. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > src/qcam/viewfinder_gl.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > > index e2aa2470..501c5bae 100644 > > --- a/src/qcam/viewfinder_gl.cpp > > +++ b/src/qcam/viewfinder_gl.cpp > > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > double offset; > > > > switch (colorSpace.range) { > > + default: /* Avoid gcc11 maybe-uninitialized warning */ > > Given that this is a valid behaviour for the compiler, and not a false > positive as I initially thought, I think we can drop the comment. If you > don't mind I'll also move the default one line down, just right after > the Full case. > > There's no need to submit a v2 if you're fine with these changes, I'll > apply them locally. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > case libcamera::ColorSpace::Range::Full: > > offset = 0.0; > > break;
On Sat, Sep 03, 2022 at 02:32:40AM +0300, Laurent Pinchart via libcamera-devel wrote: > On Sat, Sep 03, 2022 at 02:31:39AM +0300, Laurent Pinchart wrote: > > Hi Marco, > > > > Going back to the patch now. > > > > 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. > > > > I'd like to mention the result of the investigation I detailed in a > > separate reply in this mail thread. Would you be fine with the following > > commit message ? > > > > Commit 251f0534 ("qcam: viewfinder_gl: Take color space into account for > > YUV rendering") introduced a maybe-uninitialized warning with gcc 11 and > > 12. The compiler is correct, 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]). > > > > Fix the warning by making the full range the default. > > > > https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > > > > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > > I also forgot to mention, Fixes tags should use 12 digits for the commit > ID. I can also address this locally. Also, gcc 12.2.0 still warns about the second switch statement in the same function. I'll submit a v2 of your patch to fix that as well. > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > src/qcam/viewfinder_gl.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > > > index e2aa2470..501c5bae 100644 > > > --- a/src/qcam/viewfinder_gl.cpp > > > +++ b/src/qcam/viewfinder_gl.cpp > > > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > > double offset; > > > > > > switch (colorSpace.range) { > > > + default: /* Avoid gcc11 maybe-uninitialized warning */ > > > > Given that this is a valid behaviour for the compiler, and not a false > > positive as I initially thought, I think we can drop the comment. If you > > don't mind I'll also move the default one line down, just right after > > the Full case. > > > > There's no need to submit a v2 if you're fine with these changes, I'll > > apply them locally. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > case libcamera::ColorSpace::Range::Full: > > > offset = 0.0; > > > break;
Hi Laurent, On 22-09-03, Laurent Pinchart wrote: > On Sat, Sep 03, 2022 at 02:32:40AM +0300, Laurent Pinchart via libcamera-devel wrote: > > On Sat, Sep 03, 2022 at 02:31:39AM +0300, Laurent Pinchart wrote: > > > Hi Marco, > > > > > > Going back to the patch now. > > > > > > 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. > > > > > > I'd like to mention the result of the investigation I detailed in a > > > separate reply in this mail thread. Would you be fine with the following > > > commit message ? > > > > > > Commit 251f0534 ("qcam: viewfinder_gl: Take color space into account for > > > YUV rendering") introduced a maybe-uninitialized warning with gcc 11 and > > > 12. The compiler is correct, 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]). > > > > > > Fix the warning by making the full range the default. > > > > > > https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17 > > > > > > > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") > > > > I also forgot to mention, Fixes tags should use 12 digits for the commit > > ID. I can also address this locally. > > Also, gcc 12.2.0 still warns about the second switch statement in the > same function. I'll submit a v2 of your patch to fix that as well. Of course, thanks for the investigation on this topic. Regards, Marco > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > src/qcam/viewfinder_gl.cpp | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > > > > index e2aa2470..501c5bae 100644 > > > > --- a/src/qcam/viewfinder_gl.cpp > > > > +++ b/src/qcam/viewfinder_gl.cpp > > > > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) > > > > double offset; > > > > > > > > switch (colorSpace.range) { > > > > + default: /* Avoid gcc11 maybe-uninitialized warning */ > > > > > > Given that this is a valid behaviour for the compiler, and not a false > > > positive as I initially thought, I think we can drop the comment. If you > > > don't mind I'll also move the default one line down, just right after > > > the Full case. > > > > > > There's no need to submit a v2 if you're fine with these changes, I'll > > > apply them locally. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > case libcamera::ColorSpace::Range::Full: > > > > offset = 0.0; > > > > break; > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp index e2aa2470..501c5bae 100644 --- a/src/qcam/viewfinder_gl.cpp +++ b/src/qcam/viewfinder_gl.cpp @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace) double offset; switch (colorSpace.range) { + default: /* Avoid gcc11 maybe-uninitialized warning */ case libcamera::ColorSpace::Range::Full: offset = 0.0; break;
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. Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering") Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- src/qcam/viewfinder_gl.cpp | 1 + 1 file changed, 1 insertion(+)