When trying to create a too large canvas, don't throw, instead just use a smaller drawing buffer

RESOLVED FIXED in mozilla10

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bjacob, Assigned: jgilbert)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 5 obsolete attachments)

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.
Assignee

Updated

8 years ago
Blocks: 615976
No longer blocks: webgl-conformance
Assignee

Updated

8 years ago
Assignee

Updated

8 years ago
Depends on: 688112
Assignee

Updated

8 years ago
Assignee: nobody → jgilbert
Assignee

Updated

8 years ago
Blocks: 688112
No longer depends on: 688112
Assignee

Comment 1

8 years ago
This change basically guarantees that GLContext will not be changed if a resize attempt fails.
Assignee

Updated

8 years ago
Blocks: 685229
Assignee

Comment 2

8 years ago
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
Assignee

Comment 3

8 years ago
Yeah, that didn't work so well. 
Updated:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=089d1b13072f
Assignee

Updated

8 years ago
Status: NEW → ASSIGNED
Assignee

Comment 4

8 years ago
Current patch file.
Attachment #564827 - Attachment is obsolete: true
Attachment #566099 - Flags: review?(bjacob)
Assignee

Updated

8 years ago
Attachment #566099 - Flags: review?(bjacob) → feedback?(bjacob)
Assignee

Comment 5

8 years ago
Ran into a problem on OS X.
Assignee

Comment 6

8 years ago
Fixed CGL impl.
Attachment #566099 - Attachment is obsolete: true
Attachment #566099 - Flags: feedback?(bjacob)
Attachment #566404 - Flags: review?(bjacob)
Assignee

Comment 8

8 years ago
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
Assignee

Comment 9

8 years ago
Fixed errant tab issues and added further commenting.
Attachment #566404 - Attachment is obsolete: true
Attachment #566404 - Flags: review?(bjacob)
Attachment #567141 - Flags: review?(bjacob)
Assignee

Comment 10

8 years ago
Predicates too-verbose debug printf on mDebugMode.
Attachment #567141 - Attachment is obsolete: true
Attachment #567141 - Flags: review?(bjacob)
Attachment #567147 - Flags: review?(bjacob)
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?
Attachment #567147 - Flags: review?(bjacob) → review+
Assignee

Comment 12

8 years ago
> 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.
Assignee

Comment 13

8 years ago
Carrying forward r+ from bjacob after PR_TRUE/FALSE conversion.
Attachment #567147 - Attachment is obsolete: true
Attachment #567820 - Flags: review+
Re-landed with proper commit message (sorry, should have checked that)
http://hg.mozilla.org/integration/mozilla-inbound/rev/716915ed489c
https://hg.mozilla.org/mozilla-central/rev/716915ed489c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee

Updated

8 years ago
Duplicate of this bug: 688112
Assignee

Updated

8 years ago
Duplicate of this bug: 685229
Assignee

Updated

8 years ago
Duplicate of this bug: 723072
You need to log in before you can comment on or make changes to this bug.