Closed
Bug 782860
(webgl-reftests)
Opened 12 years ago
Closed 12 years ago
Begin adding reftests for WebGL
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
(Whiteboard: webgl-conformance webgl-next)
Attachments
(2 files, 5 obsolete files)
35.70 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
17.37 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
Note that these silently succeed when WebGL context creation fails.
Attachment #651975 -
Flags: review?(bjacob)
Assignee | ||
Comment 3•12 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=647fb80704e3
Assignee | ||
Comment 4•12 years ago
|
||
Bug 743105 will probably be useful later. (prefs that only apply to one of the two pages tested)
Assignee | ||
Comment 5•12 years ago
|
||
We should add tests for readback paths when bug 738833 lands.
Assignee | ||
Comment 6•12 years ago
|
||
Try run is running clean on lin64 and osx64, and osx10.7. \o/ Windows and android still to come, though. D:
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
Now with readback tests.
Attachment #651975 -
Attachment is obsolete: true
Attachment #651975 -
Flags: review?(bjacob)
Attachment #652245 -
Flags: review?(bjacob)
Comment 9•12 years ago
|
||
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-
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=85af049ce850 Clean run! \o/ Now to roll it up and submit a patch.
Assignee | ||
Updated•12 years ago
|
Alias: webgl-reftests
Assignee | ||
Comment 15•12 years ago
|
||
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
Assignee | ||
Comment 16•12 years ago
|
||
Here's the updated reftests.
Attachment #654547 -
Attachment is obsolete: true
Attachment #654547 -
Flags: review?(bjacob)
Attachment #656332 -
Flags: review?(bjacob)
Assignee | ||
Comment 17•12 years ago
|
||
And the marking of failing tests. (After application of the two bugs blocking us)
Attachment #656333 -
Flags: review?(bjacob)
Comment 18•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #656333 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 19•12 years ago
|
||
(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'.
Assignee | ||
Comment 20•12 years ago
|
||
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+
Assignee | ||
Comment 21•12 years ago
|
||
Removed trailing whitespace from reftest.js. Carrying forward r+.
Attachment #656333 -
Attachment is obsolete: true
Attachment #659076 -
Flags: review+
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95fb2e6e004 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d82814a300c
Assignee | ||
Comment 23•12 years ago
|
||
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.
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
Comment 24•12 years ago
|
||
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.
Comment 25•12 years ago
|
||
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
Updated•12 years ago
|
Assignee | ||
Comment 26•12 years ago
|
||
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.
status-firefox16:
affected → ---
status-firefox17:
affected → ---
tracking-firefox16:
+ → ---
tracking-firefox17:
+ → ---
Target Milestone: mozilla18 → ---
Assignee | ||
Comment 27•12 years ago
|
||
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.
status-firefox16:
--- → affected
status-firefox17:
--- → affected
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
Target Milestone: --- → mozilla18
Assignee | ||
Comment 28•12 years ago
|
||
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.
Assignee | ||
Comment 29•12 years ago
|
||
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.
Comment 30•12 years ago
|
||
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.
Comment 31•12 years ago
|
||
(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.
Description
•