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

WASM crashes #276

Closed
simbleau opened this issue Feb 7, 2023 · 17 comments · Fixed by #312
Closed

WASM crashes #276

simbleau opened this issue Feb 7, 2023 · 17 comments · Fixed by #312

Comments

@simbleau
Copy link
Member

simbleau commented Feb 7, 2023

So I made sure I was on wgpu 15 and using the most recent version.

I cannot seem to make the run_wasm work. On the vello example, I see a problem with adapter.features() from webgpu causing a crash. Doesn't work on canary or nightly.

image

In my personal project I get a bug saying there's no way to allocate a storage buffer. Unrelated, but just letting you know. Perhaps this could be a 2 birds with 1 stone issue.

image

@raphlinus
Copy link
Contributor

That particular error message makes it look like it's getting Limits::downlevel_defaults which will not work, at least without doing considerable additional compatibility work.

@armansito
Copy link
Collaborator

So I made sure I was on wgpu 15 and using the most recent version.

I cannot seem to make the run_wasm work. On the vello example, I see a problem with adapter.features() from webgpu causing a crash. Doesn't work on canary or nightly.

The WASM crash is due to a known issue in wgpu which should get fixed by gfx-rs/wgpu#3428.

@raphlinus
Copy link
Contributor

Vello is (carefully) designed to work with Limits::default, which generally correspond to the minimum guarantee of the WebGPU API. The next step for you is to diagnose why your error message is reporting a limit of 0 for StorageBuffers, as that's not the default value (the default value is 8, and there are other stages that utilize it fully).

@simbleau
Copy link
Member Author

simbleau commented Feb 11, 2023

@raphlinus my code not working was a red herring, the issue may be in yours/wgpu... I noticed the same issue pops up in Vello's run_wasm, after updating wgpu to 0.15.1.

If I use run_wasm --package with_winit:
image

And if I use run_wasm --package with_bevy, I get the familiar shader compute error above.
image

@simbleau
Copy link
Member Author

In the case of with_bevy, it could be related to the Backend: Gl, right? I assume it would say backend: WebGPU. What's weird is that I enabled the flag in canary and nightly and it keeps choosing Gl.

@raphlinus
Copy link
Contributor

Ah yes, it definitely won't work with a GL backend. Perhaps the error messages could be improved in this case. Do the WebGPU samples work for you?

@simbleau
Copy link
Member Author

Those samples work on canary! Huzaahh. I was using the https://wgpu.rs/examples-gpu/ and wondering why I couldn't see anything despite having all of the flags...

So apparently the wasm.is_instance_of is a known bug (per Connor). I'll ignore that.

What about the Compute pipeline? I'm wondering if that's an issue from Bevy, since neither mine or yours works with bevy :)

Is cargo run_wasm supposed to work with_bevy or only with_winit?

@DJMcNab
Copy link
Member

DJMcNab commented Feb 11, 2023

cargo run_wasm is only intended for use with with_winit; in theory, there's no reason it shouldn't work for with_bevy, but it seems that bevy doesn't support WebGPU.

The relevant line is:

https://github.com/bevyengine/bevy/blob/a1e4114ebee5b903a95dc1438b4f0d946ac3c8be/Cargo.toml#L128C2-L131

That is, you cannot use the top-level bevy crate with WebGPU, but using bevy = { package ="bevy_internal", version = "..." } might work. Although that's untested, and I'm unable to support further than that.

The only evidence from the bevy side of WebGPU support is this comment from @mockersf. That seems a bit backwards to me at this point where WebGPU is imminent-ish. If WebGPU turned out to be insufficient for bevy's needs, it seems better to find that out before it was stable in Firefox and Chrome, for example.

@simbleau
Copy link
Member Author

simbleau commented Feb 12, 2023

I see. So maybe we push some documentation at the least saying Vello cannot run with Bevy for a while. I'll go ahead and clean this issue up.

@Niedzwiedzw
Copy link

Niedzwiedzw commented Feb 28, 2023

hey, sorry for gravedigging but winit example also seems to crash in the browser for me. I'm on latest master. Tested on a linux and a windows machine (which I'm pretty sure doesn't matter but yeah)

I'm running cargo run_wasm --package with_winit

@armansito armansito reopened this Feb 28, 2023
@armansito
Copy link
Collaborator

armansito commented Feb 28, 2023

gfx-rs/wgpu#3428 has been merged and bumping vello's dependency on wgpu to 0.15.1 resolves the adapter.features() is not setlike crash for me. I'm seeing the wasm.is_instance_of error on a Debug build but it doesn't appear for me on a release build.

However I'm getting a new crash on Chrome Canary:
Screenshot 2023-02-28 at 11 42 45 AM

On a Debug build it looks like this:
Screenshot 2023-02-28 at 11 52 08 AM

@armansito
Copy link
Collaborator

gfx-rs/wgpu#3430 tracks the wasm.is_instance_of error and that discussion also mentions the getObject(...).requestDevice panic that I'm seeing on release. I suspect this is the root cause of this crash as there are multiple reports of broken wgpu examples showing this panic. This seems to be have been broken since wgpu = 0.15.

It looks like we also have a separate regression on the vello side following #273 which causes the WASM module to not execute the with_winit entry point (the result is that the aforementioned panic disappears but the example shows a blank screen). I'm able to resolve it (i.e. get the WASM panic back) by making the following change to examples/with_winit/Cargo.toml (though I haven't really tested this on Windows):

-[[bin]]
+[[target.'cfg(target_os = "windows")'.bin]]
 # Stop the PDB collision warning on windows
 name = "with_winit_bin"
 path = "src/main.rs"

armansito added a commit to armansito/vello that referenced this issue Mar 12, 2023
Fixed the WASM crashes by reverting to wgpu = 0.14. This surfaced two
more bugs in the repo for which I included fixes. A big issue is
wasm32-unknown-unknown does not support std::time::Instant, so I
implemented a polyfill based on
rust-lang/rust#48564 (comment).
The animated examples, SVG loader, and the frame statistics monitor
won't work in WASM without this. Once there is progress on the wgpu end,
I'll turn that into a proper PR.
armansito added a commit to armansito/vello that referenced this issue Mar 12, 2023
Fixed the WASM crashes by reverting to wgpu = 0.14. This surfaced two
more bugs in the repo for which I included fixes. A big issue is
wasm32-unknown-unknown does not support std::time::Instant, so I
implemented a polyfill based on
rust-lang/rust#48564 (comment).
The animated examples, SVG loader, and the frame statistics monitor
won't work in WASM without this. Once there is progress on the wgpu end,
I'll turn that into a proper PR.

Other issues:
* The WGSL won't compile on native since this version of wgpu/naga
  doesn't support `const`. Chrome Canary in WASM works though.
* There are serious visual artifacts in the examples when run in the
  browser.
@DJMcNab
Copy link
Member

DJMcNab commented Mar 12, 2023

I did consider that fix, but according to the docs,

https://doc.rust-lang.org/cargo/reference/manifest.html

That form isn't supported; it's only allowed for dependencies. I haven't tested it though.

Is there an issue on the cargo-run-wasm side tracking that changing the bin name breaks things, if it does so?

@armansito
Copy link
Collaborator

Is there an issue on the cargo-run-wasm side tracking that changing the bin name breaks things, if it does so?

I think so. Just changing the name field to "with_winit" also works, like so:

 [[bin]]
 # Stop the PDB collision warning on windows
-name = "with_winit_bin"
+name = "with_winit"
 path = "src/main.rs"

I also tried running the example with cargo run_wasm --package with_winit --bin with_winit_bin without changing the Cargo.toml and that also works. I think when determining the wasm binary cargo-run-wasm goes by the package name if --bin hasn't been set.

armansito added a commit to armansito/vello that referenced this issue Mar 12, 2023
Fixed the WASM crashes by reverting to wgpu = 0.14. This surfaced two
more bugs in the repo for which I included fixes. A big issue is
wasm32-unknown-unknown does not support std::time::Instant, so I
implemented a polyfill based on
rust-lang/rust#48564 (comment).
The animated examples, SVG loader, and the frame statistics monitor
won't work in WASM without this. Once there is progress on the wgpu end,
I'll turn that into a proper PR.

Other issues:
* The WGSL won't compile on native since this version of wgpu/naga
  doesn't support `const`. Chrome Canary in WASM works though.
* There are serious visual artifacts in the examples when run in the
  browser.
armansito added a commit to armansito/vello that referenced this issue Mar 12, 2023
Fixed the WASM crashes by reverting to wgpu = 0.14. This surfaced two
more bugs in the repo for which I included fixes. A big issue is
wasm32-unknown-unknown does not support std::time::Instant, so I
implemented a polyfill based on
rust-lang/rust#48564 (comment).
The animated examples, SVG loader, and the frame statistics monitor
won't work in WASM without this. Once there is progress on the wgpu end,
I'll turn that into a proper PR.

Also currently need to run the example with
`cargo run_wasm --package with_winit --bin with_winit_bin`

Other issues:
* The WGSL won't compile on native since this version of wgpu/naga
  doesn't support `const`. Chrome Canary in WASM works though.
* There are serious visual artifacts in the examples when run in the
  browser.
@simbleau
Copy link
Member Author

simbleau commented Mar 12, 2023

A little lost here with the details. Why does changing the package name fix the WASM build, and what does "PDB" stand for? Thank you.

Also - I don't like the idea of going back to wgpu 0.14, as I saw in your experimentation branch. Is that the intention? I would suggest keeping it broken until it's stable on 0.15 and submitting PRs to move us forwards, not backwards.

@armansito
Copy link
Collaborator

armansito commented Mar 12, 2023

A little lost here with the details. Why does changing the package name fix the WASM build, and what does "PDB" stand for? Thank you.

@DJMcNab can speak to this but I believe on Windows he had to deduplicate the binary name from the package name. This affects WASM since it looks like cargo-run-wasm doesn't automatically handle the case where the package name and the binary name differ and requires you to invoke it explicitly with cargo run_wasm --package with_winit --bin with_winit_bin. Just running cargo run_wasm --package with_winit fails silently.

Also - I don't like the idea of going back to wgpu 0.14, as I saw in your experimentation branch. Is that the intention? I would suggest keeping it broken until it's stable on 0.15 and submitting PRs to move us forwards, not backwards.

I agree, I don't support reverting Vello to wgpu 0.14 either. I created the dev branch as an experiment for demos in the short term.

To summarize, I have identified 3 overall issues blocking WASM support:

  1. Dynamic dispatch layer breaks on web gfx-rs/wgpu#3430 causing a panic.
  2. cargo-run-wasm needs a workaround
  3. std::time::Instant is not supported when targeting wasm32-unknown-unknown (see the discussion on std::time::SystemTime::now() causes panic on wasm32-unknown-unknown. rust-lang/rust#48564). This is needed for the animated scenes and some recent work on performance profiling to work. I was able to work around this on my hack branch by implementing a polyfill for timer utilities as suggested in this comment. I can clean this up and turn it into a proper PR once the wgpu upstream issues are resolved.

@simbleau
Copy link
Member Author

simbleau commented Mar 12, 2023

Thanks for the context!

regarding #3, there's a crate called instant which is a partial replacement for Instant which should have identical behavior on all platforms, including wasm32. I actually use it a lot since I do quite a bit of Web Rust. Bear in mind, some things on instant::Instant are missing from std::time::Instant, because they are impossible on wasm32. Check if all the features you need are there, and use that.

https://crates.io/crates/instant

armansito added a commit that referenced this issue Apr 25, 2023
Fix WASM build

- Rolled wgpu to 0.16.
- Incorprated the instant crate in lieu of std::time::Instant which works on WASM and native builds.
- Fixed the issue with window scaling by setting the canvas size based on winit Window dimensions.
- Fixed a division-by-zero issue in path_coarse_full

This resolves #276
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 a pull request may close this issue.

5 participants