gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.01k stars 871 forks source link

Validate `MultisampleState::sample_count` against all texture formats before attachment formats in render pipeline creation #5749

Closed ErichDonGubler closed 4 weeks ago

ErichDonGubler commented 2 months ago

Once https://github.com/gpuweb/gpuweb/pull/4681 lands, we should conform to the new spec. revision by validating MultisampleState::sample_count to be supported by the adapter when creating pipelines. When strictly complying with WebGPU, this set of values should only be 1 or 4. On native, this set will be larger.

We will likely need to add a test a la wgpu_test::pipeline::no_targetless_render, as introduced in https://github.com/gfx-rs/wgpu/pull/5715.

Note that we are already validating that multisample counts are compatible with render pipeline attachments, so that the absence of this validation isn't causing us to make invalid requests to wgpu-hal. But adding this validation will certainly improve diagnostics.

ErichDonGubler commented 2 months ago

I'm happy to mentor somebody on this. Marking as a good first issue.

andristarr commented 1 month ago

If this still applies, I would like to start working on this.

So question goes, as I was trying to mimic how NO_TARGETLESS_RENDER is set up, I dont see any references how it is being used. Can you please elaborate how this is being called?

Additionally, can I extract the said sample_count from the TestingContext I didnt find it yet?

I can see ctx.device.limits().max_sampled_textures_per_shader_stage on the testing context for example, but if I understand this correctly, we are not talking about texture sampling, rather multisampling anti aliasing.

ErichDonGubler commented 1 month ago

@andristarr:

If this still applies, I would like to start working on this.

I've assigned the issue to you now. 🙂

… I was trying to mimic how NO_TARGETLESS_RENDER is set up, I dont see any references how it is being used. Can you please elaborate how this is being called?

Yes! Tests are set up in the tests//wgpu-test crate using the #[gpu_test] attribute. NO_TARGETLESS_RENDER is annotated with it, and can be run with cargo nextest run -p wgpu-test -- wgpu_test::pipeline::no_targetless_render.

Additionally, can I extract the said sample_count from the TestingContext I didnt find it yet?

To give more context: This issue is basically a proposed addition to Device::create_render_pipeline. In that method, multisample counts are already validated against the texture format of color and depth-stencil attachments passed into the render pipeline descriptor:

With this issue, we want to validate earlier in that method (likely via TextureFormatFeatureFlags::supported_sample_counts) that the desc.multisample.count is valid for any texture format that might eventually get called by TextureFormatFeatureFlags::sample_count_supported in the later validation. This way, we have a better diagnostic; rather than saying that "this specific format didn't support that count", we might instead tell the user, "no texture format supports this with your current set of adapter features".

ErichDonGubler commented 1 month ago

Thinking more about this: The major difference between the new test I'm thinking of and the NO_TARGETLESS_RENDER test is that the created texture(s) and the render pipeline(s) are intended to be valid or invalid, depending on adapter support. All in all, I see the following cases to exercise in a new test; it should cover:

All of the above cases will also need to vary based on the texture format, particularly ones that can be used as color attachments and depth-stencil attachments.

ErichDonGubler commented 4 weeks ago

Closing in favor of #6039; see also https://github.com/gfx-rs/wgpu/pull/5920#issuecomment-2248515735.