Test builtin(position) and interpolation

[greggman]

Interpolation can affect position so these 2 seem like they need to be tested together.

Note: Running the tests on M1 Mac vs AMD Linux, sample and centroid tests get different values and it's not yet clear to me what the correct values are. Rather then continuing I'd like to at least get the working tests in as they are needed to surface known issues in WebGPU implementations.

---

Requirements for PR author:

• All missing test coverage is tracked with "TODO" or .unimplemented().
• New helpers are /** documented */ and new helper files are found in helper_index.txt.
• Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

• Tests are properly located in the test tree.
• Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
• Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
• Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

[lokokung]

Thanks Gregg!! Sorry for the delay. I am going to CC Shrek this PR also and see if he could take a look as well since he also worked with multisample tests before.

[lokokung]

I'm fine to do this in a follow up, but I'm pretty sure there are a couple of places where we are using a compute pass to copy multisampled textures, i.e. https://github.com/gpuweb/cts/blob/main/src/webgpu/gpu_test.ts#L803

I think ideally I could imagine a multisampled texture to just be like multiple TexelViews and we could provide the same expectations helpers like expectTexelViewComparisonIsOkInTexture or something along those lines instead of usning [1]. Then we could add the same type of wrapper over the buffers to create MultisampledTexelViews or something like that which we can just passthrough the buffer we copied here into. Otherwise right now when there is an incorrect value it looks like it would be pretty hard to identify which pixel at what sample.

[greggman]

That's a good idea.

For the copy, checking the code over at gpu_test copySinglePixelTextureToBufferUsingComputePass is (a) only copying a single pixel and (b) isn't recovering f32 values from an rgba8unorm texture. Do we want to try to combine all of those options into a single function or just keep them separate.

I agree that using texelviews to do the comparison would be really good to do. I think I'll mark it as a MAINTANENCE_TODO and try to change it in the next PR.

[lokokung]

I think that we should probably keep the value recovering part separate. (Might be able to do it via something internal to the TexelViews where we could specify how to interpret the binary data.)

But copying the data in/out would be nice to unify.

[greggman]

So for now I changed it. Instead of getting the values from the textures one texture at a time and then combining them after, it reads all 4 textures at once into a single buffer. This results in a buffer that's ready to use as rgba32float which makes it easy to call the TexelView code. So the code now uses TexelView

We can decide whether to switch to reading them as rgba8unorm and keeping them as rgba8unorm in a separate PR, but, at a glance, the TexelView code assumes a single buffer and so, we'd either have to merge/interleave the 4 rgba8uorm buffers into one buffer or else refactor the TexelView code so it doesn't assume one buffer. As it is, it calls "theOneBuffer.subarray" to get data for each texel but that won't work here unles the buffers are interleaved into a single buffer.

There's also the issue that the TexelView code has no concept of samples. So, if it prints an error in texel 11, 5 and your sampleCount is 4 then that really means texel 2, 5, sample index 3,

It might be simple to pass in a sampleCount and have the TexelView code do texelX = x / sampleCount | 0; sampleIndex = texelX % sampleCount but that should probably happen in another PR

[lokokung]

Gotcha! This is great already! I think that copying the data out into a single buffer is fine, we probably just need to introduce the concept of samples to the TexelView. If we default it to 1, and then allow users to specify, we can just assume that the textures will be one after another in the buffer. (As long as we define and use the same copy functions we should be fine.) And yes, absolutely those follow ups should be in a separate PR! Happy holidays!!

[shrekshao]

Sorry I'm ooo. I just defer to your decision

…

On Wed, Dec 20, 2023, 17:55 Greggman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/webgpu/shader/execution/shader_io/fragment_builtins.spec.ts <#3229 (comment)>: > + * an index with the very thing we're trying to test. Similarly for a storage + * texture. Also, using a storage buffer seems to affect certain backends like updated — Reply to this email directly, view it on GitHub <#3229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGMNLDWL2EYUAYCZN44VKTYKOJHXAVCNFSM6AAAAABA33Q7C6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJRHE4TKMJVGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[lokokung]

Hmm, I think that there is a preference to use ULPs if possible. Given the fractional diff you are using I think that it's likely within 1 ULP.