Closed Bug 847714 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 996266

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

      No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #721001 - Flags: review?(bjacob)
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.
Attachment #721001 - Flags: review?(bjacob) → review-
Oh, also: why don't we clamp the drawingbuffer width/height to 4K like Chrome does?
(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.
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.
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #721001 - Attachment is obsolete: true
Attachment #721465 - Flags: review?(bjacob)
(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?
(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.
Attachment #721465 - Flags: review?(bjacob) → review+
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
Assignee: jgilbert → gabadie
Attached patch patch by gabadie (obsolete) — Splinter Review
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
Attachment #721465 - Attachment is obsolete: true
Attachment #763801 - Flags: review?(bjacob)
Blocks: 884062
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)
Attachment #763801 - Flags: review?(bjacob)
I'm currently testing
Also, we would very much like to not limit ourselves to 4k textures when many cards can handle 8k.
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.
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 ?
Assignee: guillaume.abadie → jgilbert
Attachment #763801 - Attachment is obsolete: true
Attachment #825634 - Flags: review?(bjacob)
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?
Attachment #825634 - Flags: review?(bjacob) → review+
Blocks: 933878
(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.
Attachment #825634 - Attachment is obsolete: true
Attachment #826026 - Flags: review?(bjacob)
Attachment #826026 - Flags: review?(bjacob) → review+
Fixed a pair of warnings.
Attachment #826026 - Attachment is obsolete: true
Attachment #826027 - Flags: review?(bjacob)
Comment on attachment 826027 [details] [diff] [review]
patch: Try to resize, and halve the size until it succeeds.

Carry forward r+.
Attachment #826027 - Flags: review?(bjacob) → review+
Keywords: checkin-needed
Bitrotted, please rebase.
Keywords: checkin-needed
Ah, this is built on top of bug 933009. Never hurts to note dependencies :)
Keywords: checkin-needed
Sorry!
Depends on: 933009
See Also: → 982476
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
New try run with more printfs:
https://tbpl.mozilla.org/?tree=Try&rev=1391ea2979ab
FWIW, this looks similar to ANGLE issue 199:
https://code.google.com/p/angleproject/issues/detail?id=199
Duping forward.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: