Bug 183923 – The layout test fast/canvas/webgl/read-pixels-test.html is timing out.

• Component: WebCore Misc.
• Status: NEW

[Per Arne Vollan]

This happens when there is no WindowServer connection.

[Per Arne Vollan]

Created attachment 336325 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

Ccing Dean, who IIRC wrote this

[Brent Fulgham]

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?

[Tim Horton]

This is not about whatever you’re talking about, this is about discrete vs. integrated GPU.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 336546 [details]
Patch

[Per Arne Vollan]

Created attachment 336548 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #8)
> Created attachment 336548 [details]
> Patch

We should probably use CGLQueryRendererInfo, instead.

[Per Arne Vollan]

Created attachment 336600 [details]
Patch

[Per Arne Vollan]

Created attachment 336608 [details]
Patch

[Per Arne Vollan]

Created attachment 336609 [details]
Patch

[Brent Fulgham]

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?

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 336616 [details]
Patch

[Brent Fulgham]

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;

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 336625 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/38931912>

[Per Arne Vollan]

<rdar://problem/38756869>

[WebKit Commit Bot]

Comment on attachment 336625 [details]
Patch

Clearing flags on attachment: 336625

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