Removed arbitrary 4096 size limit on WebGL canvas backbuffers, Take 2

Removed arbitrary 4096 size limit on WebGL canvas backbuffers, Take 2

Follow up to https://codereview.chromium.org/1002523007/, which had to be
reverted due to hitting OOM errors on some hardware which advertized support
for larger textures than it could actually create. This version checks for those
errors and scales back the buffer allocation if needed.

BUG=445542
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192970

[bajones, 2015-03-30 20:06:45]

bajones@chromium.org changed reviewers:
+ kbr@chromium.org

[bajones, 2015-03-30 20:06:46]

PTAL

[Ken Russell (switch to Gerrit), 2015-03-31 02:13:34]

https://codereview.chromium.org/1040153002/diff/1/Source/platform/graphics/gp...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/1040153002/diff/1/Source/platform/graphics/gp...
Source/platform/graphics/gpu/DrawingBuffer.cpp:710: return false;
There's a preexisting bug with all of the getError() calls in this file. If it
happened that the user caused a GL error then these checks may return the other
error and miss the out-of-memory error.

In order to fix this, the m_lostContextErrors in WebGLRenderingContextBase
should be generalized to be used all the time, and a method for copying the
"real" GL errors from the WebGraphicsContext3D into it should be added. Then
before WebGLRenderingContextBase calls into DrawingBuffer, the
WebGLRenderingContextBase should copy the real errors into itself. Care is
needed to watch out for lost contexts.

Would you either add this mechanism in this CL or file a bug about doing so?
Thanks.

[Ken Russell (switch to Gerrit), 2015-03-31 02:14:28]

P.S. Please also file bugs about the unrelated layout test failures on
linux_chromium_rel_ng. They won't get fixed otherwise.

[bajones, 2015-03-31 23:25:41]

On 2015/03/31 02:13:34, Ken Russell wrote:
>
https://codereview.chromium.org/1040153002/diff/1/Source/platform/graphics/gp...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:710: return false;
> There's a preexisting bug with all of the getError() calls in this file. If it
> happened that the user caused a GL error then these checks may return the
other
> error and miss the out-of-memory error.
> 
> In order to fix this, the m_lostContextErrors in WebGLRenderingContextBase
> should be generalized to be used all the time, and a method for copying the
> "real" GL errors from the WebGraphicsContext3D into it should be added. Then
> before WebGLRenderingContextBase calls into DrawingBuffer, the
> WebGLRenderingContextBase should copy the real errors into itself. Care is
> needed to watch out for lost contexts.
> 
> Would you either add this mechanism in this CL or file a bug about doing so?
> Thanks.

Done. Any thoughts on the new approach?

[Ken Russell (switch to Gerrit), 2015-03-31 23:52:09]

Thanks, this looks better. LGTM

https://codereview.chromium.org/1040153002/diff/20001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/1040153002/diff/20001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:86: while (error != GL_NO_ERROR)
{
Please limit this to something like 100 errors just to avoid pathological
situations. Feel free to ASSERT in cases where the error count is exceeded.

[bajones, 2015-04-01 17:34:32]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-04-01 17:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1040153002/#ps40001
(title: "Added suggested ASSERT")

[commit-bot: I haz the power, 2015-04-01 17:35:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1040153002/40001

[commit-bot: I haz the power, 2015-04-01 22:41:05]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192970

[piman, 2015-04-06 22:35:09]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-04-06 22:35:10]

https://codereview.chromium.org/1040153002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/1040153002/diff/40001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:1018: ScopedErrorCache
errorCache(m_context.get());
drive-by: this sounds pretty unfortunate to do (many!) sync round trips to the
GPU process... This will hit on every webgl page load as well as on every canvas
resize. Each resizeFramebuffer will typically involve 4 to 6 round trips (at
least one for each ScopedErrorCache constructor, one for each peekError, a pair
of these for the resolved texture, the depth RB and possibly the multisampled
RB).

Could this be done by either:
1- limiting the error checks to "large" textures (if 4096x4096 were ok before,
they should still be)
2- identifying the GPUs which exhibit the bad behavior and add a workaround to
limit their max texture size to 4096
3- at the very very minimum, a single pair of round trips?