gfx-rs
Remove `SupportedLimits.maxInterStageShaderComponents`
This was removed from the WebGPU spec. in https://github.com/gpuweb/gpuweb/pull/4783, and we should, too!
- [x] Remove from WebGPU-on-web backend:
- #6377
- #6380
- [ ] Remove validation and
wgpu_types::SupportedLimits::maxInterStageShaderComponents
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
@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? 🤔
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
@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.
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.
@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
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 https://github.com/gfx-rs/wgpu/compare/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.
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.
We should also implement maxInterStageShaderVariables (listed in https://github.com/gfx-rs/wgpu/issues/2170 & https://github.com/gfx-rs/wgpu/issues/5577) because the reason maxInterStageShaderComponents was removed from the spec is that it effectively worked as 4 * maxInterStageShaderVariables.
I would much prefer that the vendoring is reproducible, barring extenuating circumstances
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.
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.
AFAIK this should indeed not require any changes in web-sys, as this part of the code should never be executed unless demanded. If it turns out that somehow just the existence of a binding causes errors, I would consider this a bug in web-sys (or in the browser engine?).
I also went ahead and updated web-sys to the new draft reflecting the changes that cause this: https://github.com/rustwasm/wasm-bindgen/pull/4145.
@cwfitzgerald, @grovesNL, @daxpedda: I wanted to apologize publicly for my mistaken mental model and changes to our vendored web-sys bindings. It's obvious now that changes to those were not necessary, and I somehow missed it. Thank you for your guidance and patience in driving towards the outcome you're certain is right here.
I am concerned by my mistake for more general reasons now, because I tried fairly hard to ensure that my local changes were actually being reflected in test runs. I'm not sure if this is an error on my part, or if there are issues with the freshness of local changes being presented in cargo xtask run-wasm that ought to be investigated. I'm going to presume that I'm at fault here, for now.
I'm not a maintainer of Wgpu, but I thought it was non-obvious what exactly the issue was. In conclusion, I think this was a good exercise and as a result we have #6381.
Thank you @ErichDonGubler for putting the effort into trying to figure this all out together :heart:!
Good summarize. https://github.com/codeart1st/wgpu-layers/actions/runs/11311932598/job/31458733621 integration test is working again on my side with wgpu git repository linked.
We've now migrated away from maxInterStageShaderComponents to maxInterStageShaderVariables as of https://github.com/gfx-rs/wgpu/pull/8652, so we can close this now. 🙌🏻