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.

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.

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

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

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 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)

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 ?

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?

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

Fixed a pair of warnings.

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

Bitrotted, please rebase.

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

Sorry!

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

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.