Support EXT_disjoint_timer_query_webgl2

[mosra]

Continuation of #4575 (from 2016), I just ran into this today when adding support for this extension into Magnum.

Until now, most of the extension worked when using the core query APIs, except for glQueryCounterEXT() and glGetQueryObjecti64vEXT() -- there the functions assumed glGenQueriesEXT() was called that populated the internal id-to-object mapping array (and not glGenQueries()). So I added branches there that do the correct thing under WebGL 2.

Moreover, the queryCounterEXT property was undefined because the code asked for EXT_disjoint_time_query on WebGL 2 (which isn't exposed there). The code now asks for EXT_disjoint_timer_query_webgl2 there, which makes queryCounterEXT available.

Finally, because code coming from GLES may expect the EXT-suffixed versions still being available on WebGL 2 builds as well (since you can use the EXT-suffixed glGenQueriesEXT() etc. with EXT_disjoint_time_query on ES3 well), those are now aliases to the core APIs.

I did quite an extensive testing with Magnum's GL tests (both WebGL 1 and WebGL 2), but it may happen that this change will affect some Emscripten tests. Also, if there's any test file that I could update to cover the now-working APIs for WebGL 2, let me know.

EDIT: upon discovering that Firefox doesn't actually implement the _webgl2 version on WebGL 2 (https://bugzilla.mozilla.org/show_bug.cgi?id=1328882), I added a fallback to the WebGL 1 extension string. As far as my testing goes, the entry points work correctly, so it's just about the extension string.

Thank you!

[kripken]

Thanks @mosra!

I don't know enough for the GL aspects here, but I had some general comments. Also, I think we should add a test for at some of this, as it seems we have nothing for queries atm.

[kainino0x]

If you build an app with USE_WEBGL2, but then it creates a WebGL 1 context (perhaps because it is on a WebGL 1 browser), is it supposed to still work? I think it is, but there are some places in this patch where I think it may not.

If tests are added then it shouldn't be too hard to test with USE_WEBGL2 but still creating a WebGL 1 context.

[mosra]

@kainino0x Apologies -- I wasn't aware creating a WebGL 1 context with USE_WEBGL2=1 was even possible :) Any ideas on how should I proceed? If I see it correctly:

• for WebGL 1 builds #ifdefing the WebGL2-specific parts
• for WebGL 2 builds, it would all need to decide at runtime, based on which context is created, right? What's the "canonical" way to do it?

Thank you!

EDIT: Oh and re tests -- there's a problem that Firefox exposes the extension only under a flag (not sure how you can enable that on the CIs), and both chrome and firefox report 0s unless the results are queried async. It could check for GL errors at least, tho.

[kainino0x]

I wasn't aware creating a WebGL 1 context with USE_WEBGL2=1 was even possible

I'm not 100% sure it is, but I think it is. Hoping @juj would chime in.

I can look into it though.

EDIT: Oh and re tests -- there's a problem that Firefox exposes the extension only under a flag (not sure how you can enable that on the CIs), and

(FYI, I believe this is a Spectre mitigation.)
I don't know anything about Emscripten's CI so not sure if we'd be able to test with a flag, but @kripken certainly knows.

both chrome and firefox report 0s unless the results are queried async. It could check for GL errors at least, tho.

Yup, that's intended behavior. A test can just use emscripten_set_main_loop or similar to allow the browser to update the value.

[kainino0x]

It's also very valuable to have tests even if we have to run them manually/locally, so it would be great to have them.

[kripken]

Testing with a firefox pref is possible, see here where we already set up a few prefs:

emscripten/.circleci/config.yml

Line 131 in ea3209a

user_pref("gfx.offscreencanvas.enabled", true);

[kainino0x]

I can look into it though.

Tried a basic emscripten_webgl_init_context_attributes with USE_WEBGL2=1 and attrs.majorVersion = 1, and that works on Safari (which doesn't have WebGL 2). Didn't try anything more complex.

[kainino0x]

@mosra

• for WebGL 2 builds, it would all need to decide at runtime, based on which context is created, right? What's the "canonical" way to do it?

I think simply with conditionals like if (GL.currentContext.version >= 2).

[mosra]

Sorry for the silence on my end -- had two extremely busy weeks. Hoping to get back to this over the weekend or early next week.

[sbc100]

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[mosra]

Sorry for the over-a-year-long delay, finally found the time to wrap this up (also a bit frightened that nobody else needed this functionality yet, huh).

I rethought the original idea a bit and the actual change in functionality is a lot smaller than before:

• there's now tests for timer queries in WebGL 1, WebGL 2 and a WebGL 2 build with a WebGL 1 context
  • and sample queries in WebGL 2, because I realized that one way to fix time queries on WebGL 2 would break the other queries there, so better to have both covered
  • what's also good is that the functionality is no longer under a flag on Firefox, so I didn't need to do anything special to test it there
• removed the EXT aliases in WebGL 2 as those are not essential for my use case (and considering nobody else expressed interest in this during the year this PR was open, I doubt anybody else needs them either); plus it would make the implementation unnecessarily complex
• as @juj suggested, I removed the GL.timerQueriesEXT in favor of GL.queries that's now used in both WebGL 1 and WebGL 2
• on WebGL 2 builds, the glGetQueryObjectui64vEXT() does a runtime check and uses appropriate functionality depending on whether WebGL 1 or WebGL 2 context is created
• on WebGL 2, EXT_disjoint_timer_query_webgl2 is requested instead of EXT_disjoint_timer_query, together with a fallback for Firefox which exposes only EXT_disjoint_timer_query on both

What isn't in the tests here is the glQueryCounterEXT() function, as I had a difficulty with it (returning 0 on Chrome on WebGL 1, causing a GL error on Chrome on WebGL 2; crashing the tab on Firefox). Not sure what's up, will investigate further and open a followup PR with tests covering this API (and exposing this problem) once this one is merged.

[kainino0x]

Haven't had a chance to read through the tests yet, but I think this looks good! Great work with the thorough testing, based on your description - that really helps be confident things work and won't break later.

[kainino0x]

I'm not sure, is this the same as MAX_WEBGL_VERSION=1? Could we just explicitly specify =1?

[mosra]

Honestly I have no idea. Copypasted it from another occurence in this file which led me to believe it indeed is an implicit =1 :)

[kripken]

Yes, emcc allows -s X instead of -s X=1.

[kainino0x]

Sorry for the delay on this. Tests LGTM, though I had an idea about how we could make them a little more thorough. Thanks again for working on this!

Let us know if you won't have time to do this and I think I could pick up the change I suggested.

[mosra]

Thank you!