op: Implement 'blend_component_clamp' test in blending.spec.ts #1976

Gyuyoung · 2022-11-08T07:17:47Z

This PR implements the' blend_conmponent_clamp' test to ensure that the blending components
are clamped in the blend equation with the simple component values.

Issue: #1835

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.

github-actions · 2022-11-08T07:26:39Z

Previews, as seen when this build job started (569d3ab):
Run tests | View tsdoc

Gyuyoung · 2022-11-08T07:28:11Z

src/webgpu/api/operation/rendering/blending.spec.ts

@@ -377,8 +377,109 @@ g.test('clamp,blend_factor')
  .unimplemented();

 g.test('clamp,blend_color')
-  .desc('For fixed-point formats, test that the blend color is clamped in the blend equation.')
-  .unimplemented();
+  .desc('For unorm/snorm formats, test that the blend color is clamped in the blend equation.')


I'm still unclear on how to implement this test according to the description. For now, I just implement a simple test to check if the expected result is ok with several 'unorm' formats in order to make a progress on the test. Could you give me a hint or teach me what should we test here?

After reading #1980 I finally realized I think this test is referring to the blend constant. At some point we renamed "blend color" in the spec to "blend constant".

So, I guess what this was saying was, whatever value you pass into setBlendConstant, it's clamped to the range of the format (0..1 or -1..1) before computing the blend results.

However, I think this is slightly wrong.

Here is the relevant bit of the Vulkan spec (which, unlike WebGPU, explains this clamping behavior):

If the color attachment is fixed-point, the components of the source and destination values and blend factors are each clamped to [0,1] or [-1,1] respectively for an unsigned normalized or signed normalized color attachment prior to evaluating the blend operations. If the color attachment is floating-point, no clamping occurs.
https://registry.khronos.org/vulkan/specs/1.2/html/chap27.html#framebuffer-blendoperations

This effectively says three things are clamped. We should update our tests to cover those.

src value (shader output)

src factor

dst factor

(Implicitly, clamping also necessarily occurs on the result, when it is stored into the texture.)

(Note, Vulkan says the dst value is also clamped, but that can't be out of range anyway because it comes directly from the render target.)

Note the src factor and dst factor are clamped after they're computed. Those are where the blend constants come in. In the case of blend factor "constant", it doesn't matter, but in the case of "one-minus-constant", I believe we want the clamping to occur after the "one minus" operation. Example:
If blend constant is 2, and format is snorm (-1..1 range):
Clamping before gives 1 - clamp(2) = 1 - 1 = 0.
Clamping after gives clamp(1 - 2) = clamp(-1) = -1.
Clamping before AND after gives clamp(1 - clamp(2)) = clamp(0) = 0.
I think -1 is correct according to Vulkan (but if we run the test and get a different result on any or all platforms, then we can investigate further).

Additionally, we should test that they're not clamped when using a float format.

Practically speaking, I think these three "clamp" tests should all be written as one test, with the cases being used to cover all of the clamps. Parameters would be, to start:

Format: one unorm, one snorm, one float format.

Blend operation: Just "add" should be fine.

A list of manually written combinations of src value, src factor, dst value, and dst factor. Each of the 4 clamps (src value, src factor, dst factor, final result) needs at least one case to check whether it's happening, but we can enhance beyond that with tests for specific scenarios like the example I gave above.

I appreciate your detailed guidance about this test. Let me request to review this PR as soon as I update the test according to this comment.

Sorry for the late update on this draft. I check the value of the expected color. In unorm format case, the color value should be clamped to 1 if the sum of the srcValue and dstValue exceeds 1. But, in the float format case, it should not be clamped.

But, in the snorm format case, I don't know if the snorm format can be clamped yet. So, I only test the unorm and float formats for now.

@kainino0x I wonder if the current test can be merged with TODOs to make a progress in the test.

Thank you for reviewing this PR. I will prepare more following PRs to remove the TODOs.

kainino0x · 2022-11-10T05:18:54Z

src/webgpu/api/operation/rendering/blending.spec.ts

@@ -377,8 +377,109 @@ g.test('clamp,blend_factor')
  .unimplemented();

 g.test('clamp,blend_color')
-  .desc('For fixed-point formats, test that the blend color is clamped in the blend equation.')
-  .unimplemented();
+  .desc('For unorm/snorm formats, test that the blend color is clamped in the blend equation.')


After reading #1980 I finally realized I think this test is referring to the blend constant. At some point we renamed "blend color" in the spec to "blend constant".

So, I guess what this was saying was, whatever value you pass into setBlendConstant, it's clamped to the range of the format (0..1 or -1..1) before computing the blend results.

However, I think this is slightly wrong.

Here is the relevant bit of the Vulkan spec (which, unlike WebGPU, explains this clamping behavior):

If the color attachment is fixed-point, the components of the source and destination values and blend factors are each clamped to [0,1] or [-1,1] respectively for an unsigned normalized or signed normalized color attachment prior to evaluating the blend operations. If the color attachment is floating-point, no clamping occurs.
https://registry.khronos.org/vulkan/specs/1.2/html/chap27.html#framebuffer-blendoperations

This effectively says three things are clamped. We should update our tests to cover those.

src value (shader output)

src factor

dst factor

(Implicitly, clamping also necessarily occurs on the result, when it is stored into the texture.)

(Note, Vulkan says the dst value is also clamped, but that can't be out of range anyway because it comes directly from the render target.)

Note the src factor and dst factor are clamped after they're computed. Those are where the blend constants come in. In the case of blend factor "constant", it doesn't matter, but in the case of "one-minus-constant", I believe we want the clamping to occur after the "one minus" operation. Example:
If blend constant is 2, and format is snorm (-1..1 range):
Clamping before gives 1 - clamp(2) = 1 - 1 = 0.
Clamping after gives clamp(1 - 2) = clamp(-1) = -1.
Clamping before AND after gives clamp(1 - clamp(2)) = clamp(0) = 0.
I think -1 is correct according to Vulkan (but if we run the test and get a different result on any or all platforms, then we can investigate further).

Additionally, we should test that they're not clamped when using a float format.

Practically speaking, I think these three "clamp" tests should all be written as one test, with the cases being used to cover all of the clamps. Parameters would be, to start:

Format: one unorm, one snorm, one float format.

Blend operation: Just "add" should be fine.

A list of manually written combinations of src value, src factor, dst value, and dst factor. Each of the 4 clamps (src value, src factor, dst factor, final result) needs at least one case to check whether it's happening, but we can enhance beyond that with tests for specific scenarios like the example I gave above.

kainino0x · 2022-11-10T05:20:41Z

src/webgpu/api/operation/rendering/blending.spec.ts

+      dstColor,
+      dstComputeFactor,
+      t.params.operation
+    );


Here, srcColor, srcComputeFactor, and dstComputeFactor need to be clamped according to the range of the format, before computing the blend operation. (either here or inside computeBlendOperation)

github-actions · 2022-11-20T05:26:15Z

Previews, as seen when this build job started (a8e6605):
Run tests | View tsdoc

github-actions · 2022-11-22T01:24:50Z

Previews, as seen when this build job started (0494109):
Run tests | View tsdoc

src/webgpu/api/operation/rendering/color_target_state.spec.ts

This PR implements the' blend_conmponents_clamp' test to ensure that the blending components are clamped in the blend equation with the simple component values. Issue: gpuweb#1835

github-actions · 2022-11-22T06:38:11Z

Previews, as seen when this build job started (f378cbd):
Run tests | View tsdoc

Gyuyoung commented Nov 8, 2022

View reviewed changes

Gyuyoung requested review from austinEng and kainino0x November 8, 2022 07:28

kainino0x mentioned this pull request Nov 10, 2022

op: Implement 'default_blend_color,initial_blend_color' test in blending.spec.ts #1980

Merged

7 tasks

kainino0x reviewed Nov 10, 2022

View reviewed changes

Gyuyoung changed the title ~~op: Implement 'clamp,blend_color' test in blending.spec.ts~~ op: Implement 'blend_component_clamp' test in blending.spec.ts Nov 20, 2022

Gyuyoung force-pushed the clamp-blend-color branch from 569d3ab to a8e6605 Compare November 20, 2022 05:16

Gyuyoung force-pushed the clamp-blend-color branch from a8e6605 to 0494109 Compare November 22, 2022 01:14

Gyuyoung marked this pull request as ready for review November 22, 2022 02:54

kainino0x approved these changes Nov 22, 2022

View reviewed changes

src/webgpu/api/operation/rendering/color_target_state.spec.ts Outdated Show resolved Hide resolved

src/webgpu/api/operation/rendering/color_target_state.spec.ts Outdated Show resolved Hide resolved

op: Implement 'blend_components_clamp' test in blending.spec.ts

f378cbd

This PR implements the' blend_conmponents_clamp' test to ensure that the blending components are clamped in the blend equation with the simple component values. Issue: gpuweb#1835

Gyuyoung force-pushed the clamp-blend-color branch from 0494109 to f378cbd Compare November 22, 2022 06:28

Gyuyoung merged commit 5bc45b0 into gpuweb:main Nov 22, 2022

Gyuyoung deleted the clamp-blend-color branch November 22, 2022 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

op: Implement 'blend_component_clamp' test in blending.spec.ts #1976

op: Implement 'blend_component_clamp' test in blending.spec.ts #1976

Gyuyoung commented Nov 8, 2022 •

edited by kainino0x

Loading

github-actions bot commented Nov 8, 2022

Gyuyoung Nov 8, 2022 •

edited

Loading

kainino0x Nov 10, 2022

Gyuyoung Nov 11, 2022 •

edited

Loading

Gyuyoung Nov 20, 2022

Gyuyoung Nov 22, 2022

Gyuyoung Nov 22, 2022

kainino0x Nov 10, 2022

kainino0x Nov 10, 2022

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

op: Implement 'blend_component_clamp' test in blending.spec.ts #1976

op: Implement 'blend_component_clamp' test in blending.spec.ts #1976

Conversation

Gyuyoung commented Nov 8, 2022 • edited by kainino0x Loading

github-actions bot commented Nov 8, 2022

Gyuyoung Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

kainino0x Nov 10, 2022

Choose a reason for hiding this comment

Gyuyoung Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Gyuyoung Nov 20, 2022

Choose a reason for hiding this comment

Gyuyoung Nov 22, 2022

Choose a reason for hiding this comment

Gyuyoung Nov 22, 2022

Choose a reason for hiding this comment

kainino0x Nov 10, 2022

Choose a reason for hiding this comment

kainino0x Nov 10, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

Gyuyoung commented Nov 8, 2022 •

edited by kainino0x

Loading

Gyuyoung Nov 8, 2022 •

edited

Loading

Gyuyoung Nov 11, 2022 •

edited

Loading