-
Notifications
You must be signed in to change notification settings - Fork 96
Add texture-component-swizzle tests #4427
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
base: main
Are you sure you want to change the base?
Conversation
d288b5e
to
a6bb46c
Compare
FYI @Kangz suggested in https://dawn-review.googlesource.com/c/dawn/+/249434/comment/235838bf_c9a05964/ to test the various ways to use the view: Load/Fetch, Sampling, Gather, maybe the compare versions etc.
Yes. I initially also tested it in a compute shader but @Kangz strongly suggested [1] in https://dawn-review.googlesource.com/c/dawn/+/249434/comment/10136679_ddbd080c/ to not rely on the accuracy of the division, so I switched to a render pass in https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn/tests/end2end/TextureComponentSwizzleTests.cpp;drc=0416ce44ae6581e6b295b061c816ab5c70bf9cb3
@Kangz explicitly asked to test with
I've opened gpuweb/gpuweb#5252 yesterday to figure this out.
See [1]
This is part of the "Open Questions" section. Without CTS tests to see how this would react, I would allow it for now and then see how it works in practice. We could also raise a validation error if G, B, or A swizzles are non default for depth texture binding. Note that I'll be on vacation next week. |
usage: GPUTextureUsage.COPY_DST | GPUTextureUsage.TEXTURE_BINDING, | ||
}); | ||
t.expectValidationError(() => { | ||
texture.createView({ swizzle: {} }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on gpuweb/gpuweb#5252, we should also try a default swizzle.
I think I'm missing someting.
I don't know what that means. There is no division in my tests. 3> This is part of the "Open Questions" section. Without CTS tests to see how this would react, I would allow it for now and then see how it works in practice. We could also raise a validation error if G, B, or A swizzles are non default for depth texture binding. I think maybe I didn't get my point across @group(0) @binding(0) var t1: texture_2d<f32>; // this as float or unfilterered-float texture
@group(0) @binding(1) var t2: texture_depth; // this is a depth texture
|
Since you use
I assumed you meant Edit: This question was also asked in gpuweb/gpuweb#5217 (comment) |
The division I mentioned @beaufortfrancois was the one done by the fragment shader postfix to turn the We should test the swizzle with the various builtins to make sure that it works in the various APIs. Right now the investigation was on what the APIs expose but they don't exhaustively say what happens with the whole zoo of texture features, so we need to validate that it is consistent across backends before exposing it. |
a6bb46c
to
8ca59a7
Compare
I'm using textureStore to a rgba32float, rgba32uint, or rgba32sint as appropriate. I'm not sure what division we are talking about here.
Ok, but how far do we want to go? The current texture tests take ~1/3 of the CTS time. If I add swizzle tests to all possible combination of stuff that will go up by 2x to 3x My current test already takes 8 minutes on an M1 Mac because of all the combinations.
I suppose we could test the 3 alternates as part of the 24 swizzles (so 27 total) instead of 24x3 which it is now. So that would lower it from 8 minutes to ~3 but then we need to add in a few more things that aren't being testing which will likely bring it back up to at least 5-6 mins. It's currently only testing with 2d textures. It is not testing 1d, 3d, 2d-array, cube, cube-array (so adding those would be ~6x the combos). It's also not testing if mip settings affect it, nor layer settings, nor other sampler settings, nor re-intepretation (view is '-srgb'), and it's also not testing |
Yeah I hear you that it's easy to blow up the test time, maybe we can test all builtins + 27 swizzles on a few representative formats (1/2/4 channels?) and stencil / depth as 2d texture if that's a thing. IDK that all stages matter, but we definitely need fragment to cover all builtins. Do you have other ideas of things that are likely to interact and that need to be tested together? |
8ca59a7
to
d2e1f80
Compare
I think this PR is ready for at least the current version of the spec proposal. It's only slow if |
src/webgpu/api/validation/capability_checks/features/texture_component_swizzle.spec.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,675 @@ | |||
export const description = ` | |||
Operational tests for the 'texture-component-swizzle' feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it live in api/operation
instead of api/validation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, maybe. Preference for location?
src/webgpu/api/operation/features/texture_component_swizzle.spec.ts
?src/webgpu/api/operation/texture/texture_component_swizzle.spec.ts
?- other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong preferences to be honest. Both suggestions look good to me.
I'm looking forward to seeing all these "basic" tests pass on https://dawn-review.googlesource.com/c/dawn/+/255296. Thanks for starting that! |
@greggman FYI https://chromium-review.googlesource.com/c/chromium/src/+/6818591 has just been merged in Chromium. It should be in next Chrome Canary. |
updated the tests for compat |
27727dc
to
14ca630
Compare
@greggman Shall we land these tests to see how different backends react in Chromium or are there more you want to add first? |
I am not sure it's okay to check them in until the spec is approved. As is, they will start failing on other browsers immediately for not validating out non-identity swizzles. |
Throwing out ideas, maybe the tests/spec should have 3 states
I don't know if state 1 is desirable or if we're just going to update the spec to require swizzle validation even if the feature doesn't exist. |
FYI, checking D3D12 NVidia, swizzle doesn't appear to work with stencil textures. Like From WG meeting: We should make the tests skip "must be identity" validation when the feature does not exist until the spec is approved. But we can check in the tests for when the feature exists. So I'll update the tests. |
I've started 256414: [dawn][d3d12] Fix texture component swizzle for stencil formats to address this issue. |
src/webgpu/shader/execution/expression/call/builtin/texture_utils.ts
Outdated
Show resolved
Hide resolved
src/webgpu/api/operation/texture_view/texture_component_swizzle.spec.ts
Outdated
Show resolved
Hide resolved
bea9195
to
5a67cd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
After applying your fix, I get the following errors in https://ci.chromium.org/ui/p/chromium/builders/try/mac-arm64-dawn-rel/19727/test-results on macOS:
for those tests:
|
And I'm getting those on Windows as well (https://ci.chromium.org/ui/p/chromium/builders/try/win-dawn-rel/49419/test-results):
for the following tests:
|
When applying the following fix (as in c805a27), I get rid of errors happening specifically in compatibility mode for depth textures. See results at https://dawn-review.googlesource.com/c/dawn/+/257115?checksResultsFilter=swizzle&tab=checks if (t.isCompatibility && isDepthTextureFormat(format)) {
t.skipIf(
func === 'textureLoad',
'textureLoad can not be used with depth textures in compatibility mode'
);
t.skipIf(
func === 'textureGather' || func === 'textureSample' || func === 'textureSampleLevel',
'texture_depth_xx can not be used with non-comparison samplers in compatibility mode'
);
} |
|
||
TODO: | ||
* test stencil aspect of depth-stencil formats | ||
* test texture_depth_xxx with textureSample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe texture_depth_xx are actually tested with textureSample in this PR. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greggman gentle ping in case it was missed.
Luckily for us, applying the following fix 9a35732 in src/webgpu/shader/execution/expression/call/builtin/texture_utils.spec.ts makes them disappear: - usage: GPUTextureUsage.COPY_DST | GPUTextureUsage.TEXTURE_BINDING,
+ usage:
+ GPUTextureUsage.COPY_DST |
+ GPUTextureUsage.TEXTURE_BINDING |
+ GPUTextureUsage.RENDER_ATTACHMENT, See https://dawn-review.googlesource.com/c/dawn/+/257174?tab=checks EDIT: This is not working for all tests sadly as it now raises the following error:
|
For swizzle, what matters is the usage of the view, not the usage of the texture. They are not necessarily the same because the view's usage can be a subset if |
Oh! I think this is the first case I'm aware of where the I updated the tests. In other news though, my M1 Mac is having issues similar to when I tested the texture builtins. Everything is going fine and then suddenly some tests take forever. As it is, it gets into this state where a few tests timeout. It could be because it's a chromium build? I had one take 18 seconds. But, if I re-run the failing test it runs fast and passes. |
checked, the speed issue is chromium. Canary doesn't have it. |
I have a local change for expected depth to always be |
Uploaded a version that expects |
I removed the 3 stages and am only testing the fragment stage. As for speed, with `debug` false it runs fast enough as is I think. About 25 seconds for all tests on M1 Mac. Note: One issue I ran into is the code that created textures would potentially create them with RENDER_ATTACHMENT if they needed to be rendered to to fill. A texture created with RENDER_ATTACHMENT can't be swizzled. My solution was to change the code that makes the texture such that, if the texture had RENDER_ATTACHMENT added, and it can be copied texture to texture, then make a copy without RENDER_ATTACHMENT Another issue I ran into and I'm not sure what solution is better, is testing depth-stencil formats. The existing code is not designed for that. All the depth-stencil formats are "unencodable". My solution was createTextureWithRandomDataAndGetTexelsForEachAspect that returns 2 sets of TexelViews, one for the depth aspect, another for the stencil aspec. It's possible, maybe we should make those formats encodable as Texels? I didn't chose that becuase depth-stencil has 2 types, f32 for depth, u32 for stencil. This seems like it would cause issues all over the place where code expects just one type. Another issue is depth24plus-stencil, the depth part is undefined. In the code, createTextureWithRandomDataAndGetTexelsForEachAspect on a depth format returns a TexelView with float data, not depth24plus data. I'm not sure all the encoders etc would handle this. So, like I mentioned above, my solution was to return 2 sets of TexelViews, one for the depth aspect and one for the stenci aspect. The code then choose one based on which aspect it's testing.
…omponent_swizzle.spec.ts Co-authored-by: François Beaufort <[email protected]>
Multisampled textures are required to have RENDER_ATTACHMENT usage. This was working by accident as `createTextureWithRandopmDataAndGetTexels` as adding the RENDER_ATTACHMENT usage. That usage addition is removed in this PR so the bug that it was not added here surfaced.
Originally the texture-utils made textures filled with random data based on a texture descriptor. If the texture format was depth or stencil they needed to render to the texture to get the data in since some formats do not support writeTexture nor copyBufferToTexture. To do this they needed to add RENDER_ATTACHMENT. But, texture-component-swizzle requires that RENDER_ATTACHMENT not be set. So, a check was added, if the texture created had texture.usage different than was originally requested then, make a new texture without those added usages and copy the texture into this new texture. 🎉 But, there were a few tests, like textureLoad multisample and texture_utils that happened to be passing by accident. They were testing multisample. Multisampled texture require RENDER_ATTACHMENT. They were not requesting a texture with RENDER_ATTACHMENT but were instead getting one by luck on the old path when RENDER_ATTACHMENT was magically added when needed. With the fix to remove RENDER_ATTACHMENT when it was not requested, these tests started failing. On top of that, the texture-component-swizzle tests test both the depth and stencil aspect of depth-stencil textures. Note that the WGSL builtin texture tests do not (yet?) do this. Adding the path to upload data for both depth AND stencil data broke because the copy above happened in the wrong place. The old broken path was 1. make texture with RENDER_ATTACHMENT 2. use copy texel data via render to fill depth data 3. make copy of texture without RENDER_ATTACHMENT 4. use copy texel data via render to fill stencil data (ERROR!) You can't fill the stencil via render if RENDER_ATTACHMENT is removed. The fix was to refactor so it's now effectively 1. make texture with RENDER_ATTACHMENT 2. use copy texel data via render to fill depth data 3. use copy texel data via render to fill stencil data 4. make copy of texture without RENDER_ATTACHMENT
1. `textureLoad` can not be uses with `texture_depth_2d` 2. `texture_depth_2d` can not be used with non-comparison samplers
The code was checking COPY_SRC when it shouldn't have
This was using texture.usage which was the wrong place to check. Switching to view.usage lets us test multisampled textures.
Note: this changes both the texture-component-swizzle tests and the general WGSL texture builtin tests so that if texture-component-swizzle is enabled, the WGSL texture builtin tests will expect D,0,0,1 instead of D,?,?,?
7e1d5df
to
76292d3
Compare
I've updated https://dawn-review.googlesource.com/c/dawn/+/258594 based on your feedback (thanks!) and tried it locally on my machine. Let me know if that works as expected for you. ![]() |
@greggman And most importantly, does it work on all backends with this PR or do we need to update Dawn? |
It seems like you added tests for this. Are there new validation rules needed if the view is multisampled because some backends fail? |
FYI I've started https://dawn-review.googlesource.com/c/dawn/+/260516?tab=checks but it's not finished yet as I'm writing this. |
These are failing on Mac but if download your CL and run locally everything passes. Are you sure https://dawn-review.googlesource.com/c/dawn/+/260516 incorporates your changes? |
Oh, I guess they are only supposed to pass on M1 Mac, which they are. Intel and AMD Mac are buggy and this has not yet been implemented on other backends. |
I'm unable to make this CTS roll with my CL changes sadly ;( Is this something you can try to see if Macs behave better? |
This was added as RENDER_ATTACHMENT usage is incompatible with swizzle under the current spec. Unfortunately, copying a texture fails for depth textures on Mac Intel (because there is no test). It turns out the current spec only requires the texture view's usage not to have RENDER_ATTACHMENT, not the textures's, so this copy step is not needed. Removing it should let tests run on devices where copy is broken.
a253882
to
e1be9a2
Compare
This is NOT ready to be merged. The extension first needs to be approved and @webgpu/types updated. Then the PR updated to removed the type hacks.
That said, @beaufortfrancois PTAL. What did I miss?
I don't think we want to add swizzle tests to all the texture builtins so I just tested 2,
textureLoad
andtextureSampleLevel
.I only tested in a compute shader. I did not test in a vertex shader nor a fragment shader. Should I?
I only tested a few representative formats
Should swizzle in any form be disallowed without this extension or should it be allowed but only generate a validation error if it's not the identity? The test current requires it to be unset and expects a validation error.
Should we test
textureGather
. That one is hard and I think to do that I'd add it to the texture builtins if we want to test as it would be easier there. Is it supposed to swizzle?Should swizzle generate a validation error on a depth texture binding. A depth texture binding only has 1 channel. (not to be confused with a 2d texture binding that has a depth texture bound. That is tested already in this PR)
Issue: Texture component swizzle 🥤 #4421