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

New tests for texImage* with visible webgl canvas #2494

Merged
merged 3 commits into from Aug 29, 2017
Merged

New tests for texImage* with visible webgl canvas #2494

merged 3 commits into from Aug 29, 2017

Conversation

junov
Copy link
Contributor

@junov junov commented Aug 25, 2017

The new tests cover cases when the source image for a texture upload
is a visible WebGL canvas. When the canvas is visible (in the DOM)
it gets presented to screen, which alters internal state, which can be a
source of buggy behavior.

Related chrome bug: crbug.com/759180

@Kangz Kangz requested a review from zhenyao August 25, 2017 21:11
@zhenyao
Copy link
Contributor

zhenyao commented Aug 25, 2017

Is it possible to modify the existing tex-image-and-sub-image-2d-with-webgl-canvas.js and tex-image-and-sub-image-3d-with-webgl-canvas.js to cover the visible webgl canvas case? So they cover both invisible and visible webgl canvas. That could avoid adding a large set of new tests being added.

@kenrussell
Copy link
Member

+1 to Mo's suggestion. Could you please modify the existing tex-image-and-sub-image-*-with-webgl-canvas harnesses to cover this case (run the tests twice, or run them twice for a subset of formats) instead of duplicating both the harnesses and the tests? Thanks.

@junov
Copy link
Contributor Author

junov commented Aug 28, 2017

Done. I modified existing tests instead of creating new ones.

Copy link
Contributor

@zhenyao zhenyao left a comment

Choose a reason for hiding this comment

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

This is much cleaner and more efficient!

var canvas = ctx.canvas;
// Note: We use preserveDrawingBuffer:true to prevent canvas
// visibility from interfering with the tests.
var ctx = wtu.create3DContext(null, { preserveDrawingBuffer:true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we only set preserveDrawingBuffer:true for the test cases where visible: true? This can be easily done by creating another canvas.

Otherwise we lose test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

var ctx = wtu.create3DContext();
// Note: We use preserveDrawingBuffer:true to prevent canvas
// visibility from interfering with the tests.
var ctx = wtu.create3DContext(null, { preserveDrawingBuffer:true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@@ -214,17 +216,29 @@ function generateTest(internalFormat, pixelFormat, pixelType, prologue, resource
function runTest()
{
var ctx = wtu.create3DContext();
var canvas = ctx.canvas;
canvas = ctx.canvas;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but you removed the var here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Fixed.

@zhenyao zhenyao merged commit e208bb9 into KhronosGroup:master Aug 29, 2017
@zhenyao
Copy link
Contributor

zhenyao commented Aug 29, 2017

Thank you!

@junov junov deleted the front-bug branch February 25, 2022 20:06
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.

None yet

3 participants