|
Bugzilla – Full Text Bug Listing |
| Summary: | Qt5 creates invalid shaders when glProgramBinary fails | ||
|---|---|---|---|
| Product: | [openSUSE] openSUSE Tumbleweed | Reporter: | Michal Srb <msrb> |
| Component: | Other | Assignee: | Michal Srb <msrb> |
| Status: | RESOLVED FIXED | QA Contact: | E-mail List <qa-bugs> |
| Severity: | Major | ||
| Priority: | P1 - Urgent | CC: | fvogt, luismiguel427, mstaudt, rickstockton, sndirsch |
| Version: | Current | ||
| Target Milestone: | --- | ||
| Hardware: | Other | ||
| OS: | Other | ||
| URL: | https://bugreports.qt.io/browse/QTBUG-66348 | ||
| See Also: | https://bugreports.qt.io/browse/QTBUG-66348 | ||
| Whiteboard: | |||
| Found By: | --- | Services Priority: | |
| Business Priority: | Blocker: | --- | |
| Marketing QA Status: | --- | IT Deployment: | --- |
| Attachments: | Let setProgramBinary bail when GL_LINK_STATUS indicates a loading error | ||
|
Description
Michal Srb
2018-02-12 12:24:36 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. (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. 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? (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. 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 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 }
(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 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.
*** Bug 1080488 has been marked as a duplicate of this bug. *** 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?. (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 *** Bug 1080454 has been marked as a duplicate of this bug. *** |