Add validation tests for Multi-Draw Indirect #3962

beaufortfrancois · 2024-09-24T13:16:24Z

This PR adds validation tests for the experimental Multi-Draw Indirect feature that can be tested in Chrome Canary behind the "Unsafe WebGPU support" flag at chrome://flag#enable-unsafe-webgpu

Issue: #3961

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.)
Test have be tested with compatibility mode validation enabled and behave as expected. (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.

Sirtsu55

Apart from the stylistic nitpick, LGTM.

Sirtsu55 · 2024-10-28T16:02:09Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+    u.expandWithParams(() => {
+      return [
+        // In bounds
+        { bufferSize: 4, indirectOffset: 0, _valid: true },


nit: Could we combine these test cases with the ones below as a variable? If I didn't miss anything these cases seem similar with small differences, and combining them would be cleaner/more readable.

+1, if possible store this array as a variable and use it twice. A pattern we use elsewhere to make this work is instead of having bufferSize as a parameter, have something like bufferExtraSize which has negative, zero, and positive cases (4 + (-1) = 3 for indirect, 5 + (-1) = 4 for indexed indirect)

And if there are any cases that are specific to one or the other then you can do:

const kIndirectOffsetOOBCommonCases = [ /* ... */ ] as const; /* ... */ .combineWithParams([ ...kIndirectOffsetOOBCommonCases, /* any additional cases which are not common between the two */ ])

I've hopefully addressed your feedback in latest patch. Please have a look!

@Sirtsu55 @kainino0x Does latest patch look good to you?

Sorry for the late response, but the patch looks good to me!

Sirtsu55 · 2024-10-28T16:22:45Z

FYI @beaufortfrancois , I'm working on #3970 to add more rendering tests. Should we incrementally add rendering tests to this branch or should submit all of the rendering tests at once?

beaufortfrancois · 2024-10-28T16:41:05Z

FYI @beaufortfrancois , I'm working on #3962 to add more rendering tests. Should we incrementally add rendering tests to this branch or should submit all of the rendering tests at once?

I'll let @kainino0x weigh in as I don't have strong preferences.

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

kainino0x · 2024-11-04T21:57:45Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+    u.expandWithParams(() => {
+      return [
+        // In bounds
+        { bufferSize: 4, indirectOffset: 0, _valid: true },


+1, if possible store this array as a variable and use it twice. A pattern we use elsewhere to make this work is instead of having bufferSize as a parameter, have something like bufferExtraSize which has negative, zero, and positive cases (4 + (-1) = 3 for indirect, 5 + (-1) = 4 for indexed indirect)

And if there are any cases that are specific to one or the other then you can do:

const kIndirectOffsetOOBCommonCases = [ /* ... */ ] as const; /* ... */ .combineWithParams([ ...kIndirectOffsetOOBCommonCases, /* any additional cases which are not common between the two */ ])

kainino0x · 2024-11-04T21:58:37Z

FYI @beaufortfrancois , I'm working on #3962 to add more rendering tests. Should we incrementally add rendering tests to this branch or should submit all of the rendering tests at once?

I'll let @kainino0x weigh in as I don't have strong preferences.

IMO land tests as soon as they're landable. It's easy enough to modify tests after they land.

src/webgpu/api/validation/encoding/encoder_open_state.spec.ts

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

kainino0x · 2024-11-19T02:08:23Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+  _valid: boolean;
+}
+
+g.test('indirect_offset_oob')


Test name implies it only tests OOB on the indirectBuffer, but is also doing a bunch of stuff with drawCountBuffer that is hard to separate in my head. Can there be two tests:

indirectBuffer_OOB (just a single boolean param for whether it uses a drawCountBuffer or not)

drawCountBuffer_OOB
?

I've renamed this test as the implementation of two tests would be identical.

I'd prefer if the two were totally separate, as the validation for OOB on each of the two buffers is totally orthogonal. For offsets_alignment it's fine because the list of cases is small, but this list of cases is large and needs to test all the corner cases of two separate checks. Reviewing the list to make sure it covers all of those cases is difficult.

If I'm misunderstanding and they're not orthogonal then this is fine, I just still will need to review the list of cases more closely.

(Also the other tests use kIndirectMultiDrawTestParams so if they were split I think they would have a lot of duplicated control cases; that's not the case here)

beaufortfrancois

Thanks for the thorough review @kainino0x!

src/webgpu/api/validation/encoding/encoder_open_state.spec.ts

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

beaufortfrancois · 2024-11-20T14:49:21Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+  _valid: boolean;
+}
+
+g.test('indirect_offset_oob')


I've renamed this test as the implementation of two tests would be identical.

kainino0x · 2024-11-20T21:34:26Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

@@ -112,7 +128,7 @@ g.test('buffers,device_mismatch')
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
      (encoder as any).multiDrawIndirect(indirectBuffer, 0, 1, drawCountBuffer);
    }
-    validateFinish(!mismatched);
+    validateFinish(!indirectMismatched && (!useDrawCountBuffer || !drawCountMismatched));


Ah I forgot also that drawCountMismatched wouldn't matter if !useDrawCountBuffer. That useless case should ideally be filtered out as well.

kainino0x · 2024-11-20T21:39:07Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+  _valid: boolean;
+}
+
+g.test('indirect_offset_oob')


I'd prefer if the two were totally separate, as the validation for OOB on each of the two buffers is totally orthogonal. For offsets_alignment it's fine because the list of cases is small, but this list of cases is large and needs to test all the corner cases of two separate checks. Reviewing the list to make sure it covers all of those cases is difficult.

If I'm misunderstanding and they're not orthogonal then this is fine, I just still will need to review the list of cases more closely.

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

kainino0x · 2024-11-23T01:24:49Z

src/webgpu/api/validation/encoding/cmds/render/indirect_multi_draw.spec.ts

+  .paramsSubcasesOnly(u =>
+    u.combine('indexed', [true, false] as const).expandWithParams(p => {
+      const indirectParamsSize = p.indexed ? 20 : 16;
+      return [


It is hard to tell reading this list which boundary conditions are checked correctly. Generally we want pairs of control case and error case where the error case is minimally-wrong: just slightly too small, or just slightly too unaligned.

I've pushed commits that refactor this a bit (and address my other comments), PTAL.

kainino0x · 2024-11-23T01:52:41Z

PTAL. If my changes look good (they pass in Chrome), you can go ahead and land the PR.

beaufortfrancois · 2024-11-25T10:27:28Z

I've checked locally and all tests pass in Chrome! Thank you @kainino0x

kainino0x · 2024-11-25T22:29:19Z

Sorry, I wrote that unclearly :)
what I meant was, "They already pass in chrome, so if they look good to you, you can land it"

Add validation tests for Multi-Draw Indirect

6ac148a

beaufortfrancois mentioned this pull request Sep 24, 2024

Add tests for multi-draw-indirect #3961

Open

5 tasks

Merge branch 'main' of github.com:gpuweb/cts into multi-draw-indirect

a48706b

Sirtsu55 reviewed Oct 28, 2024

View reviewed changes

kainino0x reviewed Nov 4, 2024

View reviewed changes

beaufortfrancois added 4 commits November 6, 2024 11:07

Merge branch 'main' of github.com:gpuweb/cts into multi-draw-indirect

92b3578

Address first pass of feedback

8278918

Address second part of feedback

49d5487

Remove types.d.ts

ac9bf64

kainino0x reviewed Nov 19, 2024

View reviewed changes

Address feedback from Kai

fa24773

beaufortfrancois commented Nov 20, 2024

View reviewed changes

kainino0x reviewed Nov 20, 2024

View reviewed changes

beaufortfrancois and others added 3 commits November 21, 2024 11:05

Address feedback | part 2

cf08f24

address comments about indirectBuffer_range

233ef0e

address comments about drawCountBuffer_range

fa5df1e

kainino0x approved these changes Nov 23, 2024

View reviewed changes

Merge branch 'main' into multi-draw-indirect

b738d54

beaufortfrancois merged commit b78115a into main Nov 25, 2024
1 check passed

beaufortfrancois deleted the multi-draw-indirect branch November 25, 2024 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation tests for Multi-Draw Indirect #3962

Add validation tests for Multi-Draw Indirect #3962

beaufortfrancois commented Sep 24, 2024 •

edited by kainino0x

Loading

Sirtsu55 left a comment

Sirtsu55 Oct 28, 2024

kainino0x Nov 4, 2024

beaufortfrancois Nov 8, 2024

beaufortfrancois Nov 14, 2024

Sirtsu55 Nov 14, 2024

Sirtsu55 commented Oct 28, 2024 •

edited

Loading

beaufortfrancois commented Oct 28, 2024

kainino0x Nov 4, 2024

kainino0x commented Nov 4, 2024 •

edited

Loading

kainino0x Nov 19, 2024

beaufortfrancois Nov 20, 2024

kainino0x Nov 20, 2024

kainino0x Nov 20, 2024

beaufortfrancois Nov 21, 2024

beaufortfrancois left a comment

beaufortfrancois Nov 20, 2024

kainino0x Nov 20, 2024

beaufortfrancois Nov 21, 2024

kainino0x Nov 20, 2024

kainino0x Nov 23, 2024

kainino0x commented Nov 23, 2024

beaufortfrancois commented Nov 25, 2024

kainino0x commented Nov 25, 2024

Add validation tests for Multi-Draw Indirect #3962

Add validation tests for Multi-Draw Indirect #3962

Conversation

beaufortfrancois commented Sep 24, 2024 • edited by kainino0x Loading

Sirtsu55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sirtsu55 commented Oct 28, 2024 • edited Loading

beaufortfrancois commented Oct 28, 2024

Choose a reason for hiding this comment

kainino0x commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaufortfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x commented Nov 23, 2024

beaufortfrancois commented Nov 25, 2024

kainino0x commented Nov 25, 2024

beaufortfrancois commented Sep 24, 2024 •

edited by kainino0x

Loading

Sirtsu55 commented Oct 28, 2024 •

edited

Loading

kainino0x commented Nov 4, 2024 •

edited

Loading