[html5] Add desynchronized WebGL context attribute#27163
[html5] Add desynchronized WebGL context attribute#27163UnknownHacker1 wants to merge 9 commits into
desynchronized WebGL context attribute#27163Conversation
Add a `desynchronized` field to EmscriptenWebGLContextAttributes. When set, emscripten_webgl_create_context() passes both `desynchronized` and its legacy name `lowLatency` to canvas.getContext(), and emscripten_webgl_get_context_attributes() reads the value back. It defaults to false via the existing memset in emscripten_webgl_init_context_attributes. Regenerated struct_info and documented the new field.
sbc100
left a comment
There was a problem hiding this comment.
You can use is_chrome() in test/test_browser.py to have the test assert different things on chrome to other browsers.
| 'failIfMajorPerformanceCaveat': !!HEAP8[attributes + {{{ C_STRUCTS.EmscriptenWebGLContextAttributes.failIfMajorPerformanceCaveat }}}], | ||
| 'desynchronized': !!HEAP8[attributes + {{{ C_STRUCTS.EmscriptenWebGLContextAttributes.desynchronized }}}], | ||
| // 'lowLatency' is the legacy name for 'desynchronized'. | ||
| 'lowLatency': !!HEAP8[attributes + {{{ C_STRUCTS.EmscriptenWebGLContextAttributes.desynchronized }}}], |
There was a problem hiding this comment.
What browser versions require/use the legacy name? Perhaps you could add a comment here linking to caniuse data and then if an #if MIN_CHROME_VERSION < XXX || MIN_FIREFOX_VERSION < YYY around this line.
This will allow is us to one day completely delete this line once we drop support for those older browsers.
Add an assertion to the html5_webgl browser test that the desynchronized attribute round-trips on Chrome, where it is supported, and reads back false on browsers that do not, selected via is_chrome(). lowLatency was Chrome's name for desynchronized in versions 73-74, so only emit it when targeting Chrome < 75.
|
Thanks for your review @sbc100 Done both:
|
| // 'lowLatency' was Chrome's experimental name for 'desynchronized', renamed in Chrome 75. | ||
| // See https://caniuse.com/?search=desynchronized. Drop once we no longer support Chrome < 75. | ||
| 'lowLatency': !!HEAP8[attributes + {{{ C_STRUCTS.EmscriptenWebGLContextAttributes.desynchronized }}}], | ||
| #endif |
There was a problem hiding this comment.
Ah, since OLDEST_SUPPORTED_CHROME is currently 85 (see feature_matrix.py) we can just delete this !
sbc100
left a comment
There was a problem hiding this comment.
lgtm with that block removed
|
Thanks for picking up that old bug and fixing it! Much appreciated. |
desynchronized WebGL context attribute (#8406)desynchronized WebGL context attribute
|
Removed, thanks! And thanks for the quick reviews. |
|
|
||
| .. c:member:: bool desynchronized | ||
|
|
||
| If ``true``, requests a "desynchronized" WebGL context, which can lower latency by bypassing the browser's normal compositing/double-buffering of the canvas. This maps to the ``desynchronized`` attribute on the JavaScript ``getContext()`` call (and its legacy name ``lowLatency``). Whether the request is honored depends on the browser and platform; use ``emscripten_webgl_get_context_attributes()`` to read back the value the context was actually created with. Default value is ``false``. |
There was a problem hiding this comment.
maybe remove the reference to the legacy name here?
Head branch was pushed to by a user without write access
|
@sbc100 Done. I think you need to re-enable auto-merge |
|
@sbc100 The Codesize check is just the expected webgl sizes shifting from the added glue. Should I rebaseline, or do we just merge through? |
|
the error message says that "-- This failure is only a warning and can be ignored" |
|
Yes, please rebasline. Do to this you need to make sure you have the very latest emsdk version with |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
|
I you like I can push the codesize updates to this branch? (assuming I have access) |
|
Uppdate on two CI things: Browser tests: fixed the chrome-2gb / wasm64-4gb failures. Those are the OffscreenCanvas variants, and desynchronized isn't honored on OffscreenCanvas, so my round-trip check was asserting the wrong value there. It now only runs on a normal canvas. Codesize: this one looks like it's failing on the target branch, not this PR. The run checks out main and reports 10 expectation files out of date from LLVM drift (the cxx_* tests, -46 bytes each), ending with "run the Rebaseline Tests github action on the target branch (main)." My change's own sizes all matched. Could you run that on main, or merge through? |
Sorry, didn't see your reply. Yes, that'd be great. |
|
Oh wait.. the expectation need updating on main. I will take care of that. |
|
#27170 is now landed. Can you rebase and update the codesize expectations again? (you might also need to ./emcc --clear-cache) |
…ized-canvas # Conflicts: # test/codesize/test_codesize_hello_dylink_all.json
|
@sbc100 The dylink_all codesize baseline keeps conflicting as main drifts. Since you've approved, could you just merge it (or push the codesize update from your side)? I can rebase again but it'll likely drift before it lands. |
|
@sbc100 codesize check failed again |
Adds a
desynchronizedfield toEmscriptenWebGLContextAttributes. When set,emscripten_webgl_create_context()passes bothdesynchronizedand its legacy namelowLatencytocanvas.getContext(), andemscripten_webgl_get_context_attributes()reads the value back so it round-trips. Defaults tofalsevia the existing memset inemscripten_webgl_init_context_attributes(). Regeneratedstruct_info(wasm32 + wasm64) and documented the new field.Tested manually in Chrome: a context created with
desynchronized=truereads back1; default is0.On the test: happy to add one, but since whether a browser honors
desynchronizedis platform-dependent, asserting the readback value could be flaky in CI. Would a test that hooksgetContextto check the requested attribute be preferred? Open to guidance.Fixes #8406.