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

fix: ensure render pipelines have at least 1 target #5715

Merged

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented May 17, 2024

Connections

None of note.

Description

Important validation is performed on render pipeline configuration (like multisampling) against render targets. However, when no targets are specified, WGPU still compiles pipelines. It misses not just this validation1, but also the case mandated by WebGPU spec., section 10.3.1. (Render Pipeline Creation), which states in the subsection titled validating GPURenderPipelineDescriptor(descriptor, layout, device):

There must exist at least one attachment, either:

Even if the entire space of possible render targets would have rejected the pipeline's configuration, wgpu-core still asks backends to compile pipelines using potentially invalid configuration when no targets are specified. These backends can then misbehave in very different ways, which I've tested with erichdongubler-mozilla#82 for multisampling specifically; examples:

  1. No issues are observed on MacOS CI. Perhaps because it supports the specific offending configurations I've tested?
  2. DX12 rejects the graphics pipeline creation on API call, resulting in an internal error.
  3. Vulkan on Linux simply segfaults. 😬

While there are valid use cases for executing render pipelines with no render targets (DX12 and Vulkan support this, for instance), the WebGPU standard currently does not enable them.

Testing

Tests have been added in this PR; see the changes in tests/ for more details. As described above, these tests were cherry-picked separately and tested in erichdongubler-mozilla#82; this PR definitely fixes them.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Footnotes

  1. While not germane to this bug, specifying a render target is the only way to get a usable render pipeline, so functioning applications are unlikely to do this in shipped code.

@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: validation Issues related to validation, diagnostics, and error handling labels May 17, 2024
@ErichDonGubler ErichDonGubler self-assigned this May 17, 2024
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner May 17, 2024 18:18
@ErichDonGubler ErichDonGubler force-pushed the at-least-1-render-target branch from e12651b to b74508e Compare May 17, 2024 18:25
@jimblandy

This comment was marked as resolved.

jimblandy

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the at-least-1-render-target branch from 8272494 to 65356d0 Compare May 17, 2024 21:15
@ErichDonGubler ErichDonGubler enabled auto-merge (rebase) May 17, 2024 21:15
@ErichDonGubler ErichDonGubler merged commit 18b758e into gfx-rs:trunk May 17, 2024
27 checks passed
@ErichDonGubler ErichDonGubler deleted the at-least-1-render-target branch May 18, 2024 12:11
@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented May 18, 2024

Oof, it appears this change has caught at least one place the CTS is doing something that seems non-standard in webgpu:api,operation,labels:wrappers_do_not_share_labels:* in a CI run I did this to test in Firefox:

  • Source: https://github.com/gpuweb/cts/blob/007e1015875a0a91794edbc14feb3ad5fe60f90b/src/webgpu/api/operation/labels.spec.ts#L264-L270

  • Sample Firefox CI output; since this disappears eventually, I'll inline the output below:

     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO - TEST-UNEXPECTED-FAIL | /_mozilla/webgpu/cts/webgpu/api/operation/labels/cts.https.html?q=webgpu:api,operation,labels:wrappers_do_not_share_labels:* | : - assert_unreached: 
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO -   - EXCEPTION: Error: Unexpected validation error occurred: At least one color attachment or depth-stencil attachment was expected, but no render target for the pipeline was specified.
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO -     TestFailedButDeviceReusable@https://web-platform.test:8443/_mozilla/webgpu/webgpu/util/device_pool.js:22:1
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO -     attemptEndTestScope@https://web-platform.test:8443/_mozilla/webgpu/webgpu/util/device_pool.js:409:13
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO -     
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO -  Reached unreachable code
     [task 2024-05-18T17:04:21.442Z] 17:04:21     INFO - wpt_fn@https://web-platform.test:8443/_mozilla/webgpu/common/runtime/wpt.js:81:25
     [task 2024-05-18T17:04:21.444Z] 17:04:21     INFO - TEST-OK | /_mozilla/webgpu/cts/webgpu/api/operation/labels/cts.https.html?q=webgpu:api,operation,labels:wrappers_do_not_share_labels:* | took 1255ms
    

@ErichDonGubler
Copy link
Member Author

CTS fix for webgpu:api,operation,labels:wrappers_do_not_share_labels:*: gpuweb/cts#3756

@sagudev
Copy link
Contributor

sagudev commented Jun 21, 2024

I think there are more such occurrences in cts: https://github.com/sagudev/servo/actions/runs/9602065915/job/26483847356,

but on my repro even Edge https://sagudev.github.io/briefcase/WGPURenderPipeline does not trigger error, so I thought I interpreted spec wrong.

EDIT: Opened gpuweb/cts#3806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants