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

Compat: refactor state_tracking test for 0 frag buffers. #4112

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

greggman
Copy link
Contributor

@greggman greggman commented Dec 19, 2024

This is a first attempt. Feel free to push back and/or give ideas.

The original tests use 2 read-only-storage buffers and 1 read-write storage buffer. Each has a single i32 in it and generally they subtract the 2nd from the first out = a - b

Storage buffers in the fragment stage might not exist on some compat devices so the question is how to work around that and still test.

This solution is to add subcases, storage and uniform. The storage case is unchanged. The compute pass case will run in compat always. The render pass and render bundle cases only run in compat if the device supports storage buffers in the fragment stage.

The uniform cases use 2 uniform buffers and render to a single pixel r32sint texture. They then copy that texture to the out buffer that the original test was checking. This path needs no storage buffers in the fragment shader and so always runs.

This works but it's effectively only checking 2 bindings, not 3. So, the question is, should I add 3rd buffer and change the algo to out = a - b - c etc.... so that we can shuffle more bindings? Or is this good enough? Or should I do something completely different. Maybe 3rd should be a 1x1 px texture_2d<i32> so it's a different type?

Also note: the last test 'compatible_pipelines' is unchanged and so only runs the comput pass unless the device supports storage buffers in fragment shaders.

I didn't update it yet because for it to work requires either (a) two render passes to render to 2 different render targets. Or it needs some viewport settings to render to 2 different pixels in the same target. Or something..., all of which seem like the might require some big refactors. In the createEncoder infra in gpu_test.ts or else they'd just have to do their own thing entirely.

Maybe that change doesn't need to happen in this PR but ideas are welcome.

@greggman greggman requested a review from toji December 19, 2024 09:32
@greggman greggman requested a review from shrekshao January 8, 2025 21:26
@shrekshao
Copy link
Contributor

This works but it's effectively only checking 2 bindings, not 3. So, the question is, should I add 3rd buffer and change the algo to out = a - b - c etc...

It's only testing 2 bindings because the uniform path changes the output buffer to render target? Am I understanding correctly?

But this only applies to the newly added uniform path. The storage path still checks 3 bindings. Test coverage is not decreasing. I think it's ... fine?

the last test 'compatible_pipelines' is unchanged and so only runs the compute pass unless the device supports storage buffers in fragment shaders.

If we really want the coverage for no storage in fragment shaders devices, is having a seperate g.test('compatible_pipelines,uniform') test serving the purpose and easier to implement? (just use uniform buffer as input, render to target as output)

(I'm good with not adding one)

Copy link
Contributor

@shrekshao shrekshao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This is a first attempt. Feel free to push back and/or
give ideas.

The original tests use 2 read-only-storage buffers and
1 read-write storage buffer. Each has a single i32 in it
and generally they substract the first 2 from the 2nd.

Storage buffers in the fragment stage might not exist on
some compat devices so the question is how to work around
that and still test.

This solution is to add subcases, `storage` and `uniform`.
The `storage` case is unchanged. The compute pass case
will run in compat always. The render pass and render
bundle cases only run in compat if the device supports
storage buffers in the fragment stage.

The uniform cases use 2 uniform buffers and render to
a single pixel r32sint texture. They then copy that
texture to the `out` buffer that the original test was
checking. This path needs no storage buffers in the
fragment shader and so always runs.

This works but it's effectively only checking 2 bindings,
not 3. So, the question is, should I add 3rd buffer and
change the algo to out =  a - b - c etc.... so that we can
shuffle more bindings? Or is this good enough? Or should
I do something completely different.

Also note: the last test 'compatible_pipelines' is unchagned
and so only runs the comput pass unless the device supports
storage buffers in fragment shaders.

I didn't update it yet because for it to work requires
either (a) two render passes to render to 2 different
render targets. Or it needs some viewport settings to
render to 2 different pixels in the same target. Or something...,
all of which seem like the might require some big refactors.
In the `createEncoder` infra in gpu_test.ts or else they'd
just have to do their own thing entirely.

Maybe that change doesn't need to happen in this PR but
ideas are welcome.
@greggman greggman enabled auto-merge (squash) January 9, 2025 21:18
@greggman
Copy link
Contributor Author

greggman commented Jan 9, 2025

This works but it's effectively only checking 2 bindings, not 3. So, the question is, should I add 3rd buffer and change the algo to out = a - b - c etc...

It's only testing 2 bindings because the uniform path changes the output buffer to render target? Am I understanding correctly?

correct

But this only applies to the newly added uniform path. The storage path still checks 3 bindings. Test coverage is not decreasing. I think it's ... fine?

yea, no coverage is lost

the last test 'compatible_pipelines' is unchanged and so only runs the compute pass unless the device supports storage buffers in fragment shaders.

If we really want the coverage for no storage in fragment shaders devices, is having a seperate g.test('compatible_pipelines,uniform') test serving the purpose and easier to implement? (just use uniform buffer as input, render to target as output)

(I'm good with not adding one)

agreed, we can add one later if needed.

@greggman greggman merged commit 3fae862 into gpuweb:main Jan 9, 2025
1 check passed
@greggman greggman deleted the state-tracking branch January 9, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants