Helper for robust texture content checking #1055

kainino0x · 2022-03-11T05:19:00Z

The commits in this PR can be reviewed separately (likely helpful for some of them).

Add new interfaces to texel_data to use for texture checking
Add TexelView helper which exposes an underlying generator or data as texels in various representations
Add textureContentIsOKByT2B helper to compare an "expected" TexelView with a GPUTexture
Update the copyToTexture tests to use these (see Assorted helpers used for texture checking #1068 for some test performance measurements)

Issue: #881

Dry run against Chromium:
https://chromium-review.googlesource.com/c/chromium/src/+/3531550/5
(The only failures are a flake and a simple precision issue, fixed in 759728c...dc82768)

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.

shaoboyan

I've integrated the PR and running it locally. After fixing some bugs, most cases in copy_to_texture works fine, except for ColorSpaceConversion cases, which requires fix asserts in floatAsNormalizedInteger and have some ULP tolerance.

src/webgpu/web_platform/copyToTexture/ImageBitmap.spec.ts

src/webgpu/util/copy_to_texture.ts

src/webgpu/web_platform/copyToTexture/ImageBitmap.spec.ts

src/webgpu/web_platform/copyToTexture/canvas.spec.ts

shaoboyan · 2022-03-17T05:03:02Z

src/webgpu/util/copy_to_texture.ts

+      { texture: dstTextureCopyView.texture },
+      copySize,
+      {
+        maxDiffULPs: 0,


I think it will fail the color space conversion cases.
The error comes from different precision in CPU color space conversion algorithm and the implementation.
Maybe we could provide maxDiffULPs in doTestAndCheckResult and set the value case by case.

For now, I've put a fixed 0.001 threshold on all of these tests. I found that even the 0.0002 from the original code for float16 wasn't enough, and I didn't dig enough to find out why.

I think this might be the color space conversion matrix precision issue. The matrix CPU used is here (https://github.com/gpuweb/cts/blob/main/src/webgpu/util/color_space_conversion.ts#L63). Even the comments said it is copied from ( http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html) but they're different.

The old commit here (f9158e4#diff-0781362fbbe96f1c4922e18e4ccead52e6c15573ed9bfa091c5b039c940e4d08R270) shows that for fp16, if we handle around 0 values, the other values could be in 1 ULP diff

I think this might be the color space conversion matrix precision issue.

I suspected this as well, especially because the G channel in some test cases is way farther off than the others.

That old commit is exactly why I'm confused I needed a larger threshold than before. Will see what I can figure out.

@shaoboyan would you be able to look into the implementation to find out if the relatively-large diff in the G channel is due to a precision issue in the implementation? It's not very important though, so only if you're able to check easily.

src/webgpu/util/copy_to_texture.ts

src/webgpu/web_platform/copyToTexture/canvas.spec.ts

src/webgpu/util/texture/texel_data.ts

shaoboyan · 2022-03-18T08:32:43Z

src/webgpu/util/conversion.ts

-  // Mask only the mantissa, and discard the lower bits.
-  const newMantissa = (bits & 0x7fffff) >> mantissaBitsToDiscard;
-  return (sign << (exponentBits + mantissaBits)) | (newBiasedExp << mantissaBits) | newMantissa;
+  if (newBiasedExp <= 0) {


Shall we throw errors or add assert for this instead of reset it to 0? I'm not sure whether this means bugs in the test cases.

I think the best would be to actually handle conversion from normal numbers to subnormal f16, but for the purpose of ULP checks, we don't care about subnormal - we round all subnormal numbers to 0. And generally GPUs don't guarantee results in the subnormal range anyway for a lot of operations. So I think it's unlikely to be a problem.

The old code asserted, but that assertion was hit because we generated small numbers in color space conversion and then tried to convert to f16.

I also recalled that on chromium implementaion, we always convert the White to D50 config (in this case D65 -> D50). This also introduce some precision difference.

shaoboyan · 2022-03-18T11:07:20Z

src/webgpu/util/copy_to_texture.ts

+      { texture: dstTextureCopyView.texture },
+      copySize,
+      {
+        maxDiffULPs: 0,


The old commit here (f9158e4#diff-0781362fbbe96f1c4922e18e4ccead52e6c15573ed9bfa091c5b039c940e4d08R270) shows that for fp16, if we handle around 0 values, the other values could be in 1 ULP diff

shaoboyan

LGTM! Hope we can get rid of the frac compare for fp16.

src/webgpu/util/color_space_conversion.ts

kainino0x · 2022-03-18T22:59:55Z

I'm going to do a big reshuffle of the commits in this PR so that they're nicely grouped (no separate commits for revisions so it's easier for a new reviewer). For posterity, here's the log at this moment, prior to squashing (note a few fixes went in after this, and #1089 was split out).
87e74a9...62820c8

kainino0x · 2022-03-18T23:25:18Z

@austinEng PTAL
This contains both the helpers and the test updates; if you want, you can skip fully reviewing the test updates and I can land those as a separate PR. (They're still useful as examples of the helper usage, of course.)

src/webgpu/util/texture/texture_ok.spec.ts

src/webgpu/web_platform/copyToTexture/canvas.spec.ts

austinEng

some comments after a first pass - I'll need to look again.
Overall though, this is super cool! great example of how a bunch of small utility functions can be put together to make something quite nuanced and complex

src/webgpu/util/texture/texel_data.ts

src/webgpu/util/texture/texel_view.ts

austinEng · 2022-03-19T05:46:16Z

src/webgpu/util/check_contents.ts

@@ -1,3 +1,7 @@
+// MAINTENANCE_TODO: The "checkThingTrue" naming is confusing; these must be used with `expectOK`
+// or the result is dropped on the floor. Rename these to things like `typedArrayIsOK`(??) to
+// make it clearer. Also, audit to make sure we aren't dropping any on the floor.


if only typescript could error if a result is unused (maybe it's a thing :o)

https://github.com/SonarSource/eslint-plugin-sonarjs/blob/master/docs/rules/no-ignored-return.md possibly?

src/webgpu/util/texture/texture_ok.ts

src/webgpu/util/texture/texture_ok.spec.ts

github-actions · 2022-03-28T20:38:20Z

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

github-actions · 2022-03-28T20:46:28Z

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

kainino0x · 2022-03-29T00:53:11Z

Small unreviewed changes:
759728c...dc82768

refactor colorspace/premul conversion into a standalone helper; fix computation of require{Unpremultiply,Premultiply}Alpha

…r_space_conversion tests work without it

github-actions · 2022-03-29T00:58:05Z

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

github-actions · 2022-03-29T01:01:06Z

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

kainino0x force-pushed the texture-checking branch 5 times, most recently from 20404bf to c08d618 Compare March 16, 2022 01:24

kainino0x mentioned this pull request Mar 16, 2022

Assorted helpers used for texture checking #1068

Merged

7 tasks

kainino0x force-pushed the texture-checking branch 10 times, most recently from df87f13 to f88294b Compare March 17, 2022 03:21

kainino0x requested a review from shaoboyan March 17, 2022 03:21

kainino0x marked this pull request as ready for review March 17, 2022 03:21

shaoboyan reviewed Mar 17, 2022

View reviewed changes

shaoboyan reviewed Mar 18, 2022

View reviewed changes

shaoboyan approved these changes Mar 18, 2022

View reviewed changes

src/webgpu/util/color_space_conversion.ts Show resolved Hide resolved

kainino0x mentioned this pull request Mar 18, 2022

color_space_conversion: use exact rational matrix formulations #1089

Merged

7 tasks

kainino0x force-pushed the texture-checking branch 2 times, most recently from a5f3280 to ae33f39 Compare March 18, 2022 23:23

kainino0x requested a review from austinEng March 18, 2022 23:23

kainino0x commented Mar 19, 2022

View reviewed changes

src/webgpu/util/texture/texture_ok.spec.ts Outdated Show resolved Hide resolved

kainino0x commented Mar 19, 2022

View reviewed changes

src/webgpu/web_platform/copyToTexture/canvas.spec.ts Outdated Show resolved Hide resolved

austinEng reviewed Mar 19, 2022

View reviewed changes

kainino0x force-pushed the texture-checking branch from 474bf97 to 764bcf4 Compare March 22, 2022 01:07

austinEng approved these changes Mar 22, 2022

View reviewed changes

kainino0x added 2 commits March 28, 2022 13:28

floatAsNormalizedInteger: add messages to asserts

a3176ec

float32ToFloatBits: round to zero on underflow, enhance tests

3c41625

kainino0x force-pushed the texture-checking branch 2 times, most recently from b3b56f1 to 29870bb Compare March 28, 2022 20:36

kainino0x force-pushed the texture-checking branch from 29870bb to 759728c Compare March 28, 2022 20:40

kainino0x force-pushed the texture-checking branch from dc82768 to 81ca4e4 Compare March 29, 2022 00:53

kainino0x added 9 commits March 28, 2022 17:54

texel_data: unpackBits, numberToBits, bitsToNumber, bitsToULPFromZero

fe97977

numberToFloatBits

6c8e6b0

add TexelView helper

1b010a3

eventualExpectOK

8e2c5c0

textureContentIsOKByT2B

13ff652

Update image_copy test MAINTENANCE_TODOs

310cc5b

Update copyToTexture tests

4404a71

copyToTexture: factor out + fix color conversion

e84848c

refactor colorspace/premul conversion into a standalone helper; fix computation of require{Unpremultiply,Premultiply}Alpha

copyToTexture tests: remove paintOpaqueRects workaround and make colo…

048dae5

…r_space_conversion tests work without it

kainino0x force-pushed the texture-checking branch from 81ca4e4 to 048dae5 Compare March 29, 2022 00:54

kainino0x enabled auto-merge (rebase) March 29, 2022 00:54

kainino0x merged commit f7bfe9f into gpuweb:main Mar 29, 2022

kainino0x deleted the texture-checking branch March 29, 2022 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helper for robust texture content checking #1055

Helper for robust texture content checking #1055

kainino0x commented Mar 11, 2022 •

edited

Loading

shaoboyan left a comment

shaoboyan Mar 17, 2022

kainino0x Mar 18, 2022

shaoboyan Mar 18, 2022

shaoboyan Mar 18, 2022

kainino0x Mar 18, 2022

kainino0x Mar 29, 2022

shaoboyan Mar 18, 2022

kainino0x Mar 18, 2022

kainino0x Mar 18, 2022

shaoboyan Mar 21, 2022

shaoboyan Mar 18, 2022

shaoboyan left a comment

kainino0x commented Mar 18, 2022 •

edited

Loading

kainino0x commented Mar 18, 2022

austinEng left a comment

austinEng Mar 19, 2022

kainino0x Mar 22, 2022

github-actions bot commented Mar 28, 2022

github-actions bot commented Mar 28, 2022

kainino0x commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

Helper for robust texture content checking #1055

Helper for robust texture content checking #1055

Conversation

kainino0x commented Mar 11, 2022 • edited Loading

shaoboyan 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

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

shaoboyan left a comment

Choose a reason for hiding this comment

kainino0x commented Mar 18, 2022 • edited Loading

kainino0x commented Mar 18, 2022

austinEng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2022

github-actions bot commented Mar 28, 2022

kainino0x commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

kainino0x commented Mar 11, 2022 •

edited

Loading

kainino0x commented Mar 18, 2022 •

edited

Loading