Bug 187750 – Safari WebGL does not consistently provide correct GPU context on eGPU systems

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

[Justin Fan]

Safari WebGL does not consistently provide correct GPU context on eGPU systems

[Justin Fan]

<rdar://problem/39531436>

[Justin Fan]

Created attachment 345230 [details]
Patch

[Per Arne Vollan]

Comment on attachment 345230 [details]
Patch

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

> Source/WebCore/page/Chrome.cpp:520
> +#if ENABLE(GRAPHICS_CONTEXT_3D)
> +    GraphicsContext3DManager::manager().screenDidChange(displayID);
> +#endif

I think this will change the GPU for all contexts in the process. There can be multiple contexts in multiple host windows. Shouldn't we only change the contexts belonging to this host window?

[Dean Jackson]

Comment on attachment 345230 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line goes under the title and before the description.

>> Source/WebCore/page/Chrome.cpp:520
>> +#endif
> 
> I think this will change the GPU for all contexts in the process. There can be multiple contexts in multiple host windows. Shouldn't we only change the contexts belonging to this host window?

I was hoping to avoid this for now, but yes, we should probably address it. I was really hoping to avoid having anything in platform know about HostWindow.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1286
> +    using PlatformDisplayID = uint32_t;
> +    void screenDidChange(PlatformDisplayID);

Maybe this should be #if PLATFORM(MAC)

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> +    if (hostWindow && hostWindow->displayID())
> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));

Where did hostWindow come from? I think maybe this context should get its display id from the manager.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DManager.h:54
> +    static GraphicsContext3DManager& manager();

Should be called sharedManager()

> Source/WebCore/platform/mac/PlatformScreenMac.mm:119
> +    // The 0th renderer is the hardware renderer

End with a period.

[Justin Fan]

Created attachment 345282 [details]
Patch

[Justin Fan]

Made some changes to try to get other platforms to build; addressing Dean's comments now.

[Justin Fan]

Created attachment 345288 [details]
Patch

[Per Arne Vollan]

Comment on attachment 345288 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> -#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> -    if (auto displayMask = primaryOpenGLDisplayMask()) {
> -        if (hostWindow && hostWindow->displayID())
> -            displayMask = displayMaskForDisplay(hostWindow->displayID());
> -        identifyAndSetCurrentGPU(pixelFormatObj, numPixelFormats, displayMask, m_contextObj);
> -    }
> +#if PLATFORM(MAC)
> +    if (hostWindow && hostWindow->displayID())
> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));

I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.

[Per Arne Vollan]

Comment on attachment 345288 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
> 
> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.

Maybe you could create a primaryRendererID() function?

[Per Arne Vollan]

Comment on attachment 345288 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
> +    // The 0th renderer is the hardware renderer.
> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);

Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?

[Justin Fan]

Comment on attachment 345288 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
>>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
>> 
>> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.
> 
> Maybe you could create a primaryRendererID() function?

If this context's window gets parented, it should change its renderer accordingly. So even if it's created with the software renderer at first, the user shouldn't see poor performance unless this is unable to identify the GPU driving the window. 

Let me see what happens on another device; I am having trouble getting ToT spade to work on my laptop.

>> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
>> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);
> 
> Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?

This is reasonable; I'll do that.

[Per Arne Vollan]

(In reply to Justin Fan from comment #11)
> Comment on attachment 345288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345288&action=review
> 
> >>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> >>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
> >> 
> >> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.
> > 
> > Maybe you could create a primaryRendererID() function?
> 
> If this context's window gets parented, it should change its renderer
> accordingly. So even if it's created with the software renderer at first,
> the user shouldn't see poor performance unless this is unable to identify
> the GPU driving the window. 
> 
> Let me see what happens on another device; I am having trouble getting ToT
> spade to work on my laptop.
> 

Right, but wouldn't it be good to avoid the software renderer from the start? The primary renderer ID will be the correct one in most cases. It will only be wrong on a secondary screen. This will avoid a switch from the software renderer to a hardware renderer on every context creation.

> >> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
> >> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);
> > 
> > Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?
> 
> This is reasonable; I'll do that.

[Justin Fan]

Created attachment 345318 [details]
Patch

[EWS Watchlist]

Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8582677

New failing tests:
webgl/max-active-contexts-webglcontextlost-prevent-default.html
webgl/max-active-contexts-oldest-context-lost.html
webgl/many-contexts-access-after-loss.html

[EWS Watchlist]

Created attachment 345321 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8582728

New failing tests:
webgl/max-active-contexts-oldest-context-lost.html
webgl/many-contexts-access-after-loss.html

[EWS Watchlist]

Created attachment 345323 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8582828

New failing tests:
webgl/max-active-contexts-oldest-context-lost.html
webgl/max-active-contexts-webglcontextlost-prevent-default.html
webgl/many-contexts-access-after-loss.html

[EWS Watchlist]

Created attachment 345324 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8583443

New failing tests:
webgl/many-contexts-access-after-loss.html

[EWS Watchlist]

Created attachment 345328 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Justin Fan]

Created attachment 345385 [details]
Patch

[Dean Jackson]

Comment on attachment 345385 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:167
> +    ASSERT(!m_contexts.contains(contextWithWindow));

We should probably also ASSERT on the unlikely case that { context, nullptr } is present in m_contexts.

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:175
> +    ContextWithWindow contextWithWindow { context, nullptr };
> +    ASSERT(m_contexts.contains(contextWithWindow));
> +    m_contexts.removeFirst(contextWithWindow);

What am I misunderstanding here? You inserted the context with it's HostWindow pointer. But you're removing it with a nullptr. How is it being found?

(LATER) - oh, I see, it's got an operator==

In this case it might be better to use a HashMap of <GC3D*, HostWindow*>. You can still iterate over all the elements easily.

[Justin Fan]

Created attachment 345402 [details]
Patch

[Justin Fan]

Updated patch so that we never create a context, shared or not, with a nullptr hostWindow. 

So far I'd only been testing on iMac Pro. I'm still seeing unreliable behavior on my MacBook Pro. Let's not land this until I can figure out what's going on.

[Justin Fan]

Created attachment 345407 [details]
Patch

[Justin Fan]

On my laptop and the eGPU display, when asking the PixelFormatObj for the rendererID for a virtual screen, I get different numbers on some iterations through the virtual screens. Thus the preferred rendererIDs determined during process creation aren't matching any of them. 

99% sure this isn't happening on iMac Pro. Could be an issue with muxing mucking up contexts.

[EWS Watchlist]

Comment on attachment 345407 [details]
Patch

Attachment 345407 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8594103

New failing tests:
http/wpt/offscreen-canvas/getContext-webgl.html
http/wpt/offscreen-canvas/transferToImageBitmap-webgl.html

[EWS Watchlist]

Created attachment 345419 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Justin Fan]

(In reply to Dean Jackson from comment #23)
> Comment on attachment 345385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345385&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:167
> > +    ASSERT(!m_contexts.contains(contextWithWindow));
> 
> We should probably also ASSERT on the unlikely case that { context, nullptr
> } is present in m_contexts.

Oops, we can't actually do that because we can create offscreen canvases that aren't associated with a window. 

In that case the code should select the hardware renderer for the primary display.

[Justin Fan]

Created attachment 345484 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345484 [details]
Patch

Clearing flags on attachment: 345484

Committed r234074: <https://trac.webkit.org/changeset/234074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.