847714 - WebGL fails to fall back to reasonable drawing buffer sizes on too-large resize

Status: RESOLVED DUPLICATE of bug 996266

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 721001 [details] [diff] [review]
patch

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

Have enough nits to require a second round.

Also, please comment on how this ends up being correctly reflected in drawingbufferWidth/Height.

::: content/canvas/src/WebGLContext.cpp
@@ +386,5 @@
> +            if (gl->ResizeOffscreen(size))
> +                break;
> +
> +            width /= 2;
> +            height /= 2;

That's where it sucks that these are signed... but whatever. Not perf critical.

@@ +387,5 @@
> +                break;
> +
> +            width /= 2;
> +            height /= 2;
> +        }

So what if width or height is zero now? Don't you want to fail early here?

@@ +545,5 @@
> +                break;
> +
> +            width /= 2;
> +            height /= 2;
> +        }

Really sucks to have to write this 3 times! Please write a little utility func? Doesn't have to be in GLContext, can be static right here.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Oh, also: why don't we clamp the drawingbuffer width/height to 4K like Chrome does?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Oh, also: why don't we clamp the drawingbuffer width/height to 4K like
> Chrome does?

4K things are not guaranteed to work. This is a simple and compliant way to guarantee it'll work. It'll also preserve aspect ratio, which, though not necessary spec-wise, would be nice.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Sure, I meant doing that _in addition to_ doing this.

Putting an upper limit on texture sizes might help with testcases trying absurdly large sizes (to prevent further OOMs), but I don't know that there is any in the test suite. I agree to not do anything in that direction until concrete problems arise.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 721001 [details] [diff] [review]
> patch
> 
> Review of attachment 721001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have enough nits to require a second round.
> 
> Also, please comment on how this ends up being correctly reflected in
> drawingbufferWidth/Height.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +386,5 @@
> > +            if (gl->ResizeOffscreen(size))
> > +                break;
> > +
> > +            width /= 2;
> > +            height /= 2;
> 
> That's where it sucks that these are signed... but whatever. Not perf
> critical.
> 
> @@ +387,5 @@
> > +                break;
> > +
> > +            width /= 2;
> > +            height /= 2;
> > +        }
> 
> So what if width or height is zero now? Don't you want to fail early here?
> 
> @@ +545,5 @@
> > +                break;
> > +
> > +            width /= 2;
> > +            height /= 2;
> > +        }
> 
> Really sucks to have to write this 3 times! Please write a little utility
> func? Doesn't have to be in GLContext, can be static right here.

I don't see a good way to do this. We could easily combine the last two with lambdas, but I don't see a really clean way to combine them otherwise.

Also, clamping the drawingbuffer unconditionally to 4k will cause issues down the road. (FSAA on a 4k screen, for one) If we can get away with it, we should just try to do what the user asks for.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> > Really sucks to have to write this 3 times! Please write a little utility
> > func? Doesn't have to be in GLContext, can be static right here.
> 
> I don't see a good way to do this. We could easily combine the last two with
> lambdas, but I don't see a really clean way to combine them otherwise.

Why can't you pass references to the objects at hand here to a utility function?

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> (In reply to Jeff Gilbert [:jgilbert] from comment #6)
> > > Really sucks to have to write this 3 times! Please write a little utility
> > > func? Doesn't have to be in GLContext, can be static right here.
> > 
> > I don't see a good way to do this. We could easily combine the last two with
> > lambdas, but I don't see a really clean way to combine them otherwise.
> 
> Why can't you pass references to the objects at hand here to a utility
> function?

They're static functions with differing arguments, so not easily.

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9f83fccf6b

Comment 11

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9bceebb8c22

because of mochitest failures:
21:12:49     INFO -  54283 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/canvas/drawingbuffer-static-canvas-test.html] Test passed - maxSize[0] > 0 is true
21:12:49     INFO -  54284 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/canvas/drawingbuffer-static-canvas-test.html] Test passed - maxSize[1] > 0 is true
21:13:02  WARNING -  TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | Exited with code -9 during test run
https://tbpl.mozilla.org/php/getParsedLog.php?id=20403818&tree=Mozilla-Inbound

Comment 13

[Guillaume Abadie]

Attachment: patch by gabadie

Patch resolving the problem by a different approach from the previous one :
 - resolve fails on conformance test https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/canvas/drawingbuffer-test.html 
 - Set the maximum value reachable by MAX_VIEWPORT_DIMS at 4096

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 763801 [details] [diff] [review]
patch by gabadie

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

As discussed offline, this needs some extra code to negociate a smaller size if a large size failed. (like in Jeff's old patch)

Comment 15

[Guillaume Abadie]

I'm currently testing

Comment 16

[Kelsey Gilbert [:jgilbert]]

Also, we would very much like to not limit ourselves to 4k textures when many cards can handle 8k.

Comment 17

[Kelsey Gilbert [:jgilbert]]

To be more clear, I would not like to take a patch which perpetuates the whole '4k is enough for everybody' thing. The approach in my patch seems the correct (non-hacky) way to do this, unless there are crippling reason we cannot go that route.

Comment 18

[Guillaume Abadie]

Let me explain you why I added that maximum reachable value :

Actualy, https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/canvas/drawingbuffer-static-canvas-test.html fails, because trying to create a canvas of size MAX_VIEWPORT_DIMS + 32.

But it just fail thanks by if (!IsOffscreenSizeAllowed(size)) in GLContext::CreateScreenBuffer

So In my patch, I clamp the canvas size by MAX_VIEWPORT_DIMS, to passe the test like it said in chapter 2.2 of the WebGL specs : "The context's drawingBufferWidth and drawingBufferHeight must match the canvas's width and height attributes as closely as possible, modulo implementation-dependent constraints."

That also the reason I create a offscreen context 16x16 first, to get MAX_VIEWPORT_DIMS before resizing to the desired dimension.

But MAX_VIEWPORT_DIMS can reach 16k with recent graphic cards. And actualy, my development laptop does. So in that case, the test try to create the main FBO with dimension of MAX_VIEWPORT_DIMS, can generate a crash of Firefox because my GTX 650M doesn't have enough graphic memory for. That why I added this maximum reachable value of MAX_VIEWPORT_DIMS, to finally pass this the whole test.

Would you like an user defined value "webgl.max-viewport-size" in about:config to set this maximum reachable value of MAX_VIEWPORT_DIMS ?

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Try to resize, and halve the size until it succeeds.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 825634 [details] [diff] [review]
patch: Try to resize, and halve the size until it succeeds.

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

::: content/canvas/src/WebGLContext.cpp
@@ +548,5 @@
>      }
>  #endif
>  
> +    mWidth = 1;
> +    mHeight = 1;

Could you make these 0 instead, to give CreateOffscreen a chance to avoid creating useless 1x1 surfaces, or give the OpenGL driver a chance to avoid some useless work by using a 0x0 surface that presumably doesn't require some of the stuff that a 1x1 surface needs?

@@ +598,5 @@
> +        // Requested size was too large.
> +        GenerateWarning("Requested size %dx%d was too large. Reduced to %dx%d.",
> +                        width, height,
> +                        mWidth, mHeight);
> +    }

The code here duplicates the one you had above to handle the resizing case. Could you share it in a helper method? Would it even fit in the existing Resize method?

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #21)
> Comment on attachment 825634 [details] [diff] [review]
> patch: Try to resize, and halve the size until it succeeds.
> 
> Review of attachment 825634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +548,5 @@
> >      }
> >  #endif
> >  
> > +    mWidth = 1;
> > +    mHeight = 1;
> 
> Could you make these 0 instead, to give CreateOffscreen a chance to avoid
> creating useless 1x1 surfaces, or give the OpenGL driver a chance to avoid
> some useless work by using a 0x0 surface that presumably doesn't require
> some of the stuff that a 1x1 surface needs?
There's not currently support for creating a headless GLContext. We can add support, but we should do it in a different bug. I filed bug 933878 for this.
> 
> @@ +598,5 @@
> > +        // Requested size was too large.
> > +        GenerateWarning("Requested size %dx%d was too large. Reduced to %dx%d.",
> > +                        width, height,
> > +                        mWidth, mHeight);
> > +    }
> 
> The code here duplicates the one you had above to handle the resizing case.
> Could you share it in a helper method? Would it even fit in the existing
> Resize method?

Sure. I didn't want to put it in the actual resize function so as to keep it nice and simple.
I can add a ResizeAndWarn function, or similar.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Try to resize, and halve the size until it succeeds.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Try to resize, and halve the size until it succeeds.

Fixed a pair of warnings.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 826027 [details] [diff] [review]
patch: Try to resize, and halve the size until it succeeds.

Carry forward r+.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Bitrotted, please rebase.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Ah, this is built on top of bug 933009. Never hurts to note dependencies :)

Comment 28

[Kelsey Gilbert [:jgilbert]]

Sorry!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23226a9c128d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Bug 933009 had to be backed out, so this was as well.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb5a87da513

Comment 32

[Kelsey Gilbert [:jgilbert]]

Almost there, but we have an assert about a D3D9 hresult error on the Win8 slaves:

E_INVALIDARG	One or more arguments are not valid	0x80070057

I had to add a printf to grab this from TBPL, as I can't reproduce it locally.
Here's the Try run:
https://tbpl.mozilla.org/?tree=Try&rev=83bfeaf50c5e

Comment 33

[Kelsey Gilbert [:jgilbert]]

New try run with more printfs:
https://tbpl.mozilla.org/?tree=Try&rev=1391ea2979ab

Comment 34

[Kelsey Gilbert [:jgilbert]]

FWIW, this looks similar to ANGLE issue 199:
https://code.google.com/p/angleproject/issues/detail?id=199

Comment 35

[Kelsey Gilbert [:jgilbert]]

Duping forward.