-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hello, WebGL 2.0 Compute #7612
Hello, WebGL 2.0 Compute #7612
Conversation
The patchset is temporarily based on #7105. |
src/parseTools.js
Outdated
@@ -8,6 +8,58 @@ | |||
|
|||
//"use strict"; | |||
|
|||
var makePPEnum = (function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change could be a separate PR?
I think the topic was discussed before, and the hope was we can avoid adding a full parser like this, and can use eval instead?
emscripten.py
Outdated
@@ -582,10 +582,13 @@ def read_proxied_function_signatures(asmConsts): | |||
|
|||
|
|||
def compile_settings(compiler_engine, libraries, temp_files): | |||
settings = shared.Settings.to_dict() | |||
settings.update((key, int(value)) for key, value in settings.items() if isinstance(value, bool)) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes also seem like they should be in separate PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's part of #7105
src/library_gl.js
Outdated
@@ -33,14 +33,14 @@ var LibraryGL = { | |||
currentContext: null, | |||
offscreenCanvases: {}, // DOM ID -> OffscreenCanvas mappings of <canvas> elements that have their rendering control transferred to offscreen. | |||
timerQueriesEXT: [], | |||
#if USE_WEBGL2 | |||
#if USE_WEBGL2 || USE_WEBGL2_COMPUTE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps WEBGL2_COMPUTE should require that WEBGL2 is on, so we don't need to ||
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did hesitate for some time if USE_WEBGL2_COMPUTE
should imply USE_WEBGL2
. I made the current choice because
- WebGL 2.0 is provided by the
webgl2
context (WebGL2RenderingContext
) and WebGL 2.0 Compute is provided by thewebgl2-compute
context (WebGL2ComputeRenderingContext
), so WebGL 2.0 Compute is not simply an extension of WebGL 2.0. - For a native application in C/C++, the two contexts are usually created with different minor version number.
It gives me the impression that USE_WEBGL2
and USE_WEBGL2_COMPUTE
should exist at the same level but be exclusive to each other. Tens of USE_WEBGL2 || USE_WEBGL2_COMPUTE
is bad, but the other style sounds slightly worse to me at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably introduce another symbol, like HAVE_WEBGL2
or AT_LEAST_WEBGL2
ALLOW_WEBGL2
, that would be more clear in its intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As another proposal, use #if WEBGL_MAJOR_VERSION >= 2
. Not sure if it's better or not.
I don't have the full context for compute (will need to look into it), but some initial comments above. Overall this is very nice work! The compute-specific changes in this PR look generally ok to me, my only concerns at this point are some of the support code like the parsing and python changes etc., but those can be separated out I think. |
8e48bd2
to
12d60f8
Compare
Glad to know the overall opinion. Will address the preprocessor concerns next. |
Perhaps it would be useful for review to link to the compare between #7105 and here, so that the changes are not squashed together: |
This is tremendously exciting @hujiajie ! Please follow @kripken and @kainino0x 's advice to get this landed. Great work! |
Ok, the preprocessor issues should be sorted out now after #7651 which just landed. Rebasing this to contain just the WebGL 2 Compute stuff should now be possible. |
Really appreciate the effort! I'll post an update in a day or two. |
Given that WEBGL2_COMPUTE implies required WebGL2 support, and now that Alon landed that excellent more flexible preprocessor support, how about introducing a new flag
which can take one of values -#if USE_WEBGL2 || USE_WEBGL2_COMPUTE
+#if MAX_WEBGL_VERSION >= 20
-#if USE_WEBGL2 && USE_WEBGL2_COMPUTE
+#if MAX_WEBGL_VERSION >= 21 Then let's change the Note that the meaning of current In the future, the above scheme will allow us to add another flag, |
src/library_gl.js
Outdated
// WebGL 2.0 Compute is deemed to be a design mistake in naming. The | ||
// following constants will make the life easier. | ||
WEBGL2_COMPUTE_MAJOR_VERSION: 2, | ||
WEBGL2_COMPUTE_MINOR_VERSION: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines would introduce regression on pages that target WebGL 2.0 only, but not WebGL 2.0 Compute. Btw about the comment here - is there a future direction to rename WebGL 2.0 Compute to WebGL 2.1 officially? Or how does the comment relate to current thinking? (If the API name "WebGL 2.0 Compute" is deemed to be permanent, we do not want to retain judgement in comments, so could leave that out, even if we do feel it was the wrong call)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to remove the subjective comment before committing.
I do not think WebGL 2.0 Compute will be renamed to WebGL 2.1 or 3.0 in the near future. Maybe @kenrussell or @gyagp have more insight on this.
And can you elaborate more on the regression? Is it about code size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Apple stops to advance OpenGL, WebGL 2.0 Compute can't run on macOS/iOS. Given it's not a general solution for all platforms, the decision from Khronos WebGL Working Group was to make it an extension, rather than a concrete version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And can you elaborate more on the regression? Is it about code size?
Oh sorry, yeah, meant here a regression on code size specifically.
I do not think WebGL 2.0 Compute will be renamed to WebGL 2.1 or 3.0 in the near future.
If WebGL 2.0 Compute will not be renamed to WebGL 2.1, but we still have items like
WEBGL2_COMPUTE_MAJOR_VERSION: 2,
WEBGL2_COMPUTE_MINOR_VERSION: 1,
making Emscripten treat WebGL 2.0 Compute as if it was WebGL 2.1, why should we diverge from them and call it 2.1 if Khronos WebGL WG does not? Is there a danger of the working group introducing a "real" WebGL 2.1 at some point after this, and then we would be left with a collision with version numbering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebGL version number is heavily used in library_gl.js
for comparison, so I suppose WebGL 2.0 Compute also needs a numerical representation for this, but it looks inappropriate to just use 2.0
. A real WebGL 2.1 could be the risk, and I haven't got a better idea yet.
src/library_gl.js
Outdated
webGLContextAttributes['minorVersion'] = defaultContext.minorVersion; | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code would introduce a code size regression for WebGL 1.0 and WebGL 2.0 code when WebGL 2.0 Compute was not being targeted. I can appreciate the desire to build simple looking code, though we need to come up with a way to do this in a manner that will at least Closure optimize away well when WebGL 2.0 Compute is not being targeted (and when WebGL 2.0 is not being targeted either)
src/library_gl.js
Outdated
@@ -720,18 +760,19 @@ var LibraryGL = { | |||
var context = { | |||
handle: handle, | |||
attributes: webGLContextAttributes, | |||
version: webGLContextAttributes['majorVersion'], | |||
majorVersion: webGLContextAttributes['majorVersion'], | |||
minorVersion: webGLContextAttributes['minorVersion'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The three above code blocks also would regress code size for WebGL1 and WebGL 2. Instead of majorVersion
and minorVersion
, I think it'd be better to do integers 10
, 20
and 21
, the generated code size will be smaller that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was over-engineered because 210
can mean either 2.10
or 21.0
, and 3.0
should be larger than 2.10
. But I'm not opposed of your suggestion considering it's pretty safe to leave the situation unhandled until it really happens. Just a little concerned about the readability for now, so looking forward to others' opinion.
src/library_gl.js
Outdated
} else if (GL.currentContext.majorVersion == 2 && GL.currentContext.minorVersion == 0) { | ||
glVersion = 'OpenGL ES 3.0'; | ||
} else if (GL.currentContext.majorVersion == 1 && GL.currentContext.minorVersion == 0) { | ||
glVersion = 'OpenGL ES 2.0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also regress WebGL 1.0/WebGL 2.0 code size.
src/library_gl.js
Outdated
} else if (GL.currentContext.majorVersion == 2 && GL.currentContext.minorVersion == 0) { | ||
major = 3; | ||
minor = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise
src/library_gl.js
Outdated
prelude = '#version 310 es\n'; | ||
} else if (GL.currentContext.majorVersion == 2 && GL.currentContext.minorVersion == 0) { | ||
prelude = '#version 300 es\n'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise
system/lib/gl/gl.c
Outdated
if (!strcmp(name, "glDispatchCompute")) return emscripten_glDispatchCompute; | ||
if (!strcmp(name, "glBindImageTexture")) return emscripten_glBindImageTexture; | ||
if (!strcmp(name, "glMemoryBarrier")) return emscripten_glMemoryBarrier; | ||
if (!strcmp(name, "glMemoryBarrierByRegion")) return emscripten_glMemoryBarrierByRegion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also regresses code size, in a particularly tricky way. For this we should migrate to codegenning this differently per API level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, many ES 3.1 APIs are still not added here, so the code size is expected to be increased further. Can we leave codegenning for future improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the regression from adding all these strings? Or is it somehow specific to glMemoryBarrierByRegion? Can this entire block just be inside of an ifdef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the regression is that these lines keep alive each of those functions whose address they return. So the compiler can't dce them out, they are all used.
We do build this file separately for when GL emulation is present. We could build separate versions for the different GL levels, to reduce the problem. That code is in tools/system_libraries.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, sounds like a reasonable fix to me. FYI @hujiajie: In tools/system_libs.py
, I'm looking at create_gl
. It does flags += legacy_gl_emulation_flags(libname)
. I think the suggestion is to add another flags += webgl2_compute_flags()
similar to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the idea has already been implemented in ef230f5. I'm rebasing on top of that.
Cool work! One other direction we should investigate here is that instead of adding WebGL 2.0 Compute support in to Another benefit of separate files is that if we did the above |
I agree it would be nice to split up library_gl.js. One major piece of work that I think will eventually be necessary is to rewrite some stuff as library_gl*.c when host bindings are available. (I forget whether I've talked to you about this.) Host Bindings (with Anyref) would allow WASM to call directly into Web API methods for better performance, which means all of the ES-to-WebGL translation has to happen in C rather than JS. In order to write library_gl.c at all, browser support for Anyref may be necessary. Also, it's not necessary to move the entire thing wholesale to use host bindings - it's only really going to be important for the more performance critical GL calls. So maybe it's better to keep most of library_gl as JS. |
Another thought on MIN_/MAX_WEBGL_VERSION: I'd prefer not to take the "21" name for WebGL compute just in case there is eventually a 2.1 that isn't exactly the same as webgl2-compute. How about ALLOW_WEBGL_VERSION=1,2,2compute or 10,20,20compute or webgl1,webgl2,webgl2-compute? Then just assert on invalid combinations of those. |
I like the idea of
|
Definitely, yeah, eventually we will want to migrate most or all of our GL bindings into compiled code, when wasm supports that. We definitely block on |
5e9686d
to
2eb13d6
Compare
The PR has been rebased with the following major changes:
Thoughts? |
Will this also include read/write support for compute shaders reading from and writing to textures? |
@jjxtra Yes. You can bind a level of a texture to an image unit with |
954cd01
to
2343660
Compare
06d2b92
to
f61531b
Compare
f61531b
to
ee74a7e
Compare
WebGL 2.0 Compute is an experimental superset of WebGL 2.0, provided by Chrome/Chromium behind the flag `--enable-webgl2-compute-context`. It exposes OpenGL ES 3.1 functionality, notably compute shader, to WebGL via the `webgl2-compute` context.[1][2] The patch adds a new flag `-s USE_WEBGL2_COMPUTE=` to opt-in support for WebGL 2.0 Compute, which is similar to but exclusive with `-s USE_WEBGL2=`. For now, only existing OpenGL ES 3.0 entry points are exposed in this context, and others (those introduced by OpenGL ES 3.1) will be added next. [1] https://www.khronos.org/registry/webgl/specs/latest/2.0-compute/ [2] https://groups.google.com/a/chromium.org/d/topic/blink-dev/bPD47wqY-r8/discussion
The test is expected to work on Windows and Linux with EMTEST_BROWSER="chrome --use-gl=angle --use-angle=gl --use-cmd-decoder=passthrough --enable-webgl2-compute-context". Sadly, the Mesa software renderer is too slow and lacks certain OpenGL extensions for WebGL 2.0 Compute to work, so the test is not enabled in CI.
ee74a7e
to
943b2b1
Compare
The deletion of the Re-opening and re-targeting to master. |
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. |
WebGL 2.0 Compute is an experimental superset of WebGL 2.0, provided by Chrome/Chromium behind the flag
--enable-webgl2-compute-context
. It exposes OpenGL ES 3.1 functionality, notably compute shader, to WebGL via thewebgl2-compute
context[1][2]. The patchset adds a new flag-s USE_WEBGL2_COMPUTE=
and implements preliminary support for this feature, in the hope that it can benefit applications in the field of machine learning, image processing, gaming, etc. A ray tracing demo is also provided as a test, but sadly it's skipped in CI because the Mesa software renderer is too slow and lacks certain OpenGL extensions for WebGL 2.0 Compute to work.[1] https://www.khronos.org/registry/webgl/specs/latest/2.0-compute/
[2] https://groups.google.com/a/chromium.org/d/topic/blink-dev/bPD47wqY-r8/discussion