Bug 178223 - WebGL clamps drawingBufferWidth to 4096 pixels on a 5120 monitor/canvas
Summary: WebGL clamps drawingBufferWidth to 4096 pixels on a 5120 monitor/canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Safari 11
Hardware: Mac macOS 10.12
: P2 Major
Assignee: Dean Jackson
URL:
Keywords: InRadar
: 162604 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-10-12 12:09 PDT by Alec Miller
Modified: 2017-10-18 10:15 PDT (History)
11 users (show)

See Also:


Attachments
Example showing clamping of WebGL renderBuffer (2.35 KB, text/html)
2017-10-12 12:09 PDT, Alec Miller
no flags Details
Patch (5.52 KB, patch)
2017-10-16 22:45 PDT, Dean Jackson
graouts: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Miller 2017-10-12 12:09:53 PDT
Created attachment 323543 [details]
Example showing clamping of WebGL renderBuffer

Safari currently clamps the WebGL context to 4096 pixels horizontally and vertically.  This is what Chrome/Firefox used to do, but they made the clamp on total pixels (4k x 4k) instead of per side.  It looks like the WebKit implementation is using the old behavior.  This is problematic on both 5K displays including the 5K iMac.

Enclosed is a simple WebGL sample.  Expand the window on a 5K display, and at a canvas size of 4097 pixels, you'll see a jump in the color and then aliasing bands.  The drawingBufferWidth is clamped to 4096 pixels, and the checkerboard pattern is non-uniformly scaled up to 5120 pixels if you expand the browser fully or go fullscreen.

This is problematic for design tools like ours, where we rely on pixel precision.  Now the experience on a 5K display will be letterboxed bars around the interactive canvas when the UI is hidden.  You can see this in figma.com rendering on 5K monitor with "Show UI" disabled.  

We would prefer to not have to wait for a long update cycle (another OSX release) on WebKit to see this corrected.
Comment 1 Dean Jackson 2017-10-16 22:42:40 PDT
<rdar://problem/34597567>
Comment 2 Dean Jackson 2017-10-16 22:45:28 PDT
Created attachment 323996 [details]
Patch
Comment 3 Antoine Quint 2017-10-16 23:05:44 PDT
Comment on attachment 323996 [details]
Patch

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

> LayoutTests/fast/canvas/webgl/large-drawing-buffer-resize.html:56
> +}, false);

You can drop this parameter.
Comment 4 Dean Jackson 2017-10-16 23:22:50 PDT
Committed r223459: <https://trac.webkit.org/changeset/223459>
Comment 5 Jon Lee 2017-10-17 09:38:46 PDT
Comment on attachment 323996 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-851
> -    maxSize = std::min(maxSize, sizeUpperLimit);

Why was this limit here in the first place? Should we restrict the size based on the device?
Comment 7 Ryan Haddad 2017-10-17 11:39:41 PDT
The test for this change is timing out on iOS 11 Release bots, failing on iOS 11 Debug:

Release:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r223553%20(595)/results.html

Debug:
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/425
--- /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/fast/canvas/webgl/large-drawing-buffer-resize-expected.txt
+++ /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/fast/canvas/webgl/large-drawing-buffer-resize-actual.txt
@@ -1,5 +1,5 @@
-Original drawing buffer: 5000 x 5000
+Original drawing buffer: 4096 x 4096
 
-Resized drawing buffer: 5001 x 5001
+Resized drawing buffer: 4096 x 4096
Comment 8 Ryan Haddad 2017-10-17 11:49:17 PDT
Reverted r223459 for reason:

This change introduced LayoutTest failures.

Committed r223563: <https://trac.webkit.org/changeset/223563>
Comment 9 Dean Jackson 2017-10-17 12:18:02 PDT
Committed r223567: <https://trac.webkit.org/changeset/223567>
Comment 10 Ryan Haddad 2017-10-17 12:46:36 PDT
(In reply to Dean Jackson from comment #9)
> Committed r223567: <https://trac.webkit.org/changeset/223567>
Thanks for updating the TestExpectations for iOS. Was anything done to address the webgl/1.0.2/conformance/canvas/drawingbuffer-static-canvas-test.html failure on macOS?
Comment 11 Dean Jackson 2017-10-17 13:17:00 PDT
(In reply to Ryan Haddad from comment #10)
> (In reply to Dean Jackson from comment #9)
> > Committed r223567: <https://trac.webkit.org/changeset/223567>
> Thanks for updating the TestExpectations for iOS. Was anything done to
> address the
> webgl/1.0.2/conformance/canvas/drawingbuffer-static-canvas-test.html failure
> on macOS?

No, I didn't see that one. I'll fix it now.
Comment 12 Dean Jackson 2017-10-17 13:18:45 PDT
(In reply to Jon Lee from comment #5)
> Comment on attachment 323996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323996&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-851
> > -    maxSize = std::min(maxSize, sizeUpperLimit);
> 
> Why was this limit here in the first place? Should we restrict the size
> based on the device?

I think this was mostly there because older hardware had problems with large buffers, but we still identify that hardware in another place.

Note that this was only being applied on resize, so it was still possible to create such large buffers. i.e. we were not protecting it properly anyway
Comment 13 Michael Herzog 2017-10-18 10:15:38 PDT
*** Bug 162604 has been marked as a duplicate of this bug. ***