Last Comment Bug 681791 - When trying to create a too large canvas, don't throw, instead just use a smaller drawing buffer
: When trying to create a too large canvas, don't throw, instead just use a sma...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
https://cvs.khronos.org/svn/repos/reg...
: 685229 688112 (view as bug list)
Depends on:
Blocks: 615976 webgl-conformance 685229 688112
  Show dependency treegraph
 
Reported: 2011-08-24 15:11 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-02-01 08:09 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixes GLContext::ResizeOffscreen leaving a mess on failure (13.33 KB, patch)
2011-10-05 06:49 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Review
Guarantees that GLContext::ResizeOffscreen is harmless if it fails (14.58 KB, patch)
2011-10-10 18:05 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Review
Guarantees that GLContext::ResizeOffscreen is harmless if it fails (18.27 KB, patch)
2011-10-11 17:28 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Review
Guarantees that GLContext::ResizeOffscreen is harmless if it fails (20.00 KB, patch)
2011-10-14 11:22 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Review
Guarantees that GLContext::ResizeOffscreen is harmless if it fails (19.78 KB, patch)
2011-10-14 11:38 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Review
Guarantees that GLContext::ResizeOffscreen is harmless if it fails (19.35 KB, patch)
2011-10-18 11:55 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-24 15:11:47 PDT
We currently file a few WebGL conformance tests, like

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/canvas/drawingbuffer-static-canvas-test.html

Because we throw on canvas.width = something_huge when SetDimensions fails.

Chrome doesn't throw there.

Chrome also won't use a drawingbuffer larger than the max texture size or 4k, whichever is smaller.

We need to fail less on resizing-canvases-to-large-sizes by using a smaller drawingbuffer if needed.
Comment 1 Jeff Gilbert [:jgilbert] 2011-10-05 06:49:14 PDT
Created attachment 564827 [details] [diff] [review]
Fixes GLContext::ResizeOffscreen leaving a mess on failure

This change basically guarantees that GLContext will not be changed if a resize attempt fails.
Comment 2 Jeff Gilbert [:jgilbert] 2011-10-10 14:29:55 PDT
It didn't occur to me before that the arguably time-saving initialize once approach was incompatible with soft-failing resizing.
Updated try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=322fe6cae175
Comment 3 Jeff Gilbert [:jgilbert] 2011-10-10 16:41:04 PDT
Yeah, that didn't work so well. 
Updated:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=089d1b13072f
Comment 4 Jeff Gilbert [:jgilbert] 2011-10-10 18:05:36 PDT
Created attachment 566099 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

Current patch file.
Comment 5 Jeff Gilbert [:jgilbert] 2011-10-11 15:22:21 PDT
Ran into a problem on OS X.
Comment 6 Jeff Gilbert [:jgilbert] 2011-10-11 17:28:30 PDT
Created attachment 566404 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

Fixed CGL impl.
Comment 7 Jeff Gilbert [:jgilbert] 2011-10-11 17:33:11 PDT
New try run:
https://tbpl.mozilla.org/?tree=Try&rev=5a9f76410202
Comment 8 Jeff Gilbert [:jgilbert] 2011-10-12 16:33:12 PDT
For some reason my last changes reset. Maybe I reset my patch file somehow.
Updated try with CGL fix:
https://tbpl.mozilla.org/?tree=Try&rev=ff8c9387530d
Comment 9 Jeff Gilbert [:jgilbert] 2011-10-14 11:22:03 PDT
Created attachment 567141 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

Fixed errant tab issues and added further commenting.
Comment 10 Jeff Gilbert [:jgilbert] 2011-10-14 11:38:43 PDT
Created attachment 567147 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

Predicates too-verbose debug printf on mDebugMode.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2011-10-16 19:56:47 PDT
Comment on attachment 567147 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

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

This looks all good!

Just one remark about something to keep in mind. The ClearSafely() call is mega-super-important for security: we must prevent WebGL scripts from reading back uninitialized video memory from a newly resized canvas. In your AA patches, the FBO becomes two separate FBOs, right. So will we need two ClearSafely() calls there?
Comment 12 Jeff Gilbert [:jgilbert] 2011-10-17 03:36:15 PDT
> Just one remark about something to keep in mind. The ClearSafely() call is
> mega-super-important for security: we must prevent WebGL scripts from
> reading back uninitialized video memory from a newly resized canvas. In your
> AA patches, the FBO becomes two separate FBOs, right. So will we need two
> ClearSafely() calls there?

I will triple-check, but we shouldn't need to. After clearing the draw buffer, the read buffer /may/ be uninitialized, but before any read call is possible, we blit over the cleared draw buffer, clearing the read buffer. Basically, it's only possible to access if you can read it without using GL commands to do so. We could just deliberately blit over the cleared draw buffer, as that should be minimal cost compared to assembling a resized buffer, but it should not be necessary. If it were necessary, there would be other problems with our WebGL restrictions anyways.
Comment 13 Jeff Gilbert [:jgilbert] 2011-10-18 11:55:00 PDT
Created attachment 567820 [details] [diff] [review]
Guarantees that GLContext::ResizeOffscreen is harmless if it fails

Carrying forward r+ from bjacob after PR_TRUE/FALSE conversion.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-10-19 12:12:49 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8583488f5241
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-10-19 12:47:01 PDT
Re-landed with proper commit message (sorry, should have checked that)
http://hg.mozilla.org/integration/mozilla-inbound/rev/716915ed489c
Comment 16 Marco Bonardo [::mak] 2011-10-20 03:03:01 PDT
https://hg.mozilla.org/mozilla-central/rev/716915ed489c
Comment 17 Jeff Gilbert [:jgilbert] 2011-10-20 21:14:04 PDT
*** Bug 688112 has been marked as a duplicate of this bug. ***
Comment 18 Jeff Gilbert [:jgilbert] 2011-12-01 04:05:50 PST
*** Bug 685229 has been marked as a duplicate of this bug. ***
Comment 19 Jeff Gilbert [:jgilbert] 2012-02-01 08:09:16 PST
*** Bug 723072 has been marked as a duplicate of this bug. ***

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