Bug 183923 - The layout test fast/canvas/webgl/read-pixels-test.html is timing out.
Summary: The layout test fast/canvas/webgl/read-pixels-test.html is timing out.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-22 16:24 PDT by Per Arne Vollan
Modified: 2018-03-27 16:39 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2018-03-22 16:28 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (13.19 KB, patch)
2018-03-26 15:29 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (13.09 KB, patch)
2018-03-26 15:39 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (14.39 KB, patch)
2018-03-27 11:25 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2018-03-27 12:08 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (14.58 KB, patch)
2018-03-27 12:11 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (17.65 KB, patch)
2018-03-27 14:09 PDT, Per Arne Vollan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch (17.62 KB, patch)
2018-03-27 15:35 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-03-22 16:24:38 PDT
This happens when there is no WindowServer connection.
Comment 1 Per Arne Vollan 2018-03-22 16:28:41 PDT
Created attachment 336325 [details]
Patch
Comment 2 Tim Horton 2018-03-22 16:33:41 PDT
Comment on attachment 336325 [details]
Patch

I assume there's a reason that we're not setting this bit if we don't have a mux-capable GPU, and also this is not the only caller of that function. We need to carefully consider each caller and what it means if this function now lies.
Comment 3 Tim Horton 2018-03-22 16:34:02 PDT
Ccing Dean, who IIRC wrote this
Comment 4 Brent Fulgham 2018-03-22 16:40:13 PDT
Comment on attachment 336325 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:337
>          attribs.append(kCGLPFAAllowOfflineRenderers);

I actually don't understand why we need to decide whether to set this or not. It seems like for modern WebKit, all of our OpenGL drawing is in an "offline renderer" mode, isn't? We don't throw bits directly from the GPU to the display, do we? I thought it all had to pass to the UIProcess for display?
Comment 5 Tim Horton 2018-03-22 16:42:38 PDT
This is not about whatever you’re talking about, this is about discrete vs. integrated GPU.
Comment 6 Per Arne Vollan 2018-03-23 07:18:05 PDT
(In reply to Tim Horton from comment #2)
> Comment on attachment 336325 [details]
> Patch
> 
> I assume there's a reason that we're not setting this bit if we don't have a
> mux-capable GPU, and also this is not the only caller of that function. We
> need to carefully consider each caller and what it means if this function
> now lies.

Thanks for reviewing! I will look more closely into this.
Comment 7 Per Arne Vollan 2018-03-26 15:29:04 PDT
Created attachment 336546 [details]
Patch
Comment 8 Per Arne Vollan 2018-03-26 15:39:53 PDT
Created attachment 336548 [details]
Patch
Comment 9 Per Arne Vollan 2018-03-26 17:10:49 PDT
(In reply to Per Arne Vollan from comment #8)
> Created attachment 336548 [details]
> Patch

We should probably use CGLQueryRendererInfo, instead.
Comment 10 Per Arne Vollan 2018-03-27 11:25:48 PDT
Created attachment 336600 [details]
Patch
Comment 11 Per Arne Vollan 2018-03-27 12:08:04 PDT
Created attachment 336608 [details]
Patch
Comment 12 Per Arne Vollan 2018-03-27 12:11:34 PDT
Created attachment 336609 [details]
Patch
Comment 13 Brent Fulgham 2018-03-27 12:27:36 PDT
Comment on attachment 336609 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:486
> +    if (m_displayMask && m_contextObj) {

I feel like this might be better as a function, "identifyAndSetCurrentGPU" or something like that.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2651
> +    CGOpenGLDisplayMask currentDisplaymask = CGDisplayIDToOpenGLDisplayMask(displayID);

auto?
Comment 14 Per Arne Vollan 2018-03-27 12:38:50 PDT
(In reply to Brent Fulgham from comment #13)
> Comment on attachment 336609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336609&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:486
> > +    if (m_displayMask && m_contextObj) {
> 
> I feel like this might be better as a function, "identifyAndSetCurrentGPU"
> or something like that.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:2651
> > +    CGOpenGLDisplayMask currentDisplaymask = CGDisplayIDToOpenGLDisplayMask(displayID);
> 
> auto?

Thanks for reviewing! I will update the patch.
Comment 15 Per Arne Vollan 2018-03-27 14:09:41 PDT
Created attachment 336616 [details]
Patch
Comment 16 Brent Fulgham 2018-03-27 14:37:39 PDT
Comment on attachment 336616 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The test is timing out because software rendering is being used when there is no WindowServer access.

The test is timing out when we do not interact directly with the WindowServer, causing
OpenGL to fall back to software rendering. In this mode, any call to CGLChoosePixelFormat
requesting an accelerated pixel format will fail because it cannot determine which GPU is
connected to the display.

OpenGL treats all GPUs as if they were offline when used in a process (like the WebContent
process) that does not directly control the display.

We can get correct behavior if we tell OpenGL which GPU is currently connected to the
display, and if we instruct CGLChoosePixelFormat to create an offline renderer pixel format
by including the 'kCGLPFAAllowOfflineRenderers' flag in its arguments.

We can use CGLSetVirtualScreen with an OpenGL display mask that tells the OpenGL framework
which GPU it should use.

See https://developer.apple.com/library/content/technotes/tn2229/_index.html#//apple_ref/doc/uid/DTS40008924-CH1-SUBSECTION7
for details on how the virtual screen is found from the OpenGL display mask.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1273
> +		49C7B9FC1042D3650009D447 /* GraphicsContext3D.h in Headers */ = {isa = PBXBuildFile; fileRef = 49C7B9FB1042D3650009D447 /* GraphicsContext3D.h */; settings = {ATTRIBUTES = (Private, ); }; };

Were these two changes intentional?

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:406
> +    // To help OpenGL choose GPU, it is possible to use the function CGLSetVirtualScreen.

// CGLSetVirtualScreen can be used to tell OpenGL which GPU it should be using.

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:412
> +    for (int virtualScreen = 0; virtualScreen < numPixelFormats; virtualScreen++) {

++virtualScreen

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:468
> +    setPixelFormat(attribs, 32, 32, !attrs.forceSoftwareRenderer, true, false, useMultisampling, attrs.useGLES3, allowOfflineRenderers());

How do we know m_displayMask is set properly here? Does the WebPage always receive OpenGLDisplayMaskChanged when a new Web Process is launched?

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:808
> +    if (!hasWindowServerConnection)

Isn't this just:

if (!!m_displayMask)
    return true;
Comment 17 Per Arne Vollan 2018-03-27 15:02:02 PDT
(In reply to Brent Fulgham from comment #16)
> Comment on attachment 336616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336616&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        The test is timing out because software rendering is being used when there is no WindowServer access.
> 
> The test is timing out when we do not interact directly with the
> WindowServer, causing
> OpenGL to fall back to software rendering. In this mode, any call to
> CGLChoosePixelFormat
> requesting an accelerated pixel format will fail because it cannot determine
> which GPU is
> connected to the display.
> 
> OpenGL treats all GPUs as if they were offline when used in a process (like
> the WebContent
> process) that does not directly control the display.
> 
> We can get correct behavior if we tell OpenGL which GPU is currently
> connected to the
> display, and if we instruct CGLChoosePixelFormat to create an offline
> renderer pixel format
> by including the 'kCGLPFAAllowOfflineRenderers' flag in its arguments.
> 
> We can use CGLSetVirtualScreen with an OpenGL display mask that tells the
> OpenGL framework
> which GPU it should use.
> 
> See
> https://developer.apple.com/library/content/technotes/tn2229/_index.html#//
> apple_ref/doc/uid/DTS40008924-CH1-SUBSECTION7
> for details on how the virtual screen is found from the OpenGL display mask.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:1273
> > +		49C7B9FC1042D3650009D447 /* GraphicsContext3D.h in Headers */ = {isa = PBXBuildFile; fileRef = 49C7B9FB1042D3650009D447 /* GraphicsContext3D.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Were these two changes intentional?
> 

Yes, these WebCore files are now included from WebKit.

> > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:406
> > +    // To help OpenGL choose GPU, it is possible to use the function CGLSetVirtualScreen.
> 
> // CGLSetVirtualScreen can be used to tell OpenGL which GPU it should be
> using.
> 
> > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:412
> > +    for (int virtualScreen = 0; virtualScreen < numPixelFormats; virtualScreen++) {
> 
> ++virtualScreen
> 
> > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:468
> > +    setPixelFormat(attribs, 32, 32, !attrs.forceSoftwareRenderer, true, false, useMultisampling, attrs.useGLES3, allowOfflineRenderers());
> 
> How do we know m_displayMask is set properly here? Does the WebPage always
> receive OpenGLDisplayMaskChanged when a new Web Process is launched?
> 

Yes, we should always get a display mask when a Web Process is launched, since the display mask is sent when the display ID is sent.

> > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:808
> > +    if (!hasWindowServerConnection)
> 
> Isn't this just:
> 
> if (!!m_displayMask)
>     return true;

Thanks for reviewing!
Comment 18 Per Arne Vollan 2018-03-27 15:35:14 PDT
Created attachment 336625 [details]
Patch
Comment 19 Radar WebKit Bug Importer 2018-03-27 15:54:18 PDT
<rdar://problem/38931912>
Comment 20 Per Arne Vollan 2018-03-27 16:17:59 PDT
<rdar://problem/38756869>
Comment 21 WebKit Commit Bot 2018-03-27 16:39:45 PDT
Comment on attachment 336625 [details]
Patch

Clearing flags on attachment: 336625

Committed r230015: <https://trac.webkit.org/changeset/230015>