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

Remove SupportedLimits.maxInterStageShaderComponents #6290

Open
ErichDonGubler opened this issue Sep 17, 2024 · 14 comments · May be fixed by #6380
Open

Remove SupportedLimits.maxInterStageShaderComponents #6290

ErichDonGubler opened this issue Sep 17, 2024 · 14 comments · May be fixed by #6380
Assignees
Labels
area: ecosystem Help the connected projects grow and prosper area: validation Issues related to validation, diagnostics, and error handling

Comments

@ErichDonGubler
Copy link
Member

This was removed from the WebGPU spec. in gpuweb/gpuweb#4783, and we should, too!

@ErichDonGubler ErichDonGubler added area: validation Issues related to validation, diagnostics, and error handling area: ecosystem Help the connected projects grow and prosper labels Sep 17, 2024
@ErichDonGubler
Copy link
Member Author

Firefox downstream tracking: bug 1919404

@codeart1st
Copy link

My test already failing in firefox: https://github.com/codeart1st/wgpu-layers/actions/runs/11090086957/job/30811980649

---- render_test::empty output ----
    info output:
        surface: Surface { context: ContextWebGpu { type: "Web" }, _handle_source: "None", id: ObjectId { id: Some(1), global_id: Some(1) }, data: Any { .. }, config: Mutex { data: None } }
        adapter: Adapter { context: ContextWebGpu { type: "Web" }, id: ObjectId { id: Some(1), global_id: Some(2) }, data: Any { .. } }
    
    error output:
        panicked at /home/runner/work/wgpu-layers/wgpu-layers/src/renderer.rs:77:8:
        Device can't be created.: RequestDeviceError { inner: WebGpu(JsValue(OperationError: requestDevice: Limit 'maxInterStageShaderComponents' not recognized.
        __wbg_get_imports/imports.wbg.__wbg_requestDevice_1c8e4f0fe8729328/<@http://127.0.0.1:45803/wasm-bindgen-test:1401:37

@ErichDonGubler
Copy link
Member Author

@codeart1st: I've removed the exposed limit in Firefox using the above link, yes! That's odd, though, because if we were failing in Firefox, then surely that means we were also failing in Chrome, no? 🤔

@codeart1st
Copy link

The integrationtest is using https://download-installer.cdn.mozilla.net/pub/firefox/nightly/latest-mozilla-central/firefox-132.0a1.en-US.linux-x86_64.tar.bz2 - chrome is a different story and currently not possible to run headless integrationtests on it under linux, so I don't know for now 😄
The last working run was 4 days ago: https://github.com/codeart1st/wgpu-layers/actions/runs/11060882848/job/30732372163

@ErichDonGubler
Copy link
Member Author

@codeart1st: Did some investigating! The failure you're reporting is because the web-sys layer that wgpu's WebGPU backend is based on is unconditionally trying to read the maxInterStageShaderComponents limit, and Firefox doesn't offer it anymore (which I'm guilty of).

This fails in Firefox and not Chrome because the Chromium issue for this is still in-progress (https://issues.chromium.org/issues/364338810). I expect that it will be committed to Chrome Canary in the near future, at which point similar failures will happen for the same downstream.

I have confirmed locally that removing the field from our vendored copy of web-sys resolves this issue. I'm not sure if our CI permits this yet, but I'll file a PR and find out (CC @daxpedda, @Wumpf, @cwfitzgerald). It'll take some time to engage downstream and propagate such a change, but I believe doing so will be a good stopgap to include in next week's WGPU release before we try to catch up with the new [Throws] annotations that web-sys upstream has added to its WebIDL (CC @daxpedda).

@jimblandy: I'm forgoing the consideration of a rollback in Firefox above, but LMK if you think that's a better course of action; Firefox's Bugzilla has had at least one entry filed for this, too.

@cwfitzgerald
Copy link
Member

This shouldn't require any webidl changes? https://github.com/gfx-rs/wgpu/blob/trunk/wgpu%2Fsrc%2Fbackend%2Fwebgpu.rs#L818 just remove this line.

@ErichDonGubler
Copy link
Member Author

@cwfitzgerald: I tried that with cargo xtask run-wasm myself, and unfortunately, just removing that line does not fix the issue. If you examine the error stack, you'll notice that it's an error returned from the bindings layer generated by wasm-bindgen. The only way to avoid the error is by avoiding the bindings code generated for the maxInterStageComponents field on the web-sys side.

@ErichDonGubler
Copy link
Member Author

Firefox downstream tracking issue: Bug 1922884 - webgpu demo doesnt run on Nightly, runs on Chrome

@grovesNL
Copy link
Collaborator

grovesNL commented Oct 7, 2024

@ErichDonGubler

WebGPU backend is based on is unconditionally trying to read the maxInterStageShaderComponents limit

This didn't seem right to me (the fields shouldn't be read unconditionally, just queried on-demand) so I took a quick look. I think trunk...grovesNL:wgpu:inter-stage is all that is needed, since that's used to pass the limits to the browser.

Updating the vendored WebIDL is fine too, but just wanted to make a note in case it wasn't clear what was going on here.

@jimblandy
Copy link
Member

jimblandy commented Oct 7, 2024

If @grovesNL is right, we should file a follow-up issue for that.

We also need a follow-up to remove the validation that enforces the limit. It should be sufficient to delete it from the wgpu_types struct, along with any entailed changes.

@teoxoy
Copy link
Member

teoxoy commented Oct 7, 2024

We should also implement maxInterStageShaderVariables (listed in #2170 & #5577) because the reason maxInterStageShaderComponents was removed from the spec is that it effectively worked as 4 * maxInterStageShaderVariables.

@cwfitzgerald
Copy link
Member

I would much prefer that the vendoring is reproducible, barring extenuating circumstances

@jimblandy
Copy link
Member

Okay - it sounds like I approved this PR without appreciating the issues and alternatives involved. I'm sorry about that. Since this was a problem that Firefox precipitated by updating our IDL, I thought we should act quickly.

My reasoning was, there must be some reason we bother to vendor the WebIDL, beyond simple stability, since we could get that with a git dependency. So I figured we must be making local adjustments already, and this PR wouldn't be a problem.

The next time we re-vendor web-sys, I'm expecting maxInterStageShaderComponents will be gone anyway, so the re-vendoring shouldn't cause problems.

@jimblandy
Copy link
Member

jimblandy commented Oct 8, 2024

This didn't seem right to me (the fields shouldn't be read unconditionally, just queried on-demand) so I took a quick look. I think trunk...grovesNL:wgpu:inter-stage is all that is needed, since that's used to pass the limits to the browser.

I wanted to understand exactly why this works.

Without changing more than just device creation, as we did in #6377, there are still going to be ways to run into the fact that Firefox doesn't support this limit any more. For example, Rust code can still call Adapter::limits, which is still going to end up in map_wgt_limits trying to fetch max_inter_stage_shader_components, which is going to throw a JS error. The change suggested by @grovesNL isn't going to fix this.

But the specific errors that people are running into when creating devices are thrown because the requiredLimits record that wgpu::Adapter::request_device ends up passing to the browser's GPUAdapter.requestDevice method includes a key named "maxInterStageShaderComponents, which Firefox rejects per spec. This is what @grovesNL's change fixes.

jimblandy added a commit to jimblandy/wgpu that referenced this issue Oct 8, 2024
Revert the changes to the vendored copy of web-sys's WebGPU bindings
made in gfx-rs#6377. The only purpose of vendoring is to pin down our WebGPU
JS bindings, not to allow local changes. And as it turns out, removing
the `max_inter_stage_shader_components` accessor isn't necessary in
order to fix gfx-rs#6290.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ecosystem Help the connected projects grow and prosper area: validation Issues related to validation, diagnostics, and error handling
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

6 participants