Bug 58054 – REGRESSION (WebKit2): Accelerated CSS animations have a lower framerate than in WebKit1

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://webkit.org/blog-files/3d-transforms/poster-circle.html in WebKit2, and compare to WebKit1

The framerate in WebKit2 is slower than in WebKit1. WebKit1 is running basically at 60fps continuously, while WebKit2 is somewhere closer to 30 or 40fps.

[Adam Roben (:aroben)]

Fixing bug 58052 and/or bug 58053 would likely help with this.

[Adam Roben (:aroben)]

<rdar://problem/9249839>

[Adam Roben (:aroben)]

Kenneth Russell explained to me the strategy that Chrome is using, and I think something similar could work for WebKit2. The basic idea is that the web process would create an HWND that is a child of the WKView HWND. The web process would render into the child HWND directly using D3D. The UI process would be in charge of sizing the child HWND so that it completely covers the WKView HWND. The child HWND would be transparent to mouse events so that WKView's event handling can proceed normally.

[Adam Roben (:aroben)]

Probably the trickiest thing with this approach will be making windowed plugins work correctly. They either need to be children of the GPU window, or be in front of it in the Z-order. And we need to make sure they still get scrolled correctly.

[Adam Roben (:aroben)]

Placing plugins in front of the GPU window in the z-order seems like the simplest solution. That way we don't have to reparent plugins when we enter/exit accelerated compositing mode.

[Simon Fraser (smfr)]

(In reply to comment #5)
> Placing plugins in front of the GPU window in the z-order seems like the simplest solution. That way we don't have to reparent plugins when we enter/exit accelerated compositing mode.

Will this cause any new rendering regressions: failure to be clipped by compositing layers, failure to be overlapped by compositing layers?

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > Placing plugins in front of the GPU window in the z-order seems like the simplest solution. That way we don't have to reparent plugins when we enter/exit accelerated compositing mode.
> 
> Will this cause any new rendering regressions: failure to be clipped by compositing layers, failure to be overlapped by compositing layers?

I don't think so; we clip plugins using window regions based on their render tree geometry, and that should continue to work even if they are in front of the GPU window. I would love to try some test cases if you have them!

[Adam Roben (:aroben)]

I should note that windowed plugins are currently always clipped to a rectangular area. Non-rectangular clipping regions aren't supported at the moment.

[Adam Roben (:aroben)]

(In reply to comment #4)
> And we need to make sure [windowed plugins] still get scrolled correctly.

This is a little tricky to get right. We need to scroll them when the WKCACFView renders to the screen, which can happen on a background thread.

[Adam Roben (:aroben)]

(In reply to comment #9)
> (In reply to comment #4)
> > And we need to make sure [windowed plugins] still get scrolled correctly.
> 
> This is a little tricky to get right. We need to scroll them when the WKCACFView renders to the screen, which can happen on a background thread.

I tried making WKCACFView call ::SetWindowRgn/::DeferWindowPos from its background thread. This causes a deadlock because this causes the background thread to block on the main thread, but the main thread may already be blocked on a mutex that is held by the background thread.

[Adam Roben (:aroben)]

It looks like moving the plugin windows in LayerTreeHostCAWin::contextDidChange (which is called on the main thread) works well enough. There's a slight asynchrony between the plugin windows and the accelerated content, but it's very hard to notice in most cases.

[Adam Roben (:aroben)]

Created attachment 95601 [details]
Move WebView's window geometry updating code to a new class

[Adam Roben (:aroben)]

Created attachment 95606 [details]
Route plugin window geometry updates through the DrawingArea

[WebKit Review Bot]

Attachment 95606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/win/DrawingAreaImplWin.cpp:42:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Created attachment 95617 [details]
Render accelerated content into a web process-owned child HWND

[Jeff Miller]

Comment on attachment 95617 [details]
Render accelerated content into a web process-owned child HWND

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

> Source/WebKit2/Shared/win/CoalescedWindowGeometriesUpdater.h:36
> +enum BringToTopOrNot { BringToTop, DoNotBringToTop };

Should this enum be defined inside the CoalescedWindowGeometriesUpdater class?

[Adam Roben (:aroben)]

Comment on attachment 95617 [details]
Render accelerated content into a web process-owned child HWND

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

>> Source/WebKit2/Shared/win/CoalescedWindowGeometriesUpdater.h:36
>> +enum BringToTopOrNot { BringToTop, DoNotBringToTop };
> 
> Should this enum be defined inside the CoalescedWindowGeometriesUpdater class?

Given how specific this enum is, I don't think there's any danger of its name conflicting with some other use. If some other class wants to use this exact enum, we can move it to a shared header. Leaving the enum outside of the class makes code that uses the enum clearer.

[Adam Roben (:aroben)]

Created attachment 95626 [details]
Render accelerated content into a web process-owned child HWND (Take 2)

Cleaned up WKCACFViewWindow a little.

[Adam Roben (:aroben)]

Created attachment 95632 [details]
Delete a bunch of dead code in DrawingAreaImpl

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

Fixed in r87805, r87806, and r87831
http://trac.webkit.org/changeset/87805
http://trac.webkit.org/changeset/87806
http://trac.webkit.org/changeset/87831

Additional cleanup note required for the fix in r87832
http://trac.webkit.org/changeset/87832

[Adam Roben (:aroben)]

(In reply to comment #21)
> Additional cleanup note required for the fix in r87832
> http://trac.webkit.org/changeset/87832

*not* required...

[Adam Roben (:aroben)]

Build fix in r87834
http://trac.webkit.org/changeset/87834