Skip to content

Fix issue with PROXY_TO_PTHREAD and OFFSCREENCANVAS_SUPPORT clashing#8901

Closed
samsinsane wants to merge 1 commit intoemscripten-core:mainfrom
Euclideon:offscreencanvas_proxy
Closed

Fix issue with PROXY_TO_PTHREAD and OFFSCREENCANVAS_SUPPORT clashing#8901
samsinsane wants to merge 1 commit intoemscripten-core:mainfrom
Euclideon:offscreencanvas_proxy

Conversation

@samsinsane
Copy link

  • eglCreateContext and eglMakeCurrent no longer proxy to the main thread when using an offscreen canvas

Resolves #8852

- eglCreateContext and eglMakeCurrent no longer proxy to the main thread when using an offscreen canvas
@kripken
Copy link
Member

kripken commented Jul 8, 2019

Thanks @samsinsane!

This seems like a general issue, and maybe there's a better solution I'm not aware of. @juj, should all these proxied methods just be turned off when offscreen canvas is enabled? Perhaps we should do that automatically for all proxied methods in library_*gl*.js?

@juj
Copy link
Collaborator

juj commented Jul 9, 2019

This seems like a general issue, and maybe there's a better solution I'm not aware of. @juj, should all these proxied methods just be turned off when offscreen canvas is enabled? Perhaps we should do that automatically for all proxied methods in library_*gl*.js?

No - that goes against the current meaning of these build flags.

The meaning of OFFSCREENCANVAS_SUPPORT=1 linker flag is to choose at link time that generated build supports (is aware of) OffscreenCanvas, but does not mean that generated build requires OffscreenCanvas. That is, when one builds with OFFSCREENCANVAS_SUPPORT=1 they can still architect a scheme that has a fallback (e.g. via offscreen frame buffer proxying) if OffscreenCanvas support is not detected at runtime.

We should not dictate proxying at link time based on OFFSCREENCANVAS_SUPPORT=1 vs OFFSCREENCANVAS_SUPPORT=0 setting. The proper Khronos/EGL-esque way here would be to add a custom Emscripten EGL extension that would allow code at runtime to signal to EGL implementation if OffscreenCanvas should be used, and if so, then implementation chooses not to proxy, but create the OffscreenCanvas context locally.

The HTML5 API for creating GL contexts works in this way - depending on the context creation flags, proxying is done or skipped.

However architecting custom EGL extensions can be more annoying than is worth it, so I'd be open to utilizing a new option -s OFFSCREENCANVAS_SUPPORT=2 to mean a link time decision "generated build requires OffscreenCanvas or else it won't run". The PR here would then become

#if OFFSCREENCANVAS_SUPPORT != 2
  eglCreateContext__proxy: 'sync',
#endif

then one can build with -s OFFSCREENCANVAS_SUPPORT=2 to tell at link time to make codegen decisions that require OffscreenCanvas support to be present.

@samsinsane
Copy link
Author

I believe that OFFSCREENCANVAS_SUPPORT=1 is causing my canvas to be sent offscreen, via transferControlToOffscreen() somewhere, I think this based on what you've said above, this is wrong too? Also, I can't really see how eglCreateContext can ever work with a canvas that has been transferred like this? It will always proxy to the main thread despite the canvas that it is given not allowing the context to be created on the main thread. Isn't it possible to detect this via controlTransferredOffscreen?

I don't recall exactly what happened with eglMakeCurrent, I think it would set the GLctx on the main thread but all of my GL functions weren't being proxied to the main thread. However, I can't see any reason to set the context on the main thread if the canvas was transferred to the pthread?

@juj
Copy link
Collaborator

juj commented Jul 11, 2019

I believe that OFFSCREENCANVAS_SUPPORT=1 is causing my canvas to be sent offscreen, via transferControlToOffscreen() somewhere, I think this based on what you've said above, this is wrong too?

Hmm, I think the transfer is due to this limitation marked with a TODO:

// TODO: Add a -s PROXY_CANVASES_TO_THREAD=parameter or similar to allow configuring this
. You are building with -s PROXY_TO_PTHREAD=1 as well?

In that code, EMSCRIPTEN_PTHREAD_TRANSFERRED_CANVASES is never currently defined, so the current behavior with -s PROXY_TO_PTHREAD=1 is that this branch is taken

// Otherwise by default, transfer whatever is set to Module.canvas, because in PROXY_TO_PTHREAD
, hence whatever is set to Module.canvas will be transferred to Offscreen.

The current problem is that the function proxy_main is only compiled once at library precompilation time, so it does not allow configuring EMSCRIPTEN_PTHREAD_TRANSFERRED_CANVASES define at all. (In an early version of the code, one would #include "proxy_main.h" to be able to configure what canvas to send over)

Given the current limitation with respect to EGL, I think I would:

  • perform the proxying of EGL functions when OFFSCREENCANVAS_SUPPORT==0,
  • skip the proxying of EGL functions when OFFSCREENCANVAS_SUPPORT==2,
  • give a link time error if EGL is included and OFFSCREENCANVAS_SUPPORT==1 stating that this configuration is currently unsupported (it is not possible currently to use EGL with runtime decision of whether to utilize OffscreenCanvas or not)

@samsinsane
Copy link
Author

it is not possible currently to use EGL with runtime decision of whether to utilize OffscreenCanvas or not

I'm sorry to be a pain but I'm not really following here. If eglCreateContext takes a canvas as a parameter, why can't it check that canvas.controlTransferredOffscreen isn't true/set before proxying the function to the main thread? Unless I'm mistaken, the C++ OpenGL functions do something similar with the context? Would this create other problems that I'm failing to see?

@juj
Copy link
Collaborator

juj commented Jul 12, 2019

why can't it check that canvas.controlTransferredOffscreen isn't true/set before proxying the function to the main thread?

That sounds like a feasible approach. If you want to develop this, it certainly can. It just has not been fixed to do so. There may be more complications, e.g. with eglSwapBuffers();, eglSwapInterval() or other functions, that may need to do similar kinds of checks - depending on how widely you use such functionality.

@samsinsane
Copy link
Author

That sounds like a feasible approach. If you want to develop this, it certainly can. It just has not been fixed to do so.

I already have a workaround in place and I'm in no rush, so I'm happy to work on the proper solution, whatever that happens to be. The only issue is that I'm not entirely sure how to proxy the function manually, it looks like I'd need to add __proxycond and handle that in jsifier.js or maybe add an extra proxy type manual and replace {{ functionIndex }} with proxiedFunctionTable.length?

depending on how widely you use such functionality.

We don't call the EGL functions directly, they're called via SDL2 and these appeared to be the only two functions that caused issues.

@juj
Copy link
Collaborator

juj commented Jul 15, 2019

The only issue is that I'm not entirely sure how to proxy the function manually

You can find an example of how manual proxying is done in the implementation of emscripten_webgl_create_context() function. That function performs the needed checks for main thread vs pthread and regular vs OffscreenCanvas vs Offscreen Framebuffer cases.

Thinking about this - implementing manual proxying logic independently to library_egl.js would be duplicating all that logic for no good. The best way to proceed would be to make eglCreateContext back on top of the HTML5 emscripten_webgl_create_context() function. Then it will get the proxying automatically.

PR #5580 is probably the best way to approach this, i.e. rewrite EGL to C/C++ to back on top of HTML5 API, so it gets multithreading support out of the box.

If resurrecting #5580 is a bit too much, then another way might be to make eglCreateContext directly call emscripten_webgl_create_context() JS function to get the context created, instead of GL.createContext.

@sbc100 sbc100 closed this Jan 30, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

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.

@sbc100 sbc100 reopened this Jan 30, 2020
@sbc100 sbc100 changed the base branch from incoming to master January 30, 2020 19:50
@stale
Copy link

stale bot commented Jan 31, 2021

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.

@stale stale bot added the wontfix label Jan 31, 2021
Base automatically changed from master to main March 8, 2021 23:49
@stale stale bot removed the wontfix label Mar 8, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PROXY_TO_PTHREAD and OFFSCREENCANVAS_SUPPORT issues

4 participants