Safari WebGL does not consistently provide correct GPU context on eGPU systems
<rdar://problem/39531436>
Created attachment 345230 [details] Patch
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?
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.
Created attachment 345282 [details] Patch
Made some changes to try to get other platforms to build; addressing Dean's comments now.
Created attachment 345288 [details] Patch
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.
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?
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?
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.
(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.
Created attachment 345318 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 345385 [details] Patch
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.
Created attachment 345402 [details] Patch
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.
Created attachment 345407 [details] Patch
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.
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
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
(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.
Created attachment 345484 [details] Patch
Comment on attachment 345484 [details] Patch Clearing flags on attachment: 345484 Committed r234074: <https://trac.webkit.org/changeset/234074>
All reviewed patches have been landed. Closing bug.