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

Add reftests: canvas display after device lost #3669

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

perryuwang
Copy link
Contributor

@perryuwang perryuwang commented Apr 16, 2024

Issue: #3251


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.)
  • Test have be tested with compatibility mode validation enabled and behave as expected. (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.

@perryuwang perryuwang marked this pull request as draft April 16, 2024 23:26
@perryuwang perryuwang force-pushed the canvas_display_after_device_lost branch from 2a3dd68 to 7e01b6b Compare April 17, 2024 07:27
@perryuwang perryuwang marked this pull request as ready for review April 17, 2024 07:27
@kainino0x kainino0x self-requested a review April 24, 2024 00:12
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; 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({
Copy link
Collaborator

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

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@kainino0x
Copy link
Collaborator

Let me remove my change from the end and open a new PR for the extra tests I want to add.

@kainino0x kainino0x force-pushed the canvas_display_after_device_lost branch from 877b760 to 8b15e09 Compare April 25, 2024 22:46
@kainino0x kainino0x enabled auto-merge (squash) April 25, 2024 22:47
@kainino0x kainino0x merged commit 7245ccb into gpuweb:main Apr 25, 2024
1 check passed
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