[Win][WK2] Implement IPC::Semaphore to run WebGL in GPU process

• Component: WebGL
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][WK2] Implement IPC::Semaphore to run WebGL in GPU process

WebGL canvas tests are failing in WinCairo WK2 layout tests since r274327 enabled GPU process mode for WebKitTestRunner.

  webgl/1.0.3/conformance/canvas/canvas-test.html [ Failure ]
  webgl/1.0.3/conformance/canvas/draw-static-webgl-to-multiple-canvas-test.html [ Failure ]
  webgl/1.0.3/conformance/canvas/draw-webgl-to-canvas-test.html [ Failure ]
  webgl/1.0.3/conformance/canvas/to-data-url-test.html [ Failure ]
  webgl/1.0.3/conformance/context/context-attribute-preserve-drawing-buffer.html [ Failure ]
  webgl/1.0.3/conformance/textures/gl-pixelstorei.html [ Failure ]
  (snipped)

[Fujii Hironori]

Created attachment 424814 [details]
WIP patch

[Kimmo Kinnunen]

Comment on attachment 424814 [details]
WIP patch

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

Great stuff. I know it's WIP, just some ideas.

> Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp:46
> +        m_semaphoreHandle = CreateSemaphoreA(nullptr, 0, 1, getSemaphoreName(m_semaphoreNumber).data());

Would be great if you investigated if it's possible to create a nameless semaphore and then duplicate handle it to the other process.
I think the support already might exist in AttachmentWin.

> Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp:87
> +    ReleaseSemaphore(m_semaphoreHandle, 1, nullptr);

These could assert maybe that the result is somehow expected

> Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp:119
> +        CloseHandle(m_semaphoreHandle);

need to clear the handle after this

[Fujii Hironori]

Thank you very much. All sound good ideas. Will fix.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #0)
> [Win][WK2] Implement IPC::Semaphore to run WebGL in GPU process
> 
> WebGL canvas tests are failing in WinCairo WK2 layout tests since r274327
> enabled GPU process mode for WebKitTestRunner.

I was wrong. Implementing IPC::Semaphore doesn't solve those test failures. They are different issue. I will fix those test failures in other bug ticket.

[Fujii Hironori]

Created attachment 424994 [details]
Patch

[Fujii Hironori]

Comment on attachment 424814 [details]
WIP patch

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

>> Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp:87
>> +    ReleaseSemaphore(m_semaphoreHandle, 1, nullptr);
> 
> These could assert maybe that the result is somehow expected

This ReleaseSemaphore can fail as ERROR_TOO_MANY_POSTS because the maximum count of semaphore is 1.

[Fujii Hironori]

Created attachment 425140 [details]
Patch

[Don Olmstead]

Comment on attachment 425140 [details]
Patch

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

r=me

Just have some comments on whether or not different guards might be more appropriate here.

> Source/WebKit/Platform/IPC/StreamClientConnection.h:105
> +#if PLATFORM(COCOA) || PLATFORM(WIN)

Maybe a HAVE(IPC_SEMAPHORE) or something like that?

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:116
> +#if PLATFORM(COCOA) || PLATFORM(WIN)
>  #include "RemoteGraphicsContextGLProxy.h"
>  #endif

Is there a better guard for this? Maybe an `ENABLE_WEBGL_IN_GPU_PROCESS`

[Fujii Hironori]

Comment on attachment 425140 [details]
Patch

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

Thank you very much.

>> Source/WebKit/Platform/IPC/StreamClientConnection.h:105
>> +#if PLATFORM(COCOA) || PLATFORM(WIN)
> 
> Maybe a HAVE(IPC_SEMAPHORE) or something like that?

Will fix in a follow-up patch.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:116
>>  #endif
> 
> Is there a better guard for this? Maybe an `ENABLE_WEBGL_IN_GPU_PROCESS`

Will consider.

[Fujii Hironori]

Comment on attachment 425140 [details]
Patch

Clearing flags on attachment: 425140

Committed r275463 (236123@main): <https://commits.webkit.org/236123@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/76245142>

[Myles C. Maxfield]

*** Bug 223764 has been marked as a duplicate of this bug. ***