Bug 1080578 - Qt5 creates invalid shaders when glProgramBinary fails
Qt5 creates invalid shaders when glProgramBinary fails
Status: RESOLVED FIXED
: 1080454 1080488 (view as bug list)
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Other
Current
Other Other
: P1 - Urgent : Major (vote)
: ---
Assigned To: Michal Srb
E-mail List
https://bugreports.qt.io/browse/QTBUG...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-12 12:24 UTC by Michal Srb
Modified: 2020-12-03 13:56 UTC (History)
5 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments
Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error (1005 bytes, patch)
2018-02-12 13:59 UTC, Max Staudt
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Srb 2018-02-12 12:24:36 UTC
The `QOpenGLShaderProgram` class in Qt can be used to compile and link OpenGL shaders. If the `GL_ARB_get_program_binary` OpenGL extension is available, it can cache those shaders on disk (~/.cache/qtshadercache). The i965 driver in Mesa supports this extension since version 18.0.0.

When the shader is loaded using the `glProgramBinary` function, OpenGL can refuse it if for example some hardware or software component changed. Mesa refuses binaries that were created by any other build of Mesa (using among other things the build_id of the library).

If the shader is refused, Qt should fallback to compiling it from sources, but it incorrectly calls glLinkProgram first. The glLinkProgram succeeds, because it actually links 0 shaders together. That is allowed in OpenGL compatibility profile and the resulting program works as a fixed pipeline. Which of course does not render as expected.

This causes rendering errors in Qt applications every time Mesa is updated since version 18.0.0. For example white screen in sddm.
Comment 1 Michal Srb 2018-02-12 12:43:58 UTC
This bug appeared first after we updated Mesa with the fix for bug 1079465. It was believed that this bug is the same as bug 1079465 because the recommended (but not really necessary!) cleaning of ~/.cache in that bug also temporarily fixes this bug. This bug would however come back during the next update of Mesa.
Comment 2 Max Staudt 2018-02-12 13:16:57 UTC
(In reply to Michal Srb from comment #0)
> When the shader is loaded using the `glProgramBinary` function, OpenGL can
> refuse it if for example some hardware or software component changed. Mesa
> refuses binaries that were created by any other build of Mesa (using among
> other things the build_id of the library).
> 
> If the shader is refused, Qt should fallback to compiling it from sources,
> but it incorrectly calls glLinkProgram first.

This happens because Qt doesn't check the LinkStatus that Mesa sets on the program:


      QOpenGLShaderProgramPrivate::linkBinary()
calls QOpenGLProgramBinaryCache::load()
calls QOpenGLProgramBinaryCache::setProgramBinary()
calls _mesa_program_binary()


Now, Qt's setProgramBinary() checks glGetError(), but doesn't look at the LinkStatus. Thus, it returns true, thus Qt's load() returns true, thus linkBinary() thinks that the program from cache was loaded alright, and Qt goes on to link it.
Comment 3 Max Staudt 2018-02-12 13:25:22 UTC
So the fix seems to be checking GL_LINK_STATUS by calling glGetProgram() in QOpenGLProgramBinaryCache::setProgramBinary() :

https://www.khronos.org/opengl/wiki/GlProgramBinary
https://www.khronos.org/opengl/wiki/GLAPI/glGetProgram

Michal, what do you think?
Comment 4 Michal Srb 2018-02-12 13:26:32 UTC
(In reply to Max Staudt from comment #3)
> So the fix seems to be checking GL_LINK_STATUS by calling glGetProgram() in
> QOpenGLProgramBinaryCache::setProgramBinary() :

Actually it already checks it. But if it is false, it continues doing strange things.

The QOpenGLShaderProgramPrivate::linkBinary calls QOpenGLShaderProgram::link which checks the GL_LINK_STATUS. And if the GL_LINK_STATUS would be true, it would immediately return true. But if it is false, it (for unknown reasons) continues to call `glLinkProgram`, which at that point makes no sense. The `glLinkProgram` succeeds, because it links a 0 shaders together, which is valid thing to do. But the resulting shader does not render what it is supposed to render.
Comment 5 Max Staudt 2018-02-12 13:59:11 UTC
Created attachment 759816 [details]
Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error

(In reply to Michal Srb from comment #4)
> (In reply to Max Staudt from comment #3)
> > So the fix seems to be checking GL_LINK_STATUS by calling glGetProgram() in
> > QOpenGLProgramBinaryCache::setProgramBinary() :
> 
> Actually it already checks it. But if it is false, it continues doing
> strange things.

Seems like QOpenGLProgramBinaryCache::load() is the only user of QOpenGLProgramBinaryCache::setProgramBinary() - so here's a patch to make the latter return false when loading fails.
Comment 6 Fabian Vogt 2018-02-12 14:14:36 UTC
Comment on attachment 759816 [details]
Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error

The patch is missing a header and seems wrong to me now as it uses both glGetError and GL_LINK_STATUS. I'd swap the order of the checks which seems cleaner to me.

glGetProgram(GL_LINK_STATUS)...
if (value != GL_TRUE || err != GL_NO_ERROR) { complain }
Comment 7 Max Staudt 2018-02-12 14:26:09 UTC
(In reply to Fabian Vogt from comment #6)
> Comment on attachment 759816 [details]
> Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error
> 
> The patch is missing a header and seems wrong to me now as it uses both
> glGetError and GL_LINK_STATUS. I'd swap the order of the checks which seems
> cleaner to me.
> 
> glGetProgram(GL_LINK_STATUS)...
> if (value != GL_TRUE || err != GL_NO_ERROR) { complain }

True, will fix. Thanks!
Comment 8 Max Staudt 2018-02-12 14:36:43 UTC
Comment on attachment 759816 [details]
Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error

The patch is obsolete now, we're testing a better version in OBS.
Comment 9 Michal Srb 2018-02-12 15:12:40 UTC
*** Bug 1080488 has been marked as a duplicate of this bug. ***
Comment 10 Michal Srb 2018-02-13 08:17:39 UTC
I have reviewed and tested the opengl-Bail-if-cached-shader-fails-to-load.patch patch currently in home:mstaudt:1080578boo-broken-shader-cache/libqt5-qtbase. It looks good and fixes the problem on my test machine. Max, can you please submit it?.
Comment 11 Fabian Vogt 2018-02-13 08:22:55 UTC
(In reply to Michal Srb from comment #10)
> I have reviewed and tested the
> opengl-Bail-if-cached-shader-fails-to-load.patch patch currently in
> home:mstaudt:1080578boo-broken-shader-cache/libqt5-qtbase. It looks good and
> fixes the problem on my test machine. Max, can you please submit it?.

It's already in Tumbleweed ;-)

https://build.opensuse.org/request/show/575866
Comment 13 Rick Stockton 2018-03-03 17:29:55 UTC
*** Bug 1080454 has been marked as a duplicate of this bug. ***