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

Test CopyEI2T and GPUExternalTexture convert video contents to display-p3 color space #3189

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

shaoboyan
Copy link
Contributor

@shaoboyan shaoboyan commented Nov 24, 2023

This PR refactor video related utils and add tests to cover GPUExternalTextureDescriptor:colorSpace = 'display-p3'.

Issue: #2362


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
Copy link
Contributor Author

shaoboyan commented Nov 24, 2023

NOTE: this PR catches implemenation issues in chromium:

  • GPUExternalTexture handles crop+rotate video not correctly.
  • GPUExternalTexture in compute pipeline has big diff with expectation while render pipeline not (Due to case error)
  • display-p3 is not supported (fail a check) in chromium.

This PR refactor video related utils and add tests to cover
GPUExternalTextureDescriptor:colorSpace = 'display-p3'.
@kainino0x
Copy link
Collaborator

Filled in issue #2362 above

@shaoboyan
Copy link
Contributor Author

Almost forget to upload the excel
Display-P3.xlsx
Will detailed the steps in docs/

src/webgpu/web_platform/external_texture/video.spec.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/util.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/util.ts Outdated Show resolved Hide resolved
@shaoboyan shaoboyan changed the title Test GPUExternalTexture sampling in display-p3 color space Test CopyEI2T and GPUExternalTexture convert video contents to display-p3 color space Nov 28, 2023
@shaoboyan shaoboyan requested a review from kainino0x November 28, 2023 06:53
@@ -165,7 +165,7 @@ export function displayP3ToSrgb(pixel: { R: number; G: number; B: number; A: num
pixel.G = rgbVec[1];
pixel.B = rgbVec[2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good fix, I didn't realize this was mutating in place ^_^ We should make this take a readonly input.

I think you meant to remove these lines as well.

I will fix both.

Copy link
Collaborator

@kainino0x kainino0x Nov 28, 2023

Choose a reason for hiding this comment

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

Ugh, sadly web_platform/util.ts is a place where readonly types don't get enforced. Oh well, at least it would have caught these lines.

src/webgpu/web_platform/util.ts Show resolved Hide resolved
@kainino0x kainino0x merged commit 7da5c3c into gpuweb:main Nov 28, 2023
1 check passed
@shaoboyan
Copy link
Contributor Author

shaoboyan commented Nov 29, 2023

Thanks for the fix!

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