GL/DebugTools: read buffer data on WebGL 2.0 #560

pezcode · 2022-05-03T18:37:24Z

This became a bit of a rabbit hole 👀

GL::Buffer::data() and subData() are now supported on WebGL 2.0. This is exposed by Emscripten 2.0.17+ as a glGetBufferSubData() convenience entrypoint. On older Emscripten versions, data()/subData() are callable but lead to an "unreachable" assert.
DebugTools::bufferData() and bufferSubData() now forward to GL::Buffer::subData() where available, which now makes it work on desktop GL without requiring ARB_map_buffer_range and on WebGL 2.0. Essentially everywhere except WebGL 1.0 and GLES 2.0 without EXT_map_buffer_range
I updated a few GL tests to verify buffer contents on GLES 3 using DebugTools::bufferData(). Technically they could also be verified on non-WebGL GLES 2.0 with the right extension, but I didn't feel like adding all the checks for that

Todos:

Some documentation about requiring Emscripten 2.0.17 on WebGL 2
emscripten-webgl2 doesn't actually find glGetBufferSubData(), I probably messed up the Emscripten version check
On some CIs DebugTools/BufferData.cpp isn't being compiled, leading to missing symbols. Investigate.
~~Mac GLES3 CI has mysterious GL::Context::current(): no current context asserts in tests~~ postponed to Test buffer contents on GLES3 & WebGL, fix global symbol duplication in GL test libraries #565

Post-merge todos for @mosra:

Remove the template and de-inline the implementation of DebugTools::buffer[Sub]Data(). Originally Buffer used the same API pattern, but it got deprecated and eventually dropped in ecc0ab6 for being too error-prone. Time to do that here too.

src/Magnum/DebugTools/BufferData.h

mosra

Doesn't feel that much of a rabbit hole to me :)

I didn't feel like adding all the checks for that

Sounds reasonable, ES2 is nowadays only important as a debugging-friendly alternative to WebGL 1 anyway. And it's not like WebGL 1 would be that heavily used either. Plus, I don't remember any suspicious ES2-specific behavior until now so it's unlikely that the tests would discover some new bug :)

src/Magnum/GL/Buffer.cpp

mosra · 2022-05-04T09:34:16Z

src/Magnum/GL/Buffer.cpp

    glGetBufferSubData(GLenum(bindSomewhereInternal(_targetHint)), offset, size, data);
+    #else
+    CORRADE_ASSERT_UNREACHABLE("GL::Buffer::subData(): Buffer data queries in WebGL 2.0 possible only since Emscripten 2.0.17", );


As we discussed on Gitter, i'd make the whole thing fail at compile time, and not at runtime, to prevent an accidental footgun. So wrap this and also Buffer::subData() in #if __EMSCRIPTEN_major__*....

Hmm but that means all test code using DebugTools::bufferSubData() now has to do #if __EMSCRIPTEN_major__*... as well, umm... OTOH, it'd have to do that anyway, to prevent a runtime assert, so I guess that's fine. Yeah, so make this function disabled at compile time instead.

I was thinking... couldn't we fall back to using EM_ASM to call getBufferSubData() function on older Emscripten versions? There'd be some runtime overhead on those versions, but it's much better than not being able to read buffers at all. And it'd remove the need for ugly version checks around the two functions as well as in all tests.

Realistically, is anybody using such old Emscripten anymore? (Except me on the CI, which I should upgrade anyway.)

If ancient Emscriptens would be regularly used by people because they're in LTS Linux distro repos (which is the case with old GCC and old CMake), then it would make sense to add some fallback path, but I don't think anybody really does that.

I'd say -- let's upgrade the WebGL 2 build on the CI to 2.0.17 to not have to ifdef each and every test (can you do that in the PR?) but keep the API in GL::Buffer ifdef'd to not cause a linker failure for users on older versions. I don't expect regular users to build tests as well, so this should be fine. Then support for < 2.0.17 is just on a best-effort basis (we aim to support but we don't explicitly test for it). To keep at least some variety in the testing, the WebGL 1 CI build would stay on 1.39 for now and the next upgrade round (in a year or so?) would bump WebGL 2 build to 3.0.x and WebGL 1 to 2.0.x.

Hm, now I remember we still have the compatibility code for pre-1.39 event handling behavior in the Application classes, right? Probably a good time to drop that now :)

I upgraded to 2.0.17 for the emscripten-webgl2 job but it's failing with very funny errors. Not sure if we actually hit a compiler error there, but I'll try to build with that exact version locally.

src/Magnum/DebugTools/CMakeLists.txt

src/Magnum/DebugTools/BufferData.h

src/Magnum/GL/Test/BufferGLTest.cpp

src/Magnum/GL/Test/BufferImageGLTest.cpp

pezcode · 2022-05-05T12:10:43Z

The macos-gles3 failure seems to be a broken interaction with MagnumDebugTools and MagnumOpenGLTesterTestLib. I've read the comments about the latter, but it's not entirely clear to me why MagnumDebugTools breaks things.

mosra · 2022-05-13T10:25:32Z

src/Magnum/GL/Test/CMakeLists.txt

@@ -162,10 +170,10 @@ if(BUILD_GL_TESTS)
    endif()

    if(NOT MAGNUM_TARGET_GLES2)
-        corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumOpenGLTesterTestLib)
+        corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumDebugTools MagnumOpenGLTesterTestLib)


To fix the Mac CI failure, can you try swapping the order?

Suggested change

corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumDebugTools MagnumOpenGLTesterTestLib)

corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumOpenGLTesterTestLib MagnumDebugTools)

Same below. The *TestLib has the GL library built with graceful assertions, while the regular DebugTools links to a regular MagnumGL. Meaning, there's two duplicate candidates for the (global) GL symbols and the linker picks the first it finds (from DebugTools), but then the code in MagnumOpenGLTesterTestLib relies on the second duplicate, causing an assertion due to a different copy of the global used in different parts of the code.

I hope that's enough to make this work, if not then I'd probably need to introduce MagnumDebugToolsTestLib that relies on MagnumGLTestLib instead of MagnumGL.

Ah, thanks, I understand the issue now. Sadly changing the order doesn't seem to make a difference 😢 I assume CMake sorts the link order anyway

Since glGetBufferSubData() is only exposed on Emscripten 2.0.17 and up, both functions are not available on older versions. This is to avoid any accidental foot guns since it explodes at compile time. The webgl2 CI will be upgraded to 2.0.17 in a later commit.

…ata() This avoids requiring any Desktop GL extensions and allows forwarding to Buffer::subData() also on WebGL 2.0 (in a follow-up commit)

…r all cases

This is already done for bufferData()

Required for Buffer::data()/subData() used by GLES3 tests. Otherwise all the tests would have to check the Emscripten version and skip if it's too old.

mosra · 2022-05-15T13:33:51Z

Okay, seems we're at the cursed issues again. I'll take it from here :D

mosra · 2022-05-15T14:02:25Z

Ah yes, 2.0.18 doesn't die anymore, but I started hitting #507 now. Curses!

Investigating...

mosra · 2022-05-15T14:21:09Z

Ah well, and an attempted fix for the macOS global symbol mismatch, where I made MagnumDebugToolsTestLib depend on MagnumGLTestLib causes an ASan complaint due to duplicate symbols now appearing in different places. I suppose I just can't use DebugTools::bufferData() in the GL library tests, then...

pezcode · 2022-05-15T15:34:05Z

Oh dear, what have I done 🙊 sorry about this mess

mosra · 2022-05-15T16:54:37Z

All "good" for Emscripten now (see #507 (comment) for details).

For the other problem, I managed to "fix" it so now it's not macOS or ASan complaining, but Linux tests failing due to asserts not being graceful anymore. I suppose this is the culprit, gotta fix that properly I guess.

mosra · 2022-05-15T17:22:17Z

Hm, reshuffled the library order once more and now it seems to work. Although it's a tower of brittle hacks and wishful thinking, hopefully not breaking again anytime soon.

Will merge once I implement the template-removal TODO as well. Tomorrow, enough suffering for a Sunday 💀

mosra · 2022-05-25T07:35:20Z

Finally merged a subset of this as 500e41e...4350f13, sorry for the extremely long delay. CircleCI macOS builds started also acting up in the meanwhile, further delaying everything.

I ended up deprecating the templated bufferData() API in 0113b34 and then rebased your commits on top of that, as that order made more sense (and allowed me to reduce the deprecated code size quite a bit). Using DebugTools inside GL tests however proved to be too cursed, so I split that off into #565 for now. Will get back to fixing that later.

Thanks for your work!

pezcode commented May 3, 2022

View reviewed changes

src/Magnum/DebugTools/BufferData.h Outdated Show resolved Hide resolved

mosra changed the base branch from next to master May 4, 2022 09:00

mosra added this to the 2022.0a milestone May 4, 2022

mosra reviewed May 4, 2022

View reviewed changes

pezcode force-pushed the webgl2-buffer-data branch from 9fa66f0 to 09ce4ee Compare May 4, 2022 18:49

pezcode force-pushed the webgl2-buffer-data branch from 09ce4ee to c43151b Compare May 5, 2022 23:56

mosra reviewed May 13, 2022

View reviewed changes

pezcode added 7 commits May 14, 2022 22:16

DebugTools: make bufferData()/bufferSubData() default to Buffer::subD…

effebf3

…ata() This avoids requiring any Desktop GL extensions and allows forwarding to Buffer::subData() also on WebGL 2.0 (in a follow-up commit)

DebugTools: make bufferData()/bufferSubData() available on WebGL 2.0

b929cf4

DebugTools: handle and test size 0 in bufferData()/bufferSubData() fo…

63dd315

…r all cases

DebugTools: make bufferSubData()'s template type default to char

de94fc9

This is already done for bufferData()

GL, Text: verify buffer contents on GLES3

48d97eb

package/ci: Run emscripten-webgl2 on Emscripten 2.0.17

1ae3471

Required for Buffer::data()/subData() used by GLES3 tests. Otherwise all the tests would have to check the Emscripten version and skip if it's too old.

pezcode force-pushed the webgl2-buffer-data branch from c43151b to 1ae3471 Compare May 14, 2022 20:21

mosra mentioned this pull request May 16, 2022

Emscripten toolchain should point to cache sys root mosra/corrade#133

Closed

mosra mentioned this pull request May 25, 2022

Test buffer contents on GLES3 & WebGL, fix global symbol duplication in GL test libraries #565

Draft

4 tasks

mosra closed this May 25, 2022

mosra added the changelog mention added label May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GL/DebugTools: read buffer data on WebGL 2.0 #560

GL/DebugTools: read buffer data on WebGL 2.0 #560

pezcode commented May 3, 2022 •

edited by mosra

Loading

mosra left a comment

mosra May 4, 2022

pezcode May 4, 2022

mosra May 13, 2022

pezcode May 15, 2022

pezcode commented May 5, 2022

mosra May 13, 2022

pezcode May 15, 2022

mosra commented May 15, 2022

mosra commented May 15, 2022

mosra commented May 15, 2022

pezcode commented May 15, 2022

mosra commented May 15, 2022 •

edited

Loading

mosra commented May 15, 2022

mosra commented May 25, 2022

	corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumDebugTools MagnumOpenGLTesterTestLib)
	corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumOpenGLTesterTestLib MagnumDebugTools)

GL/DebugTools: read buffer data on WebGL 2.0 #560

GL/DebugTools: read buffer data on WebGL 2.0 #560

Conversation

pezcode commented May 3, 2022 • edited by mosra Loading

mosra left a comment

Choose a reason for hiding this comment

mosra May 4, 2022

Choose a reason for hiding this comment

pezcode May 4, 2022

Choose a reason for hiding this comment

mosra May 13, 2022

Choose a reason for hiding this comment

pezcode May 15, 2022

Choose a reason for hiding this comment

pezcode commented May 5, 2022

mosra May 13, 2022

Choose a reason for hiding this comment

pezcode May 15, 2022

Choose a reason for hiding this comment

mosra commented May 15, 2022

mosra commented May 15, 2022

mosra commented May 15, 2022

pezcode commented May 15, 2022

mosra commented May 15, 2022 • edited Loading

mosra commented May 15, 2022

mosra commented May 25, 2022

pezcode commented May 3, 2022 •

edited by mosra

Loading

mosra commented May 15, 2022 •

edited

Loading