-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Multithreading 14/N: OFFSCREEN_FRAMEBUFFER #5581
Conversation
I don't understand this - when would this be used? |
eed8cbf
to
ca78adb
Compare
ca78adb
to
d9300bb
Compare
This is basically a polyfill/emulation in the lack of real OffscreenCanvas support. If a browser doesn't support OffscreenCanvas, we can still proxy all WebGL commands to the main thread. However when doing so, we can't issue the draw calls directly to the WebGL backbuffer, but must issue them to an offscreen FBO because of the implicit "WebGL presents rendered content when one returns from an event handler" swap logic. |
src/library_glfw.js
Outdated
@@ -996,6 +996,11 @@ var LibraryGLFW = { | |||
stencil: (GLFW.hints[0x00021006] > 0), // GLFW_STENCIL_BITS | |||
alpha: (GLFW.hints[0x00021004] > 0) // GLFW_ALPHA_BITS | |||
} | |||
#if OFFSCREEN_FRAMEBUFFER | |||
// TODO: Make GLFW explicitly aware of whether it is being proxied or not, and set these to true only when proxying is being performed. | |||
contextAttributes.renderViaOffscreenBackBuffer = true; |
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 combination of 2 lines appears 3 times. perhaps we should have a shared function for it?
or can those lines go in createContext which is called by them?
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.
Hmm, I think that would make the field initialization more cumbersome to read, since I'd still want to #if calls to that function out, and having an extra function, say,
#if OFFSCREEN_FRAMEBUFFER
GL.enableOffscreenBackBuffer = function(attributes) {
attributes.renderViaOffscreenBackBuffer = true;
attributes.preserveDrawingBuffer = true;
}
#endif
...
glfwCreateWindow: function() {
...
#if OFFSCREEN_FRAMEBUFFER
GL.enableOffscreenBackBuffer(contextAttributes);
#endif
...
}
builds an indirection that probably does not save much, and adds a mental detour when reading the code.
The semantical idea of -s OFFSCREEN_FRAMEBUFFER=1
follows the same idea that -s USE_WEBGL2=1
also does:
- At compile-time, enable generating in the code to add the feature,
- For APIs that are not able to express initialization parameters between Offscreen Framebuffer or not, always enable the feature when creating the context (GLUT, GLFW, SDL),
- For HTML5 API, in which we are able to express initialization parameters for enabling Offscreen Framebuffer or not, don't always-enable the feature, but let user's submit the desired initialization mode in the context init attribute parameters.
This means that we would not want to make createContext
unconditionally enable these bits for all created contexts if -s OFFSCREEN_FRAMEBUFFER=1
, but let the caller decide.
(Also duplicating the code between SDL, GLUT and GLFW will not be a duplication in generated code, since those APIs are generally never used at the same time)
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.
Agreed on createContext, thanks.
I see what you're saying about the code duplication issue, but on the other hand, it would make the code more maintainable to use a little helper. It's likely we'll have more work done on these APIs, and having to remember to update all the various locations is risky.
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.
Gotcha, updated the code, how does it look now?
src/settings.js
Outdated
@@ -850,6 +850,7 @@ var TEXTDECODER = 1; // Is enabled, use the JavaScript TextDecoder API for strin | |||
var OFFSCREENCANVAS_SUPPORT = 0; // If set to 1, enables support for transferring canvases to pthreads and creating WebGL contexts in them, | |||
// as well as explicit swap control for GL contexts. This needs browser support for the OffscreenCanvas | |||
// specification. | |||
var OFFSCREEN_FRAMEBUFFER = 0; // If set to 1, enables rendering to an offscreen render target first, and then finally flipping on to the screen. |
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.
please add more details here, in particular how it is tied to OFFSCREENCANVAS_SUPPORT
and pthreads.
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.
Gotcha, updated the notes here, which were a bit imprecise before as well.
e1eb0f3
to
ef6d60a
Compare
lgtm, thanks! |
src/library_html5.js
Outdated
} | ||
|
||
if (canvas.transferControlToOffscreen) { | ||
GL.offscreenCanvases[canvas.id] = canvas.transferControlToOffscreen(); | ||
GL.offscreenCanvases[canvas.id].id = canvas.id; | ||
canvas = GL.offscreenCanvases[canvas.id]; | ||
} | ||
} | ||
#else |
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.
note to self -- this does not seem to be "replaced" with the following line
ef6d60a
to
9a60599
Compare
Rebased this on top of latest incoming, it's been some time. @kripken : does this still look like lgtm to land? (The test for this comes eventually in at https://github.com/kripken/emscripten/pull/6254/files#diff-b739639dfafbbd8097296133efb95a05R3808) |
@juj Before landing this I think we need a plan for maintenance. I don't fully understand this code, and I don't think anyone other than you does. What is your availability for supporting this code going forward? |
Recall the original
This has the effect that returning from an event handler function will not cause the partially so far rendered content to be presented with such a "implicit swap" mechanism that WebGL has, but since all the rendering was done to an offscreen surface, the drawn content will stay hidden between multiple event callback functions. To present this offscreen rendered content on screen, one can either do so explicitly in user code by calling Overall the effects are that As for availability, I am still around, even though I have not recently had the capacity to follow everything that is going on. I am hoping that I'd be able to work the pending PRs to such a state that they can be understood by more people, and not bitrot as multithreading eventually becomes available in WebAssembly. |
Thanks for the detailed explanation! Aside from being around to rework your PRs, it sounds like you are not available to maintain the multithreading code going forward? In that case, we need to find someone that can before we merge new big changes, or think about another approach. (In general, for code without a maintainer, it would be good to only merge things that simplify it, not add features and complexity, in my opinion.) |
Threading is the number 1 post-MVP thing that everyone is asking us for, and I definitely consider threading in general as something that we will need to keep maintaining (where "we" is both the general emscripten community and our Google team specifically). I've recently been testing that along with the browser implementation of wasm threads, and I think we'll want to keep considering threading in general as first-class. As for offscreen framebuffer specifically, that isn't something I've thought much about, although my sense from talking to WebGL folks is that offscreen canvas is something they care about generally (and that game engines want). I think I have a decent handle on the general multithreading stuff, but less so on the graphics-specific stuff. |
Thanks @dschuff! If you can review the general multithreading PRs that would be great. About the graphics-focused ones, like this one, you're probably still the best person we have currently since you at least know the non-graphics portions. I'd say in general for situations like this where we don't have perfect reviewers for, I think it's reasonable to merge things if they at least make general sense to the best reviewer we have, they add tests as necessary, and they don't introduce complexity for other stuff. |
90e38e5
to
765c06d
Compare
Updated this PR to latest and added a dedicated test and documentation to make this easier to merge without dependencies to the other multithreading pull requests. @dschuff : would you be able to give this a look? |
…ased fallback to doing WebGL from a pthread when OffscreenCanvas is not available, as well as doing explicitSwapControl semantics for a WebGL context.
…context attributes for Offscreen Framebuffer use
…reenBackBuffer when OffscreenCanvas is not supported. Add testing for OFFSCREEN_FRAMEBUFFER mode. Add documentation for OFFSCREEN_FRAMEBUFFER.
765c06d
to
02aa29f
Compare
Ping @dschuff @kripken : I am currently a bit paralyzed by the number of pending PRs I have, and with the recent Unity multithreading work we have been doing, what we have is a bit of a forked direction of Emscripten. I'd really like not to diverge, although can appreciate with all that is going on that there is difficulty for people to find time to review. Any chance these could be checked from the perspective that they are reviewed that the CI bot is green, and that the existing usage without the feature enabled, for example in the case of this PR, the existing usage when the OFFSCREEN_FRAMEBUFFER feature is disabled, is not affected? |
Hi @juj, I think this is OK to go it. I have a little bit of concern about an explosion of options, but if I understand correctly this is really just a polyfill for offscreen canvas, so I'd be happy to make that easier to use. Relatedly, have you been following the progress of the offscreen canvas API more generally? Apparently there were some concerns about the API during standardization review (see e.g. w3ctag/design-reviews#288 and w3ctag/design-reviews#141) so they are considering some changes. If we're going to have a polyfill for offscreen canvas, It would be good if we were convinced we could implement whatever they come up with. Also if you have input on the API proposal itself, I'm sure your input would be valued. |
(but LGTM for now, especially if it's going to help you land your other stuff in emscripten) |
OFFSCREEN_FRAMEBUFFER is not a polyfill in the traditional sense that it would make it appear as if the JS context then had support for OffscreenCanvas if it does not support it built-in. It can however be used as a fallback for when OffscreenCanvas is not available - this all is independent of the "synchronous .commit() API in a blocking loop" conversations. I have been following the conversation on occassion. Currently I do not see anything critical happening there, although if |
This breaks GL context creation in incoming because the added
|
Thanks for noticing. The initialization check was intended to be a hint for users, but we don't want such check to be present in release builds. Put the init check behind |
Implement
-s OFFSCREEN_FRAMEBUFFER=1
option which allows a proxying based fallback to doing WebGL from a pthread when OffscreenCanvas is not available, as well as doingexplicitSwapControl
semantics for a WebGL context (independent of whether one is proxying).