Skip to content

[wasm64] Fix window system/graphics JS libraries #20276

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

Merged
merged 9 commits into from
Sep 20, 2023

Conversation

jspanchu
Copy link
Contributor

@jspanchu jspanchu commented Sep 16, 2023

Closes #20274

These changes got some amount of webgl, webgpu and event processing for wasm64 working in Chromium Version 119.0.6011.0 (Developer Build) custom (64-bit). Had to enable experimental Webassembly flags.

@jspanchu
Copy link
Contributor Author

jspanchu commented Sep 16, 2023

@sbc100 not sure if you've any open PR that includes these changes. I found these commits fix VTK rendering for wasm64. Please let me know if any of these changes already exist in an open PR.

@jspanchu jspanchu changed the title [wasm64] Fix window system/graphics code [wasm64] Fix window system/graphics JS libraries Sep 16, 2023
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

lgtm % a couple of comments.

Normally I would want to see some new tests passing under wasm64 but maybe thats not currently an easy option?

@jspanchu
Copy link
Contributor Author

I think a new workflow is needed for wasm64 on browsers? It appears that the workflows which run wasm64 tests only install node canary.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 18, 2023

I think a new workflow is needed for wasm64 on browsers? It appears that the workflows which run wasm64 tests only install node canary.

We have the @also_with_wasm64 in test_browser.py. Perhaps add that decorator to one of the existing graphics tests?

@jspanchu
Copy link
Contributor Author

Thanks! I think the SDL2 tests will fail because the port will not compile. Let me try that for unit tests which use low level html5 api.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 18, 2023

Thanks! I think the SDL2 tests will fail because the port will not compile. Let me try that for unit tests which use low level html5 api.

sdl1 tests should maybe work too?

@jspanchu
Copy link
Contributor Author

@sbc100 I'm making progress with wasm64 tests inside test_browser.py. So far the sdl1 and egl tests are looking good. Turns out I also have to modify library_webgl.js to use * where necessary. Working on it. Is there any extra step to enable the wasm64 browser tests in the GitHub workflow?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 19, 2023

@sbc100 I'm making progress with wasm64 tests inside test_browser.py. So far the sdl1 and egl tests are looking good. Turns out I also have to modify library_webgl.js to use * where necessary. Working on it. Is there any extra step to enable the wasm64 browser tests in the GitHub workflow?

Just to add @also_with_wasm64 to a test or two. Just adding one or two would be enough to land this change I think

@jspanchu jspanchu force-pushed the fix_sigs_for_wasm64 branch 3 times, most recently from c902bf9 to 8e65611 Compare September 19, 2023 12:23
@jspanchu
Copy link
Contributor Author

@sbc100 I've enabled wasm64 for a few simple tests. Just checked the "Run tests" section in circleci and it looks like both firefox and chrome ran the newly enabled wasm64 tests successfully.

There is more work to do for webgl and proxied contexts. We don't use proxied contexts in our library, but I'd love to explore that. I think it's a good idea to make those changes in follow up PRs.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm with the final comment fixed.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 19, 2023
The webidl `long` type is defined to be a 32-bit type:

https://webidl.spec.whatwg.org/#idl-long
https://webidl.spec.whatwg.org/#idl-unsigned-long

Using `long` on the native side works find for wasm32 but breaks
under wasm64 where `long` is 64-bit.  Using `int` instead means the
type is the same under wasm32 and wasm64 which more accurately
represents the html5/webidl type.

See emscripten-core#20276
@sbc100
Copy link
Collaborator

sbc100 commented Sep 19, 2023

Actually, I think we should fix the types using html5.h.. which should allow you revert certain parts of this change: #20290

@jspanchu
Copy link
Contributor Author

That is much better imo. I'll revert those after that is merged.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 19, 2023
The webidl `long` type is defined to be a 32-bit type:

https://webidl.spec.whatwg.org/#idl-long
https://webidl.spec.whatwg.org/#idl-unsigned-long

Using `long` on the native side works find for wasm32 but breaks
under wasm64 where `long` is 64-bit.  Using `int` instead means the
type is the same under wasm32 and wasm64 which more accurately
represents the html5/webidl type.

See emscripten-core#20276
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 19, 2023
The webidl `long` type is defined to be a 32-bit type:

https://webidl.spec.whatwg.org/#idl-long
https://webidl.spec.whatwg.org/#idl-unsigned-long

Using `long` on the native side works find for wasm32 but breaks
under wasm64 where `long` is 64-bit.  Using `int` instead means the
type is the same under wasm32 and wasm64 which more accurately
represents the html5/webidl type.

See emscripten-core#20276
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 19, 2023
The webidl `long` type is defined to be a 32-bit type:

https://webidl.spec.whatwg.org/#idl-long
https://webidl.spec.whatwg.org/#idl-unsigned-long

Using `long` on the native side works find for wasm32 but breaks
under wasm64 where `long` is 64-bit.  Using `int` instead means the
type is the same under wasm32 and wasm64 which more accurately
represents the html5/webidl type.

See emscripten-core#20276
sbc100 added a commit that referenced this pull request Sep 19, 2023
The webidl `long` type is defined to be a 32-bit type:

https://webidl.spec.whatwg.org/#idl-long
https://webidl.spec.whatwg.org/#idl-unsigned-long

Using `long` on the native side works find for wasm32 but breaks
under wasm64 where `long` is 64-bit.  Using `int` instead means the
type is the same under wasm32 and wasm64 which more accurately
represents the html5/webidl type.

See #20276
@jspanchu
Copy link
Contributor Author

Upon rebasing against main, my canvas was flipped and mouse/keyboard weren't being recognized in browser with wasm64. Seems like the contents of the structs in generated_struct_info64.json were copied verbatim from generated_struct_info32.json? After regenerating with --wasm64, it works as expected!

@jspanchu jspanchu force-pushed the fix_sigs_for_wasm64 branch 2 times, most recently from 0c1db5f to e880b4d Compare September 19, 2023 23:56
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks! Almost ready to land.

@jspanchu jspanchu force-pushed the fix_sigs_for_wasm64 branch 3 times, most recently from 5af5ee6 to 75a9a6c Compare September 20, 2023 11:55
@sbc100
Copy link
Collaborator

sbc100 commented Sep 20, 2023

I just fixed the code size failures on main.. can you try rebasing/merging one more time?

@sbc100 sbc100 enabled auto-merge (squash) September 20, 2023 22:09
@sbc100 sbc100 merged commit cb40f59 into emscripten-core:main Sep 20, 2023
@jspanchu jspanchu deleted the fix_sigs_for_wasm64 branch September 21, 2023 01:23
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.

wasm64: emscripten library functions which take pointers throw TypeError: Cannot convert number to BigInt
2 participants