-
Notifications
You must be signed in to change notification settings - Fork 86
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
CopyEI2T: Cases To Cover Rotated Images #4111
Conversation
This PR created images with rotation metadata. It tests using CopyEI2T to upload image and imageBitmap with 'from-image' orientation flag. Issue:#4180
Seems I cannot add reviewers in right side bar. Explicit @kainino0x @beaufortfrancois for reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test code lgtm, but a few comments
src/webgpu/web_platform/util.ts
Outdated
xhr.open('GET', url); | ||
xhr.responseType = 'blob'; | ||
xhr.onerror = function () { | ||
reject(new Error('Network error.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kainino0x I'm not sure what's the best way to handle this reject case. Do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, we want any error case to fail the test.
However I don't think XHR should ever be need, rather than fetch()
+ .blob()
:
https://developer.mozilla.org/en-US/docs/Web/API/Response/blob
(I'm surprised to see we don't already have any code that uses fetch()
, but it seems the only place we use a still-image resource uses HTMLImageElement
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a pile of small things
src/webgpu/web_platform/util.ts
Outdated
xhr.open('GET', url); | ||
xhr.responseType = 'blob'; | ||
xhr.onerror = function () { | ||
reject(new Error('Network error.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, we want any error case to fail the test.
However I don't think XHR should ever be need, rather than fetch()
+ .blob()
:
https://developer.mozilla.org/en-US/docs/Web/API/Response/blob
(I'm surprised to see we don't already have any code that uses fetch()
, but it seems the only place we use a still-image resource uses HTMLImageElement
.)
Co-authored-by: Kai Ninomiya <[email protected]>
Co-authored-by: Kai Ninomiya <[email protected]>
Co-authored-by: Kai Ninomiya <[email protected]>
Co-authored-by: Kai Ninomiya <[email protected]>
FYI I sent your new account an invite to gpuweb (https://github.com/orgs/gpuweb/people) so you should be able to land changes once you accept it. |
@kainino0x Thanks!
Where can I find the invitation so that I could accept it? |
@shaoboyan091 it should be sent to the primary email address on your github account. I think this link should work: |
This PR created images with rotation metadata. It tests using CopyEI2T to upload image and imageBitmap with 'from-image' orientation flag.
Issue: #4108
Requirements for PR author:
.unimplemented()
./** documented */
and new helper files are found inhelper_index.txt
.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.