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

CTS: Color space conversion operation test for CopyToTexture #1043

Merged
merged 6 commits into from
Mar 15, 2022

Conversation

shaoboyan
Copy link
Contributor

@shaoboyan shaoboyan commented Mar 7, 2022

This PR add cts to test copyExternalImageToTexture() color space
conversion ability.
It creates canvas with color space attr and copy to dst texture in
user defined color space through CopyExternalImageToTexture().

Issue: #913


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.

This PR add cts to test copyExternalImageToTexture() color space
conversion ability.
It creates canvas with color space attr and copy to dst texture in
user defined color space through CopyExternalImageToTexture().

Issue:gpuweb#913
@shaoboyan shaoboyan requested a review from kainino0x March 7, 2022 10:36
@shaoboyan
Copy link
Contributor Author

The compatible Blink CL is here https://chromium-review.googlesource.com/c/chromium/src/+/3501779

Copy link
Collaborator

@austinEng austinEng left a comment

Choose a reason for hiding this comment

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

Some comments - though I did not actually review the test content. I think Kai would be a better reviewer here.

src/webgpu/util/color_space_conversion.ts Outdated Show resolved Hide resolved
src/webgpu/util/color_space_conversion.ts Show resolved Hide resolved
src/webgpu/web_platform/copyToTexture/canvas.spec.ts Outdated Show resolved Hide resolved
@shaoboyan
Copy link
Contributor Author

@austinEng I try to upgrade tsc but got lots of errors. Maybe we should use a seperate PR to do the upgrading.

@shaoboyan shaoboyan requested a review from austinEng March 9, 2022 10:58
@kainino0x
Copy link
Collaborator

I'm working on a tsc upgrade, should be able to get it done today.

@kainino0x kainino0x mentioned this pull request Mar 9, 2022
2 tasks
@kainino0x
Copy link
Collaborator

TypeScript update landed, please merge main into your branch.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM w/ a few nits

src/webgpu/util/math.ts Outdated Show resolved Hide resolved
src/webgpu/util/color_space_conversion.ts Outdated Show resolved Hide resolved
src/webgpu/util/color_space_conversion.ts Outdated Show resolved Hide resolved
src/webgpu/util/color_space_conversion.ts Show resolved Hide resolved
src/webgpu/util/color_space_conversion.ts Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/copyToTexture/canvas.spec.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

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

@shaoboyan
Copy link
Contributor Author

One problem here:
The test runs super slow(I think ~ a minute) with f16 formats.

src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
@kainino0x kainino0x enabled auto-merge (squash) March 10, 2022 20:02
@kainino0x kainino0x disabled auto-merge March 10, 2022 20:02
@kainino0x
Copy link
Collaborator

The test runs super slow(I think ~ a minute) with f16 formats.

Do you know what causes this? If it's slowness in the implementation, we can mark the test as slow in our expectations (hopefully - though we should consider doing so before landing this PR so that the Chrome WebGPU Gardener won't have to debug it).
If it's slowness in the test we should see if we can optimize it. I'm guessing that the float16BitsToFloat32 helper is a bottleneck.

@github-actions
Copy link

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

@shaoboyan
Copy link
Contributor Author

shaoboyan commented Mar 11, 2022

I'm guessing that the float16BitsToFloat32 helper is a bottleneck.

Yes, and the actual and expected arrays needs to reinterpret to Uin16Array first and then call float16ToFloat32 for each entries.
Since the other format works fine, I think this is the bottleneck.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

Apparently I forgot to complete my re-review, sorry.

Since the other format works fine, I think this is the bottleneck.

OK. Since we know it's slow, do you think we could disable the f16 tests in this PR so we can land it, and re-enable them (hopefully with optimizations) in a separate PR?

src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
src/webgpu/util/copy_to_texture.ts Outdated Show resolved Hide resolved
@shaoboyan
Copy link
Contributor Author

OK. Since we know it's slow, do you think we could disable the f16 tests in this PR so we can land it, and re-enable them (hopefully with optimizations) in a separate PR?

The f16 expect logic will cover more than color_space conversion cases. So I try to fix it by comparing the f16bits and handling ~0 values. PTAL again, thanks!

@github-actions
Copy link

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

@shaoboyan
Copy link
Contributor Author

After this fixing, f16 format cases could finished in ~4s on my local machine.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

Nice fix for both the tolerance and the performance! and simpler too

@kainino0x kainino0x merged commit f9158e4 into gpuweb:main Mar 15, 2022
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.

3 participants