Image rendered in gray since 1847c72

[bansan85]

I tried to update from corrade/magnum 2020.06 to master bc1859f.

I now have my image in gray instead of color. I made a bisect and git gave me commit 1847c72.

I used corrade mosra/corrade@68d0216

I only maintains the application. I didn't develop this part. What information do you need to diagnostic the problem ?

[bansan85]

I finally took to some time to understand what's the problem is.

At 1847c72#diff-c0ea44662863be913517b9e9a9ce2befe9c9eaadf01e4e36283ff9e97e0164d5R147, you say that supportedVersion is GLES310 / 300 / 200.

But at 1847c72#diff-c0ea44662863be913517b9e9a9ce2befe9c9eaadf01e4e36283ff9e97e0164d5R344, you exclude GLES310.

Do you think you should replace state._version < GL::Version::GLES310 by state._version <= GL::Version::GLES310 ?

[mosra]

Hey, sorry for completely neglecting this issue, and your PRs.

This particular commit is enabling a GLES 3.1+ feature where texture, input and output locations can be specified directly in the shader using layout() qualifiers instead of having to specify them externally from the C++ code. Thus, the checks should indeed be version < GLES310, i.e. calling bindAttribute() etc. only if GLES 3.1 isn't supported.

That it renders a gray image seems to be some sort of a driver bug, which I would most probably need to work around in some way. I ran through the shader tests on a bunch of targets that have GLES 3.2 and it all passes for me. You're saying that if you use <= instead, it works? Do you need to make that change in both branches or just in one?

Can you share more details about the hardware you test on? Ideally the output of the magnum-gl-info executable if you can run it there. Thank you!

[bansan85]

No problem for the delay. Plus, hardware specific problem could be difficult to fix.

The phone:

https://rog.asus.com/phones/rog-phone-6-model/

The log:

magnum-gl-info.log

Yes, replacing < by <= solves the problem.

Branch 1 and 2 enabled : color
Branch 1 disabled, 2 enabled : gray
Branch 1 enabled, 2 disabled : color
Branch 1 and 2 disabled : gray

Magnum/configure.h:

#define MAGNUM_TARGET_GL
#define MAGNUM_TARGET_GLES
/* #undef MAGNUM_TARGET_GLES2 */
#define MAGNUM_TARGET_GLES3
/* #undef MAGNUM_TARGET_WEBGL */
#define MAGNUM_TARGET_EGL
/* #undef MAGNUM_TARGET_VK */

For branch 1, if I force it :

#ifndef MAGNUM_TARGET_GLES2 : true

_flags : 0

_jointCount : false

Tell me if you need for information.

[mosra]

Thank you!

So that seems like explicit uniform location isn't working on Adreno. Or a subset of it, or the shader is triggering some unfortunate code path in the driver but the rest of the feature works. (I remember having an issue with explicit uniform location on Intel drivers on Windows, where the explicit uniform location only worked if the uniforms were numbered in an order exactly as they appeared in the code, with no gaps, and that was just impossible to workaround in a robust way so there I had to disable them completely.)

I looked around and didn't find any bugreports related to this. But since it's an ES3.1 feature, it's possible that very few projects actually opted in to use it and stayed on the classic way for more compatibility (which also is why it may be buggy). The list at https://chromium.googlesource.com/chromium/src/gpu/+/refs/heads/main/config/gpu_driver_bug_list.json contains a ton of Adreno-related bugs, but this one not, probably for a similar reason, that Chrome just doesn't make use of explicit uniform location.

I have extensive tests for the Flat shader, could you run them on the device? Assuming you have adb already set up, it should be about doing this:

• in case you're not on latest Magnum master yet, you may want to update to include commit 38b6c0f that adds graceful handling for certain driver limits that I discovered when running the tests myself yesterday
• build Magnum with MAGNUM_BUILD_TESTS and MAGNUM_BUILD_GL_TESTS enabled, and MAGNUM_WITH_TGAIMPORTER and MAGNUM_WITH_ANYIMAGEIMPORTER as well so the tests can load images
• with a device connected, run CORRADE_TEST_COLOR=ON ctest -R ShadersFlatGLTest -V -- CMake's ctest uploads the test executable together with all input and ground truth images to /data/local/tmp and runs it, the colors enabled are just to make the output look clearer in the terminal but otherwise are non-essential

I'm interested in an output both with vanilla unchanged code, and with the "Branch 1 enabled, 2 disabled" changes from above. The output will show some SKIP cases for multi-draw tests (as those work only on ANGLE and WebGL) and possibly for SSBO tests, but other than that it should run through all, printing various detailed error reports if something fails. I still half-hope this problem is related just to some corner case and not the whole feature, so hopefully even the unchanged build would show at least some of the render*() cases passing and then I could possibly figure out a smaller workaround. If not, then I'd have to add an Adreno-specific workaround to disable explicit uniform location altogether.

Thanks again!

[bansan85]

I wasn't able to run ctest. So I copy all built files and all tga files to device. I hope it's fine.

false_false.log : first branch disabled / second branch disabled (vanilla)

true_false.log : first branch forced (if(true)) / second branch disabled (if(false))

true_true.log : first branch forced (if(true)) / second branch forced (if(true))

Both branch definitely must be forced in my case.

[mosra]

Thanks, nice, so it can be convinced to render everything properly :)

The GL errors in the first case are suspicious, and might point to something. Could you re-run the first case with --magnum-gpu-validation on passed on the command line? That should print more detailed info for the GL errors happening.

In the second case it looks like it only affects UBO usage and not classic uniforms, which reduces the scope of the needed workaround quite a lot. On the other hand, SSBO binding works, even though it's the same layout(binding=N) syntax. Heh.

Sorry that you couldn't get ctest to run, forgot to mention the runner script was only ever tested on Linux. (I wanted to eventually ask you to run all tests there to uncover potential other driver bugs, but given there's about 300 test executables, without the ctest automation it'd be way too painful.)

[mosra]

Oh, and one more thing so I have the full information, can you get me the output of magnum-gl-info --limits as well? Thank you!

[bansan85]

I launched a find . -name "*Test" -exec {} \; &> all_tests.log on vanilla. There is a lots of failures :/ But I have a working software so I can live with that. But if you want I do more tests, I can. all_tests.log

The result of vanilla for ShadersFlatGLTest --magnum-gpu-validation on. vanilla_gpu_validation.log

And magnum-gl-info --limits. magnum-gl-info--limits.log

[mosra]

It doesn't seem that bad, actually :)

• Most of the test failures is due to the shaders not working, as a lot of the GL test code renders something with some shader to verify that a mesh is properly configured, texture is uploaded, etc.
• Then there's some random test failures due to files not present, but I think all those would pass if the files would be there. No need to bother with these.
• And it seems like querying texture image size isn't working at all on this driver, which makes all texture view tests fail, although the texture view functionality likely works. To fix that I'd probably need to redo the tests in a way that doesn't need size queries. I don't think that's too important right now.
• Then there's a GL::Buffer::bind() error, which is likely my fault. This call is used heavily in the shader tests, and you managed to make all those pass, so that's just a broken test case, most likely due to not respecting alignment restrictions there.
• Some other tests just abort midway through. It's mostly the shader tests again (where it's likely due to all those GL errors), what's a bit strange is the Text::Renderer test that aborts due to an assertion that shouln't be there on a GLES3 build at all. I assume that's just some stale binary, so also nothing to be concerned with.

The debug / GPU validation log unfortunately didn't enlighten me much. It seems like the driver just completely ignores the uniform(binding=N) and uniform(location=N) qualifiers in the shader code.

I got two "last hope" ideas for fixing the Flat shader -- can you try this patch with no other change? Maybe the driver just has some version checks messed up and compiling the shader as GLSL ES 3.20 instead of 3.10 would make it recognize everything properly?

diff --git a/src/Magnum/Shaders/FlatGL.cpp b/src/Magnum/Shaders/FlatGL.cpp
index fe5a5cbe9..412c34931 100644
--- a/src/Magnum/Shaders/FlatGL.cpp
+++ b/src/Magnum/Shaders/FlatGL.cpp
@@ -162,7 +162,7 @@ template<UnsignedInt dimensions> typename FlatGL<dimensions>::CompileState FlatG
     #else
     const GL::Version version = context.supportedVersion({
         #ifndef MAGNUM_TARGET_WEBGL
-        GL::Version::GLES310,
+        GL::Version::GLES320,
         #endif
         GL::Version::GLES300, GL::Version::GLES200});
     #endif

The other idea would be this -- again try this change alone, not even with the above. This makes the EXPLICIT_BINDING and EXPLICIT_UNIFORM_LOCATION defines present always, regardless of the __VERSION__ used. These guard the layout(binding=N) and layout(location=N) qualifiers. I'm thinking that maybe the driver just reports some strange __VERSION__ value which caused these to get ignored. If this change helps, I'd be interested in knowing what's the __VERSION__ actually reported.

diff --git a/src/Magnum/Shaders/compatibility.glsl b/src/Magnum/Shaders/compatibility.glsl
index 7f5b9eb24..d8bde772c 100644
--- a/src/Magnum/Shaders/compatibility.glsl
+++ b/src/Magnum/Shaders/compatibility.glsl
@@ -47,12 +47,10 @@
     #if __VERSION__ >= 300
         #define EXPLICIT_ATTRIB_LOCATION
     #endif
-    #if __VERSION__ >= 310
-        #define EXPLICIT_BINDING
-        #define EXPLICIT_UNIFORM_LOCATION
-    #endif
     /* RUNTIME_CONST is not available in OpenGL ES */
 #endif
+#define EXPLICIT_BINDING
+#define EXPLICIT_UNIFORM_LOCATION
 
 /* Precision qualifiers are not supported in GLSL 1.20 */
 #if !defined(GL_ES) && __VERSION__ == 120

If neither of the two make any difference, I'll proceed with creating the full workaround. Thanks a lot for being this responsive!

[bansan85]

Thanks for your diagnostic. It's annoying that GPU driver are so buggy. LibreOffice also maintains a (very) small blacklist GPU list for OpenGL / Skia for computer but blocks very widely.

For vanilly + GL::Version::GLES310 => GL::Version::GLES320, it doesn't solve anything. ShadersFlatGLTest --magnum-gpu-validation on randomly crash at random tests. After 10 runs, I success to have a full log. gles320.log

For vanilly + change to force EXPLICIT_BINDING / EXPLICIT_UNIFORM_LOCATION. Nice :) Everything works. force_explicit.log

__VERSION__ in compatibility.glsl equals 300.

[mosra]

Thank you!

__VERSION__ in compatibility.glsl equals 300.

Well, that's just stupid :/ Such a silly bug. So basically the driver supports everything and has no bugs whatsoever, it's just that when they added ES3.1+ support they forgot to update the __VERSION__ builtin? 😅

So the fix would be to supply my own version define and ignore __VERSION__ on this driver. I'm figuring out some nice way to do this, will get back to you later today with a patch to test.

[bansan85]

I searched a little on https://rog-forum.asus.com without success. My Rog 6 phone is up to date (33.0610.2810.104) and not rooted.

[mosra]

Alright, if I did everything correctly, the a00270c and 9b160e9 commits in the next branch should detect the Adreno driver, apply a adreno-glsl-version-stuck-at-300 workaround (which will be listed in the startup log), the ShadersFlatGLTest should now pass without errors, and so should work your own code as well.

Once you confirm, I'll merge to master.

[bansan85]

I checkout next. I have color without any modification! Thanks,

log for all tests. I added some missing data files. all_tests.log

[mosra]

Wonderful :) The above is now pushed to master.

I went through the log and everything seems fine now, the only remaining errors I see in ShadersMeshVisualizerGLTest. The GL errors happen only in some test cases, so it's likely something minor that I could fix quickly.

If you still have time, could you re-run this single test with --magnum-gpu-validation on and give me the output? It also asserts on a shader compilation error due to the test not checking limits thoroughly enough, hopefully this patch fixes all of those asserts and it's just the GL errors left:

diff --git a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp
index b579a4d35..45fd6e710 100644
--- a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp
+++ b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp
@@ -4581,8 +4581,9 @@ template<MeshVisualizerGL2D::Flag flag> void MeshVisualizerGLTest::renderObjectV
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.flags2D & MeshVisualizerGL2D::Flag::TextureTransformation ? 4 : 3))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -4796,8 +4797,9 @@ template<MeshVisualizerGL3D::Flag flag> void MeshVisualizerGLTest::renderObjectV
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.flags3D & MeshVisualizerGL3D::Flag::TextureTransformation ? 5 : 4))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -5290,8 +5292,9 @@ template<MeshVisualizerGL2D::Flag flag> void MeshVisualizerGLTest::renderSkinnin
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 4)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.jointCount ? 4 : 3))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -5451,8 +5454,9 @@ template<MeshVisualizerGL3D::Flag flag> void MeshVisualizerGLTest::renderSkinnin
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 4)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.jointCount ? 5 : 4))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -5613,8 +5617,9 @@ template<MeshVisualizerGL2D::Flag flag> void MeshVisualizerGLTest::renderInstanc
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.flags & MeshVisualizerGL2D::Flag::TextureTransformation ? 3 : 4))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -5911,7 +5916,7 @@ template<MeshVisualizerGL3D::Flag flag> void MeshVisualizerGLTest::renderInstanc
         #endif
 
         /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) <  (data.flags & MeshVisualizerGL3D::Flag::TextureTransformation ? 5 : 4))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -6239,8 +6244,9 @@ template<MeshVisualizerGL2D::Flag flag> void MeshVisualizerGLTest::renderInstanc
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 4)
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -6396,8 +6402,9 @@ template<MeshVisualizerGL3D::Flag flag> void MeshVisualizerGLTest::renderInstanc
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 5)
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     } else
     #endif
@@ -6571,8 +6578,9 @@ void MeshVisualizerGLTest::renderMulti2D() {
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.flags & MeshVisualizerGL2D::Flag::TextureTransformation ? 4 : 3))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     }
     #endif
@@ -6910,8 +6918,9 @@ void MeshVisualizerGLTest::renderMulti3D() {
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 3)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < (data.flags & MeshVisualizerGL3D::Flag::TextureTransformation ? 5 : 4))
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     }
     #endif
@@ -7253,7 +7262,8 @@ void MeshVisualizerGLTest::renderMultiSkinningWireframe2D() {
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
         if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 4)
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     }
@@ -7503,8 +7513,9 @@ void MeshVisualizerGLTest::renderMultiSkinningWireframe3D() {
             CORRADE_SKIP(GL::Version::GLES310 << "is not supported.");
         #endif
 
-        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders */
-        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 4)
+        /* Some drivers (ARM Mali-G71) don't support SSBOs in vertex shaders,
+           some others (Qualcomm Adreno 730) support just 4 blocks */
+        if(GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) < 5)
             CORRADE_SKIP("Only" << GL::Shader::maxShaderStorageBlocks(GL::Shader::Type::Vertex) << "shader storage blocks supported in vertex shaders.");
     }
     #endif

Thanks!

[bansan85]

I patched MeshVisualizerGLTest.cpp then ran all tests with and without gpu-validation. all_tests_rog6.zip

And if you are interested in, the same tests ran in a ROG5, rooted and not up to date 18.0840.2112.210. rog5.zip

[mosra]

Ahh, finally it clicked for me. The remaining errors were my fault, geometry-shader-less wireframe rendering in the MeshVisualizer has some unused uniforms (in particular, they get DCE'd by the shader compiler), and turns out the Adreno driver doesn't like that I'm attempting to set them even though they're not there anymore. Not sure if it's valid or invalid according to the GL spec, nevertheless it's something I probably shouldn't be doing.

So I patched the shader in 65669be to not do that anymore, and that should fix the remaining errors. Which makes this issue fully resolved and I can finally close it 🎉

Thanks for all the help and diagnostic here, much appreciated! I looked at the ROG5 test output, it seems that the older drivers work equally well, which is very nice.

[bansan85]

Thanks for spending time on my problem :) I don't have any remaining problem.