Bug 186364 - [WinCairo] Enable coordinated graphics
Summary: [WinCairo] Enable coordinated graphics
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 186371 186374 186444 195270 196190
Blocks: 194904
  Show dependency treegraph
 
Reported: 2018-06-06 15:37 PDT by Don Olmstead
Modified: 2019-05-29 23:04 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch (32.22 KB, patch)
2019-02-18 02:10 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (33.71 KB, patch)
2019-02-19 01:40 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (28.69 KB, patch)
2019-02-21 02:25 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.89 KB, patch)
2019-02-21 02:29 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (30.21 KB, patch)
2019-02-26 01:47 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.40 KB, patch)
2019-03-03 18:53 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (25.58 KB, patch)
2019-03-04 17:52 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (31.47 KB, patch)
2019-03-06 01:30 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (30.78 KB, patch)
2019-03-11 02:23 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Synchronous RunLoop::TimerBase::stop patch (2.24 KB, patch)
2019-03-25 23:00 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (31.60 KB, patch)
2019-03-26 00:14 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.95 KB, patch)
2019-05-12 19:58 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (30.75 KB, patch)
2019-05-13 21:55 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (28.51 KB, patch)
2019-05-14 01:28 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.68 KB, patch)
2019-05-15 00:38 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.68 KB, patch)
2019-05-19 22:59 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (27.75 KB, patch)
2019-05-29 23:04 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-06-06 15:37:13 PDT
WinCairo should use USE(COORDINATED_GRAPHICS).
Comment 1 Yoshiaki Jitsukawa 2018-11-13 16:31:57 PST
I have a WIP branch https://github.com/unicodon/webkit/tree/win-cg
Comment 2 Fujii Hironori 2019-02-18 02:10:43 PST
Created attachment 362273 [details]
WIP patch

* Rebased onto ToT
Comment 3 Fujii Hironori 2019-02-19 01:40:13 PST
Created attachment 362377 [details]
WIP patch

* Fix crash of shutting down
* Fix crash of reentering AC mode
Comment 4 Fujii Hironori 2019-02-21 02:25:38 PST
Created attachment 362600 [details]
WIP patch

* Call SetProcessDPIAware in web process
Comment 5 Fujii Hironori 2019-02-21 02:29:29 PST
Created attachment 362601 [details]
WIP patch

* Use AcceleratedSurfaceWin::window instead of setNativeSurfaceHandleForCompositing
Comment 6 Fujii Hironori 2019-02-21 02:30:58 PST
ToDo

* Sometimes animation does't start. https://webkit.org/blog-files/3d-transforms/morphing-cubes.html
* Crashes occasionally.
* Nothing is drawn after some web surfing. Needs to restart.
Comment 7 Fujii Hironori 2019-02-26 01:47:26 PST
Created attachment 362971 [details]
WIP patch

* Added ThreadedCompositor::pauseCompositing to stop CompositingRunLoop when exit AC mode.
* Implement CompositingRunLoop with WorkQueue

ToDo:

* Sometimes animation does't start. https://webkit.org/blog-files/3d-transforms/morphing-cubes.html
* Crashes or freezes occasionally.
Comment 8 Fujii Hironori 2019-02-26 02:29:21 PST
(In reply to Fujii Hironori from comment #7)
> * Sometimes animation does't start.
> https://webkit.org/blog-files/3d-transforms/morphing-cubes.html

I confirmed this issue happens in GTK port, too.
Comment 9 Fujii Hironori 2019-02-26 20:41:58 PST
The crash occured in eglMakeCurrent.
It seems that the previous context was already destructed.

> libGLESv2.dll!gl::ResourceMap<gl::Framebuffer>::assign(unsigned int handle, gl::Framebuffer * resource) Line 169	C++
> libGLESv2.dll!gl::FramebufferManager::setDefaultFramebuffer(gl::Framebuffer * framebuffer) Line 443	C++
> libGLESv2.dll!gl::Context::releaseSurface(const egl::Display * display) Line 574	C++
> libGLESv2.dll!egl::MakeCurrent(void * dpy, void * draw, void * read, void * ctx) Line 518	C++
> libEGL.dll!eglMakeCurrent(void * dpy, void * draw, void * read, void * ctx) Line 93	C++
> WebKit2.dll!WebCore::GLContextEGL::makeContextCurrent() Line 424	C++
> WebKit2.dll!WebKit::ThreadedCompositor::createGLContext() Line 94	C++
> WebKit2.dll!WebKit::ThreadedCompositor::{ctor}::__l2::<lambda>() Line 77	C++
> WebKit2.dll!WTF::Function<void __cdecl(void)>::CallableWrapper<void <lambda>(void) >::call() Line 102	C++
> WebKit2.dll!WTF::Function<void __cdecl(void)>::operator()() Line 58	C++
> WebKit2.dll!WebKit::CompositingRunLoop::performTaskSync::__l2::<lambda>() Line 126	C++
> WebKit2.dll!WTF::Function<void __cdecl(void)>::CallableWrapper<void <lambda>(void) >::call() Line 102	C++
> WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 58	C++
> WTF.dll!WTF::WorkQueue::performWorkOnRegisteredWorkThread() Line 62	C++
> WTF.dll!WTF::WorkQueue::workThreadCallback(void * context) Line 44	C++
> [External Code]	

Windows port WorkQueue is using CreateTimerQueueTimer with WT_EXECUTEINTIMERTHREAD.
So, WorkQueue jobs are dispatched in random timer threads.

There are two possible solutions.
1. Execute all CompositingRunLoop jobs in a single thread
2. Clear current context by invoking eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT) every time the end of jobs.
Comment 10 Fujii Hironori 2019-03-03 18:53:25 PST
Created attachment 363475 [details]
WIP patch

* Rebased onto ToT
* Morphing Power Cubes animation works better than before
* No crash in eglMakeCurrent

ToDo:
* Nothing is drawn after some web surfing. Needs to restart.
Comment 11 Fujii Hironori 2019-03-03 22:34:30 PST
Remaining Issues:

* ~CompositingRunLoop() calls RunLoop::stop. Windows port's RunLoop::stop terminates the current thread's RunLoop, and WebProcess exits.
* After respawning WebProcess, WebPage has 0x0 size. Nothing drawn.
* It seems ThreadedCompositor::pauseCompositing, which is added by me, doesn't work as expected.
Comment 12 Fujii Hironori 2019-03-04 17:52:10 PST
Created attachment 363575 [details]
WIP patch

* Rebased onto ToT. It works nicely.

Remaining Issues:

* After respawning WebProcess, WebPage has 0x0 size. Nothing drawn.
Comment 13 Carlos Garcia Campos 2019-03-05 02:03:22 PST
Comment on attachment 363575 [details]
WIP patch

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

> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:70
>      // Make sure the RunLoop is stopped after the CompositingRunLoop, because m_updateTimer has a reference.

You should remove this comment too.

> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:72
> -    RunLoop::main().dispatch([runLoop = makeRef(*m_runLoop)] {
> -        runLoop->stop();
> -        runLoop->dispatch([] {
> -            RunLoop::current().stop();
> -        });
> +    m_runLoop->dispatch([] {
> +        RunLoop::current().stop();

Please, file a new bug report for this change.
Comment 14 Fujii Hironori 2019-03-05 02:53:28 PST
(In reply to Carlos Garcia Campos from comment #13)
> Please, file a new bug report for this change.

Thank you for the comment. Will do. However, I should fix Windows port's RunLoop::stop issue before it. I found there is Yusuke's WIP patch in Bug 181151.
Comment 15 Fujii Hironori 2019-03-06 01:30:04 PST
Created attachment 363736 [details]
WIP patch

* Almost all WebAnimation tests were timing out. Added dummy DisplayRefreshMonitorWin to fix them.

Remaining Issues:

* After respawning WebProcess, WebPage has 0x0 size. Nothing drawn.
* Some tests are failing due to an assertion failure in Nicosia::Scene::accessState
  ASSERTION FAILED: state.id == m_nicosia.state.id
* Needs to check other new test failures
Comment 16 Fujii Hironori 2019-03-11 02:23:47 PDT
Created attachment 364241 [details]
WIP patch

* Rebased onto ToT
Comment 17 Fujii Hironori 2019-03-13 02:55:56 PDT
* Some compositing LayoutTests are randomly failing with "ASSERTION FAILED: state.id == m_nicosia.state.id"
* Some compositing LayoutTests are randomly timing out
Comment 18 Fujii Hironori 2019-03-24 18:50:42 PDT
(In reply to Fujii Hironori from comment #17)
> * Some compositing LayoutTests are randomly failing with "ASSERTION FAILED:
> state.id == m_nicosia.state.id"

Filed it in Bug 196190.
Comment 19 Fujii Hironori 2019-03-25 03:48:32 PDT
(In reply to Fujii Hironori from comment #17)
> * Some compositing LayoutTests are randomly timing out

ref tests and pixel tests are randomly timing out because output pixel images aren't generated. Nothing drawn in WKTR WebView in such cases.
Comment 20 Fujii Hironori 2019-03-25 23:00:03 PDT
Created attachment 365947 [details]
Synchronous RunLoop::TimerBase::stop patch

This patch makes RunLoop::TimerBase::stop synchronous as well as GTK port.
RunLoop::TimerBase::stop blocks while the timer callback is being called back.
Unfortunately, this patch doesn't address the timing out issue.
I don't know why GTK port does'n have the timing out issue.
Comment 21 Fujii Hironori 2019-03-25 23:59:37 PDT
It's just a simple bug. It is blocked in TestInvocation::dumpResults.
WKPageForceRepaint doesn't work as expected becasue ThreadedCompositor::forceRepaint is not implemented yet.
Comment 22 Fujii Hironori 2019-03-26 00:14:05 PDT
Created attachment 365949 [details]
WIP patch

* Fixed ThreadedCompositor::forceRepaint for the timing out issues
Comment 23 Carlos Garcia Campos 2019-03-26 02:50:10 PDT
Comment on attachment 365949 [details]
WIP patch

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

> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:191
> -#if PLATFORM(GTK)
> +#if !PLATFORM(WPE)

This is exactly what I was going to suggest you to do :-)

> Source/WebKit/WebProcess/WebPage/WebPage.h:960
> -#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> +#if PLATFORM(WIN) && USE(TEXTURE_MAPPER_GL)

Why do you remove GTK?

> Source/WebKit/WebProcess/WebPage/WebPage.h:1640
> -#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> +#if PLATFORM(WIN) && USE(TEXTURE_MAPPER_GL)

Ditto.

> Source/WebKit/WebProcess/WebPage/win/AcceleratedSurfaceWin.cpp:56
> +void AcceleratedSurfaceWin::initialize()
> +{
> +}
> +
> +void AcceleratedSurfaceWin::finalize()
> +{
> +}

These are not pure virtual, you don't need to override them.

> Source/WebKit/WebProcess/WebPage/win/AcceleratedSurfaceWin.cpp:70
> +void AcceleratedSurfaceWin::willRenderFrame()
> +{
> +}

Ditto.
Comment 24 Fujii Hironori 2019-03-26 02:59:19 PDT
Comment on attachment 365949 [details]
WIP patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.h:960
>> +#if PLATFORM(WIN) && USE(TEXTURE_MAPPER_GL)
> 
> Why do you remove GTK?

m_nativeWindowHandle is simply an unused member variable in GTK port.

>> Source/WebKit/WebProcess/WebPage/win/AcceleratedSurfaceWin.cpp:56
>> +}
> 
> These are not pure virtual, you don't need to override them.

Good point. Will fix.
Comment 25 Fujii Hironori 2019-05-12 19:58:39 PDT
Created attachment 369688 [details]
WIP patch

* Rebased onto ToT
* Addressed review feedbacks
* A new assertion failure has been introduced, which occurs at WebProcess termination.

setDefaultFramebuffer(8196): 	! Assert failed in setDefaultFramebuffer(8196): mCurrentSurface == nullptrException thrown at 0x00007FFE6A1B40C4 (libGLESv2.dll) in WebKitWebProcess.exe: 0xC000001D: Illegal Instruction.
Unhandled exception at 0x00007FFE6A1B40C4 (libGLESv2.dll) in WebKitWebProcess.exe: 0xC000001D: Illegal Instruction.

> libGLESv2.dll!gl::LogMessage::~LogMessage() Line 171	C++
> libGLESv2.dll!gl::Context::setDefaultFramebuffer(egl::Surface * surface) Line 8198	C++
> libGLESv2.dll!gl::Context::makeCurrent(egl::Display * display, egl::Surface * surface) Line 608	C++
> libGLESv2.dll!egl::Display::makeCurrent(const egl::Thread * thread, egl::Surface * drawSurface, egl::Surface * readSurface, gl::Context * context) Line 945	C++
> libGLESv2.dll!egl::Display::destroyContext(const egl::Thread * thread, gl::Context * context) Line 1018	C++
> libGLESv2.dll!egl::Display::terminate(const egl::Thread * thread) Line 600	C++
> libGLESv2.dll!EGL_Terminate(void * dpy) Line 111	C++
> libEGL.dll!eglTerminate(void * dpy) Line 209	C++
> WebKit2.dll!WebCore::PlatformDisplay::terminateEGLDisplay() Line 235	C++
> WebKit2.dll!WebCore::PlatformDisplay::shutDownEglDisplays() Line 241	C++
> [External Code]	
> WebKit2.dll!WebKit::callExit(IPC::Connection *) Line 168	C++
> WebKit2.dll!IPC::Connection::connectionDidClose() Line 841	C++
> WebKit2.dll!IPC::Connection::readEventHandler() Line 156	C++
> WebKit2.dll!IPC::Connection::invokeReadEventHandler::<unnamed-tag>::operator()() Line 235	C++
> WebKit2.dll!WTF::Detail::CallableWrapper<`lambda at ..\..\Source\WebKit\Platform\IPC\win\ConnectionWin.cpp:233:33',void>::call() Line 52	C++
> WTF.dll!WTF::Function<void ()>::operator()() Line 79	C++
> WTF.dll!WTF::WorkQueue::performWorkOnRegisteredWorkThread() Line 61	C++
> WTF.dll!WTF::WorkQueue::workThreadCallback(void * context) Line 44	C++
> [External Code]
Comment 26 Fujii Hironori 2019-05-13 00:50:57 PDT
(In reply to Fujii Hironori from comment #25)
> * A new assertion failure has been introduced, which occurs at WebProcess
> termination.

This issue?
https://bugs.chromium.org/p/angleproject/issues/detail?id=3414
Comment 27 Fujii Hironori 2019-05-13 21:55:50 PDT
Created attachment 369820 [details]
WIP patch

(In reply to Fujii Hironori from comment #26)
> (In reply to Fujii Hironori from comment #25)
> > * A new assertion failure has been introduced, which occurs at WebProcess
> > termination.
> 
> This issue?
> https://bugs.chromium.org/p/angleproject/issues/detail?id=3414

Not relevant.

This is a similar issue with comment 9.
It seems that ANGLE need to unbind the context explicitly by doing eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT).

* Added GLContextEGL::unmakeContextCurrent

I'm going to file a new bug ticket for the part.
Comment 28 Fujii Hironori 2019-05-13 23:45:53 PDT
(In reply to Fujii Hironori from comment #27)
> It seems that ANGLE need to unbind the context explicitly by doing
> eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT).
> 
> * Added GLContextEGL::unmakeContextCurrent

Oh, no. It turns out this patch doesn't solve the problem. It just reduces the probability.
It is inevitable WebKit::callExit is called during the rendering.
Comment 29 Fujii Hironori 2019-05-14 01:28:10 PDT
Created attachment 369827 [details]
WIP patch

* Quick ANGLE hack to avoid the assertion failure (Needs to consult upstream ANGLE.)
Comment 30 Fujii Hironori 2019-05-15 00:36:50 PDT
EGL eglTerminate spec says:

> If contexts or surfaces created with respect to dpy are current (see section 3.7.3) to any thread, then they are not actually destroyed while they remain current.

https://www.khronos.org/registry/EGL/specs/eglspec.1.4.pdf

This seems ANGLE bug.
Comment 31 Fujii Hironori 2019-05-15 00:38:39 PDT
Created attachment 369934 [details]
WIP patch

* Removed the ANGLE hack
* Do not register an atexit handler to cleanup EGL display for Windows
* Condition out an assertion in ~PlatformDisplay for Windows
Comment 32 Don Olmstead 2019-05-15 09:27:55 PDT
Comment on attachment 369934 [details]
WIP patch

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

Looks good. In the final patch you should do if (ENABLE_WEBKIT) around the USE and ENABLE flags until we completely turn off legacy

> Source/WebKit/WebProcess/WebPage/WebPage.h:969
> +#if PLATFORM(WIN) && USE(TEXTURE_MAPPER_GL)

Good catch that this was dead code in GTK.
Comment 33 Don Olmstead 2019-05-15 10:24:01 PDT
I see Web Inspector crashing at startup with this patch. We're doing some work with remote inspector on WinCairo so we can still land this but its something to note. WebGL demos seemed to work fine but it was reporting 30 fps as the framerate.
Comment 34 Fujii Hironori 2019-05-15 18:53:12 PDT
(In reply to Don Olmstead from comment #32)
> Looks good. In the final patch you should do if (ENABLE_WEBKIT) around the
> USE and ENABLE flags until we completely turn off legacy

I plan to take more aggressive approach.
I'm going to disable AC in WinCairo WK1 and remove GraphicsLayerTextureMapper.

If someone want to use AC in WK1 in the future and aims to maintain, s/he should port CoordinatedGraphicsLayer to WinCairo WK1.
Comment 35 Fujii Hironori 2019-05-15 18:57:35 PDT
(In reply to Don Olmstead from comment #33)
> I see Web Inspector crashing at startup with this patch. 

Thanks for testing, but I don't see any crash.
Comment 36 Fujii Hironori 2019-05-15 19:00:24 PDT
(In reply to Don Olmstead from comment #33)
> WebGL demos seemed to work fine but it was reporting 30
> fps as the framerate.

I don't know the reason, but my DisplayRefreshMonitorWin implementation is supper sloppy at the moment.
Comment 37 Fujii Hironori 2019-05-19 22:59:39 PDT
Created attachment 370239 [details]
WIP patch

* Rebased onto ToT
Comment 38 Fujii Hironori 2019-05-29 23:04:59 PDT
Created attachment 370924 [details]
WIP patch

* Rebased onto ToT