Bug 91976 – [WebGL] Initial size of canvas can be larger than MAX_VIEWPORT_DIMS.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Nayan Kumar K]

Following WebGL conformance test is failing with the current version of WebKit,

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/canvas/drawingbuffer-test.html

This test got added in r18179 revision of WebGL conformance test suite.

From section 2.2 of the spec the WebGL implementation says initial size of the canvas can be larger than MAX_VIEWPORT_DIMS.

[Nayan Kumar K]

Created attachment 153767 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153767 [details]
Patch

Attachment 153767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13318921

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/drawingbuffer-test.html
fast/canvas/webgl/drawingbuffer-test.html

[WebKit Review Bot]

Created attachment 153794 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Nayan Kumar K]

Created attachment 154061 [details]
Patch

[Kenneth Russell]

Comment on attachment 154061 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154061&action=review

Thanks for the patch! It looks great except for the same logic being duplicated three times. Please refactor that and submit a new patch, and I'll gladly r+ it.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:451
> +    m_drawingBuffer = DrawingBuffer::create(m_context.get(), IntSize(clamp(canvas()->width(), 1, m_maxViewportDims[0]), clamp(canvas()->height(), 1, m_maxViewportDims[1])), preserve, alpha);

Could you please add a private helper method "IntSize clampedCanvasSize()" which returns the canvas' width and height clamped to m_maxViewportDims? Then you can call it here, in initializeNewContext and markContextChanged.

[Nayan Kumar K]

Created attachment 154265 [details]
Reworked patch

[Kenneth Russell]

Comment on attachment 154265 [details]
Reworked patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154265&action=review

Thanks, looks better. r=me. One comment. Mark cq? if you want this committed as is.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:523
> +        m_drawingBuffer->reset(clampedCanvasSize());

How about a local variable instead of calling clampedCanvasSize() five times?

> LayoutTests/fast/canvas/webgl/drawingbuffer-test.html:62
> +test(100, maxSize[1]+100)

In the near future, please align the contents of this test with conformance/canvas/drawingbuffer-test.html in the WebGL conformance suite. In general, if we make a code change in WebKit affecting one of the layout tests that's in the conformance suite, we pull the latest version of the conformance test back into WebKit as a layout test.

[Nayan Kumar K]

Created attachment 154544 [details]
Reworked patch

[Nayan Kumar K]

(In reply to comment #7)
> (From update of attachment 154265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154265&action=review
> 
> Thanks, looks better. r=me. One comment. Mark cq? if you want this committed as is.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:523
> > +        m_drawingBuffer->reset(clampedCanvasSize());
> 
> How about a local variable instead of calling clampedCanvasSize() five times?

I have reworked and used a local variable here. Reworked patch is set for r?. I'll land the patch once it gets r=.

> 
> > LayoutTests/fast/canvas/webgl/drawingbuffer-test.html:62
> > +test(100, maxSize[1]+100)
> 
> In the near future, please align the contents of this test with conformance/canvas/drawingbuffer-test.html in the WebGL conformance suite. In general, if we make a code change in WebKit affecting one of the layout tests that's in the conformance suite, we pull the latest version of the conformance test back into WebKit as a layout test.

Sure. Soon I'll submit another patch to align this test case with the one in WebGL conformance suite.

[Kenneth Russell]

Comment on attachment 154544 [details]
Reworked patch

Nice, thanks. r=me

I'm setting cq+ since I assume you wanted this committed, but please set the cq? bit in the future. Thanks.

[WebKit Review Bot]

Comment on attachment 154544 [details]
Reworked patch

Clearing flags on attachment: 154544

Committed r123816: <http://trac.webkit.org/changeset/123816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nayan Kumar K]

Reopening to attach new patch.

[Nayan Kumar K]

Created attachment 154839 [details]
Reworked patch

[Nayan Kumar K]

By mistake, attached wrong patch to a wrong bug. Hence closing this issue manually.