Skip to content
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

Use internal name for Module['canvas'] #21536

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 14, 2024

Instead of referring to this via Module['canvas'] everywhere we can use the normal makeModuleReceiveExpr method.

@sbc100 sbc100 force-pushed the canvas branch 2 times, most recently from bc32251 to 4b2b61c Compare March 15, 2024 17:55
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 15, 2024

We don't have any code side tests that so canvas stuff but I measured the code size saving locally:

Before:

$ ./emcc -O2 --closure=1 ./test/test_html5_core.c
$ wc -c a.out.js
30372 a.out.js

After:

$ ./emcc -O2 --closure=1 ./test/test_html5_core.c
$ wc -c a.out.js
30286 a.out.js

@sbc100 sbc100 requested a review from kripken March 15, 2024 18:04
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 15, 2024

In terms of what to call this I decided on mainCanvas since just canvas would make it very hard to grep for and more likely to conflict with other locals and globals. I'm open to other ideas for the name though.

Instead of referring to this via `Module['canvas']` everywhere we can
use the normal `makeModuleReceiveExpr` method.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this seems like a good idea! Two concerns though:

  1. I seem to see missing deps in the first few files. Is it handled in a way I am missing?
  2. How is existing user code handled? That is, I see some test changes here - will existing user code with Module['canvas'] break in an unclear way? I think if there is a good error in debug builds it might be ok.

@@ -591,7 +591,7 @@ var emscriptenCpuProfiler = {
},

detectWebGLContext() {
if (Module['canvas']?.GLctxObject?.GLctx) return Module['canvas'].GLctxObject.GLctx;
if (mainCanvas?.GLctxObject?.GLctx) return mainCanvas.GLctxObject.GLctx;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not have a __deps for it?

@@ -358,7 +358,7 @@ var LibraryEGL = {
EGL.contextAttributes.majorVersion = glesContextVersion - 1; // WebGL 1 is GLES 2, WebGL2 is GLES3
EGL.contextAttributes.minorVersion = 0;

EGL.context = GL.createContext(Module['canvas'], EGL.contextAttributes);
EGL.context = GL.createContext(mainCanvas, EGL.contextAttributes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__deps in this file?

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 19, 2024

In general this seems like a good idea! Two concerns though:

  1. I seem to see missing deps in the first few files. Is it handled in a way I am missing?

There could be some missing ones. I'll take a look. As with all the deps we are kind of reliant here on out test coverage (and review process!) to ensure correctness.

  1. How is existing user code handled? That is, I see some test changes here - will existing user code with Module['canvas'] break in an unclear way? I think if there is a good error in debug builds it might be ok.

As for existing code that does Module['canvas'] I think it will just work since IIUC the main/only way to inject/create a canvas in emscripten is to set Module['canvas'] on the incoming module object as the sample html file does.

This means that in practice Module['canvas'] will always (unless I'm misunderstanding) be equivalent to write mainCanvas since the latter is initialized as a reference to the former. I suppose there could be programs out there that update or set Module['canvas'] after initialization? But I'm not aware of that, and our test suite doesn't cover it. Do you know if that is a thing?

@sbc100 sbc100 enabled auto-merge (squash) March 19, 2024 07:34
@juj
Copy link
Collaborator

juj commented Mar 19, 2024

One thing I worry here is that previously the binding to Module['canvas'] would have been continuous/combinational, but after this change, the assignment to mainCanvas is now latched. I.e. if user changes the value of Module['canvas'] after the moment it is latched (which happens in this PR at Module instantiation time?), then after that if user sets Module['canvas'] = xxx' manually afterwards, such code will start to silently fail.

Also, I see previously we have had a couple of places that would assign back to Module['canvas'] = to set a update a changed canvas object. After this PR the code is changed to update mainCanvas = variable instead. So if there might exist any code that would be reading Module['canvas'], they would be seeing stale data?

Module['canvas'] = GL.offscreenCanvases[data.moduleCanvasId].offscreenCanvas;
Module['canvas'].id = data.moduleCanvasId;
if (!mainCanvas && data.moduleCanvasId && GL.offscreenCanvases[data.moduleCanvasId]) {
mainCanvas = GL.offscreenCanvases[data.moduleCanvasId].offscreenCanvas;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is one such place where if user had existing code that accessed Module['canvas'], that code would no longer work?

@juj
Copy link
Collaborator

juj commented Mar 19, 2024

Fwiw I've considered the whole variable Module['canvas'] to be on the legacy track - only the old library_glfw/sdl/egl/browser files use it. Generally when creating WebGL or WebGPU contexts, the old Module['canvas'] abstraction is not used/no longer relevant, and the CSS query selector based access mechanism is used instead.

Although now reading this PR I see that when using OffscreenCanvas, there is that access to Module['canvas'] that still remains - OffscreenCanvas is not legacy, so maybe there might be something to refactor there at some point, to make it disconnected from Module['canvas'].

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 19, 2024

One thing I worry here is that previously the binding to Module['canvas'] would have been continuous/combinational, but after this change, the assignment to mainCanvas is now latched. I.e. if user changes the value of Module['canvas'] after the moment it is latched (which happens in this PR at Module instantiation time?), then after that if user sets Module['canvas'] = xxx' manually afterwards, such code will start to silently fail.

Also, I see previously we have had a couple of places that would assign back to Module['canvas'] = to set a update a changed canvas object. After this PR the code is changed to update mainCanvas = variable instead. So if there might exist any code that would be reading Module['canvas'], they would be seeing stale data?

Yes, I think you are correct in your assessment here.

I guess I will hold off on this change for now then.

Marked as draft.

@sbc100 sbc100 marked this pull request as draft March 19, 2024 21:39
auto-merge was automatically disabled March 19, 2024 21:39

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants