Closed Bug 782860 (webgl-reftests) Opened 12 years ago Closed 12 years ago

Begin adding reftests for WebGL

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro ?
Tracking Status
firefox16 - affected
firefox17 - affected
firefox18 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: webgl-conformance webgl-next)

Attachments

(2 files, 5 obsolete files)

While we have extensive conformance tests, they're basically all gl.readPixels based, so we don't actually test whether anything gets to the screen.

At minimum, we should test that (where WebGL works) when we draw with WebGL, the result correctly ends up on the screen on all platforms.

We should also check that pixels are the right color, and that the canvas draws the right way up. (that is, we handle y-flip correctly)
This is very important: we have had a ton of webgl compositing regressions, that that would have caught.
blocking-kilimanjaro: --- → ?
Whiteboard: webgl-conformance webgl-next
Attached patch reftests! \o/ (obsolete) — Splinter Review
Note that these silently succeed when WebGL context creation fails.
Attachment #651975 - Flags: review?(bjacob)
Bug 743105 will probably be useful later. (prefs that only apply to one of the two pages tested)
We should add tests for readback paths when bug 738833 lands.
Try run is running clean on lin64 and osx64, and osx10.7. \o/
Windows and android still to come, though. D:
Hah, everything's clean except android XUL, which is failing with a black canvas.

Looks like WebGL could very well be broken on android XUL.
Now with readback tests.
Attachment #651975 - Attachment is obsolete: true
Attachment #651975 - Flags: review?(bjacob)
Attachment #652245 - Flags: review?(bjacob)
Depends on: 738833
Comment on attachment 652245 [details] [diff] [review]
Add reftests with force-readback pref

Review of attachment 652245 [details] [diff] [review]:
-----------------------------------------------------------------

Just enough nits here to require another round.

::: content/canvas/test/reftest/reftest.list
@@ +6,5 @@
> +== webgl-orientation-test.html wrapper.html?white-top-left.png
> +== webgl-color-test.html       wrapper.html?colors.png
> +pref(webgl.prefer-16bpp,true) == webgl-color-test.html wrapper.html?colors.png
> +
> +# Force native GL:

It would be nice to only check this on Windows, since it should make no difference on other platforms.

@@ +18,5 @@
> +pref(webgl.force-layers-readback,true) == webgl-orientation-test.html wrapper.html?white-top-left.png
> +pref(webgl.force-layers-readback,true) == webgl-color-test.html       wrapper.html?colors.png
> +pref(webgl.force-layers-readback,true) pref(webgl.prefer-16bpp,true) == webgl-color-test.html wrapper.html?colors.png
> +
> +# Readback & native GL:

Same.

::: content/canvas/test/reftest/webgl-clear-test.html
@@ +5,5 @@
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +function InitGL(canvas) {

I don't know that our CamelCase from C++ has to be used in JS code. I would do camelCase. Actually this is said explicitly here:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide
"In JavaScript, functions should use camelCase but should not capitalize the first letter."

@@ +9,5 @@
> +function InitGL(canvas) {
> +	var gl = null;
> +	
> +  try {
> +  	var argDict = {

Indentation?

@@ +15,5 @@
> +  		depth: false,
> +  		stencil: false,
> +  		antialias: false,
> +  		premultipliedAlpha: false,
> +  		preserveDrawingBuffer: false,

These aren't the actual default values. Are these intentional or was this just inadvertently setting all to false?

::: content/canvas/test/reftest/webgl-color-test.html
@@ +16,5 @@
> +  		stencil: false,
> +  		antialias: false,
> +  		premultipliedAlpha: false,
> +  		preserveDrawingBuffer: false,
> +  	};

Same comments as in clear-test.

@@ +24,5 @@
> +  return gl;
> +}
> +
> +function RenderGL(gl) {
> +	gl.enable(gl.SCISSOR_TEST);

indentation again.

::: content/canvas/test/reftest/webgl-disable-test.html
@@ +15,5 @@
> +  		depth: false,
> +  		stencil: false,
> +  		antialias: false,
> +  		premultipliedAlpha: false,
> +  		preserveDrawingBuffer: false,

same comments in this file.
Attachment #652245 - Flags: review?(bjacob) → review-
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Comment on attachment 652245 [details] [diff] [review]
> Add reftests with force-readback pref
> 
> Review of attachment 652245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just enough nits here to require another round.
> 
> ::: content/canvas/test/reftest/reftest.list
> @@ +6,5 @@
> > +== webgl-orientation-test.html wrapper.html?white-top-left.png
> > +== webgl-color-test.html       wrapper.html?colors.png
> > +pref(webgl.prefer-16bpp,true) == webgl-color-test.html wrapper.html?colors.png
> > +
> > +# Force native GL:
> 
> It would be nice to only check this on Windows, since it should make no
> difference on other platforms.
Makes sense.

> @@ +18,5 @@
> > +pref(webgl.force-layers-readback,true) == webgl-orientation-test.html wrapper.html?white-top-left.png
> > +pref(webgl.force-layers-readback,true) == webgl-color-test.html       wrapper.html?colors.png
> > +pref(webgl.force-layers-readback,true) pref(webgl.prefer-16bpp,true) == webgl-color-test.html wrapper.html?colors.png
> > +
> > +# Readback & native GL:
> 
> Same.
> 
> ::: content/canvas/test/reftest/webgl-clear-test.html
> @@ +5,5 @@
> > +
> > +<script type="text/javascript">
> > +"use strict";
> > +
> > +function InitGL(canvas) {
> 
> I don't know that our CamelCase from C++ has to be used in JS code. I would
> do camelCase. Actually this is said explicitly here:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide
> "In JavaScript, functions should use camelCase but should not capitalize the
> first letter."
Ugh, ok. I don't like it, but if that's our style...

> @@ +9,5 @@
> > +function InitGL(canvas) {
> > +	var gl = null;
> > +	
> > +  try {
> > +  	var argDict = {
> 
> Indentation?
Yep.
> 
> @@ +15,5 @@
> > +  		depth: false,
> > +  		stencil: false,
> > +  		antialias: false,
> > +  		premultipliedAlpha: false,
> > +  		preserveDrawingBuffer: false,
> 
> These aren't the actual default values. Are these intentional or was this
> just inadvertently setting all to false?
The defaults (especially premultAlpha and antialias) are not exactly trivial. Really we should test our 'basically everything false' baseline, and then test the matrix of options.
> 
> ::: content/canvas/test/reftest/webgl-color-test.html
> @@ +16,5 @@
> > +  		stencil: false,
> > +  		antialias: false,
> > +  		premultipliedAlpha: false,
> > +  		preserveDrawingBuffer: false,
> > +  	};
> 
> Same comments as in clear-test.
> 
> @@ +24,5 @@
> > +  return gl;
> > +}
> > +
> > +function RenderGL(gl) {
> > +	gl.enable(gl.SCISSOR_TEST);
> 
> indentation again.
> 
> ::: content/canvas/test/reftest/webgl-disable-test.html
> @@ +15,5 @@
> > +  		depth: false,
> > +  		stencil: false,
> > +  		antialias: false,
> > +  		premultipliedAlpha: false,
> > +  		preserveDrawingBuffer: false,
> 
> same comments in this file.
Blocks: 784925
No longer blocks: 784925
Depends on: 784925
Attached patch moar reftests (also nit fixes) (obsolete) — Splinter Review
I also added sanity tests. (append ?nogl (or &nogl) to the url to disable webgl for the test, so we test our fallbacks)

It's up to 136 tests, iirc.
Attachment #652245 - Attachment is obsolete: true
Attachment #654547 - Flags: review?(bjacob)
Try run for this plus a possible fix for the mac issues is running at:
https://tbpl.mozilla.org/?tree=Try&rev=95eba60eaccc
Status: NEW → ASSIGNED
Here's an update with Mac (and likely Linux) passing: (Windows was already passing)
https://tbpl.mozilla.org/?tree=Try&rev=068d90e6efd7

New-fennec looks like it may have a similar problem to what Mac had. XUL-fennec is just broken. (Basically everything fails except disable and &nogl tests)

Mac had two issues. One was the same thing as (the first half of) the toDataURL bug. The second is it seems to not be handling 0.5 color/0.5 alpha/no premult test correctly. I've marked this test as failing on Mac, but need to file a bug to actually fix it.
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=85af049ce850

Clean run! \o/
Now to roll it up and submit a patch.
Alias: webgl-reftests
Hopefully, the following will be green:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=d7f816d67c46

I've marked the two blocking bugs as such, so once we get those in, we can land this. Then it's off to backporting.

I think we should really get these tests all the way to Beta, even if it means marking a bunch of tests as failing. At least we won't regress further than we are.
Depends on: 735932
Attached patch WebGL reftests (obsolete) — Splinter Review
Here's the updated reftests.
Attachment #654547 - Attachment is obsolete: true
Attachment #654547 - Flags: review?(bjacob)
Attachment #656332 - Flags: review?(bjacob)
Attached patch Mark failing tests as failing (obsolete) — Splinter Review
And the marking of failing tests. (After application of the two bugs blocking us)
Attachment #656333 - Flags: review?(bjacob)
Comment on attachment 656332 [details] [diff] [review]
WebGL reftests

Review of attachment 656332 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really like if you could share the repeated code in a common included js file.

::: content/canvas/test/reftest/reftest.list
@@ +11,5 @@
> +# Test: {aa, alpha, preserve, readback} = 16
> +== webgl-clear-test.html?nogl  wrapper.html?green.png
> +
> +== webgl-clear-test.html?__&_____&________  wrapper.html?green.png
> +== webgl-clear-test.html?aa&_____&________  wrapper.html?green.png

Cool__ness.

::: content/canvas/test/reftest/webgl-clear-test.html
@@ +7,5 @@
> +"use strict";
> +
> +function parseArgs() {
> +  var query = window.location.search.substring(1);
> +  

Check trailing whitespace in this file.

::: content/canvas/test/reftest/webgl-color-alpha-test.html
@@ +7,5 @@
> +"use strict";
> +
> +function parseArgs() {
> +  var query = window.location.search.substring(1);
> +  

Here too. (according to splinter, anyway)

Not mentioning it in other files.

::: content/canvas/test/reftest/webgl-color-test.html
@@ +5,5 @@
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +function parseArgs() {

All this generic code is repeated in each test page; it would be nice to share it.
Attachment #656332 - Flags: review?(bjacob) → review+
No longer depends on: 735932
Depends on: 735932
No longer depends on: 735932
Attachment #656333 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #18)
> Comment on attachment 656332 [details] [diff] [review]
> WebGL reftests
> 
> Review of attachment 656332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd really like if you could share the repeated code in a common included js
> file.
> 
> ::: content/canvas/test/reftest/reftest.list
> @@ +11,5 @@
> > +# Test: {aa, alpha, preserve, readback} = 16
> > +== webgl-clear-test.html?nogl  wrapper.html?green.png
> > +
> > +== webgl-clear-test.html?__&_____&________  wrapper.html?green.png
> > +== webgl-clear-test.html?aa&_____&________  wrapper.html?green.png
> 
> Cool__ness.
It really helps line things up in the reftest list. Not the cleanest thing I've ever done, but useful, since we can't really use spaces.
> 
> ::: content/canvas/test/reftest/webgl-clear-test.html
> @@ +7,5 @@
> > +"use strict";
> > +
> > +function parseArgs() {
> > +  var query = window.location.search.substring(1);
> > +  
> 
> Check trailing whitespace in this file.
Will fix.
> 
> ::: content/canvas/test/reftest/webgl-color-alpha-test.html
> @@ +7,5 @@
> > +"use strict";
> > +
> > +function parseArgs() {
> > +  var query = window.location.search.substring(1);
> > +  
> 
> Here too. (according to splinter, anyway)
> 
> Not mentioning it in other files.
> 
> ::: content/canvas/test/reftest/webgl-color-test.html
> @@ +5,5 @@
> > +
> > +<script type="text/javascript">
> > +"use strict";
> > +
> > +function parseArgs() {
> 
> All this generic code is repeated in each test page; it would be nice to
> share it.
Now in 'webgl-utils.js'.
Pulled duplicate JS out into a util file. Removed trailing spaces from all the files. 

I also added a description near the top of each test html file, explaining what each test is, and why we test it.

Carrying forward r+.
Attachment #656332 - Attachment is obsolete: true
Attachment #659074 - Flags: review+
Removed trailing whitespace from reftest.js.
Carrying forward r+.
Attachment #656333 - Attachment is obsolete: true
Attachment #659076 - Flags: review+
We should backport these tests to Aurora and Beta, and mark any tests that fail there accordingly. I think we should only backport the fixes that blocked this bug *after* we get reftests running on Aurora and Beta.
I agree that this should block kilimanjaro. Also agree this should be backported to aurora since there is no code change and this reduces the risk of regression. Backporting to beta would make sense for the same reason, but we normally only backport high priority fixes to beta, and for this reason, regressions are unlikely anyway on beta.
https://hg.mozilla.org/mozilla-central/rev/b95fb2e6e004
https://hg.mozilla.org/mozilla-central/rev/1d82814a300c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 789619
Aurora backport try run:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=cc6a5e28a4c8

It looks like Aurora also has the bug in the OGL Layers readback path, which causes us to assert out on debug builds. I requested tracking status for Aurora 17 in bug 784925.

Beta will not be affected by this, since the readback pref landed in 17.
Target Milestone: mozilla18 → ---
Fun, looks like my page had cached the old tracking values. Sorry about that. I can't set the tracking-* values, but I can reset the status-* values.
Target Milestone: --- → mozilla18
Try for Beta 16:
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=d61bf7fca863

For the moment, I left in the readback tests, even though those don't do anything for 16, since they landed in 17. I'll probably pull them out when putting the backport patch up for review.
Hah, ah, right. I forgot that reftests complain if you try to set a pref that doesn't exist. I'll remove the reftests that use prefs that don't exist on Beta and spin up another Try run.
If there's reason to block the release on these bugs, please re-nominate. Otherwise, we'll just take aurora/beta approval noms when ready.
Depends on: 819846
(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Mac had two issues. 
> [...] The second is it seems to not be handling 0.5 color/0.5
> alpha/no premult test correctly. I've marked this test as failing on Mac,
> but need to file a bug to actually fix it.

bug 819846
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: