-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add reftests: canvas display after device lost #3669
Add reftests: canvas display after device lost #3669
Conversation
2a3dd68
to
7e01b6b
Compare
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; please review the commit I pushed on the end!
|
||
const canvas = document.getElementById(canvasId) as HTMLCanvasElement; | ||
const ctx = canvas.getContext('webgpu') as unknown as GPUCanvasContext; | ||
ctx.configure({ |
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 will reconfigure the canvas between device.destroy() and ctx.getCurrentTexture(). This could interact with the test. I pushed a commit which adds 4 more cases that draw without reconfiguring - please review to make sure the expected results look right.
draw('cvs02', 'premultiplied', { reconfigureAfterLost: false, drawAfterLost: false }); | ||
draw('cvs03', 'premultiplied', { reconfigureAfterLost: false, drawAfterLost: true }); | ||
|
||
draw('cvs10', 'opaque', { reconfigureAfterLost: true, drawAfterLost: false }); |
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.
I think cvs10
should display transparent color.
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.
Hm, why is that? I think the spec says, reconfiguring should Replace the drawing buffer
(which sets the drawing buffer to transparent black), then when the canvas is presented, get a copy of the image contents of a context
clears the alpha to 0.
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.
My understanding is that although reconfiguring has set the drawing buffer to black, but drawAfterLoss is false(getCurrentTexture() is not called), canvas is not redrawn and it is no content to presented, so it should be transparent.
For cvs11, because drawAfterLost is true, it should be displayed in black.
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.
According to the spec, configure() also replaces the drawing buffer with a blank image (as do context creation, resize, unconfigure(), getCurrentTexture(), and transferToImageBitmap()).
Which makes me realize there should probably be some tests for unconfigure as well. I was going to add some but didn't think of why it would be useful.
Let me remove my change from the end and open a new PR for the extra tests I want to add. |
877b760
to
8b15e09
Compare
Issue: #3251
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.