-
Notifications
You must be signed in to change notification settings - Fork 670
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
Initial conformance tests for WEBGL_shader_pixel_local_storage #3530
Conversation
@kenrussell @lexaknyazev please take a look. Ken, this collides with your previous PR. I can rebase after you land. I think this, combined with the ANGLE tests, ought to be enough coverage to land a draft implementation in Chrome! |
Done |
Closed by accident. This is rebased and ready for review. |
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.
Please add a test that asserts correct PLS behavior on page composition (wtu.waitForComposite
) events without an app explicitly ending PLS. It should include app-observable state of indexed color masks (if they could be implicitly changed) and implicit clears of the default FBO.
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
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.
Great work Chris on these tests! A few small revisions requested.
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
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.
Please add a test that asserts correct PLS behavior on page composition (wtu.waitForComposite) events without an app explicitly ending PLS.
This is done, but commented for now because it breaks Chrome. Good test idea!
It should include app-observable state of indexed color masks (if they could be implicitly changed) and implicit clears of the default FBO.
ANGLE has a comprehensive test for indexed color mask. I think I can bring in all of the ANGLE validation tests with this PR. Did you have anything more specific in mind?
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
Browsers may change color masks to enable all channels while doing an implicit clear; alpha mask may be forced to be false for contexts created with opaque default framebuffer (via The test should ensure that browser's and PLS' implicit color mask updates work correctly together and the last user-set state is restored. |
Got it, thanks! https://chromium-review.googlesource.com/c/angle/angle/+/4355032 would enable us to easily handle the clear after compositing when thte app leaves PLS enabled. Do you think these spec changes look OK? |
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
sdk/tests/conformance2/extensions/webgl-shader-pixel-local-storage.html
Outdated
Show resolved
Hide resolved
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 had some local changes I had forgotten to commit before I did my last update 🤦.
Let's hold off until I finish building Chrome and can verify these tests are all correct and passing, but I think we should be just about ready.
I think all the comments are addressed now, and these tests are passing locally for me in Chrome. @kenrussell and @lexaknyazev, does this change look ready to land now? |
|
||
const wtu = WebGLTestUtils; | ||
const canvas = document.getElementById("canvas"); | ||
const gl = wtu.create3DContext(canvas, {alpha: false}, 2); |
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.
Could the rendering test be repeated for the default (alpha: true
) case? No-alpha composition may use different code paths.
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.
Would you just create two different canvases on the page? Or use an offscreen canvas for one of them?
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.
Create a regular canvas from JS, get an alpha-enabled context, run tests, delete that canvas, repeat with alpha disabled.
It's a good idea to check PLS operations with OffscreenCanvas
but it would better be a separate test.
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.
Done! Can't quite delete it yet though because it needs to go to sleep for compositing.
This test also captures PLS being active on two contexts at once.
OffscreenCanvas might be a good candidate for some of the future tests I can import from ANGLE.
Does this look good to land now?
273b022
to
7f89fc4
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.
Thanks for revising this Chris. It looks like there were a couple of small mistakes in the extension.xml. Did you try running "make" in your extensions/ directory locally?
Creates a set of tests to check the WebGL behavior not found in the ANGLE_shader_pixel_local_storage specification, as well as providing some scaffolding to verify the ANGLE extension is properly hooked up. If these all look good, we can start porting tests from the ANGLE extension next.
I think this should all be good now. I ran |
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.
Thanks for the updates - looks good!
Await the results of asynchronous functions, and explicitly call finishTest() at the end of runTest(). Follow-on to KhronosGroup#3530 .
Await the results of asynchronous functions, and explicitly call finishTest() at the end of runTest(). Follow-on to #3530 .
Timeouts were accidentally introduced in the last commit by failing to do this. Follow-on to KhronosGroup#3562 and KhronosGroup#3530.
* Call finishTest() when PLS isn't supported. Timeouts were accidentally introduced in the last commit by failing to do this. Follow-on to #3562 and #3530. Fixed bugs caught by @lexaknyazev in code review.
Creates a set of tests that verify the non-normative WebGL behavior, as well as providing some scaffolding to verify the ANGLE extension is properly hooked up.
If these all look good, we can start porting tests from the ANGLE extension next.