Bug 45567

Summary: Get accelerated compositing working with WebKit2 on Windows
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: WebKit2Assignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, buildbot, cmarrin, noam, simon.fraser, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 53805, 57046, 57060, 57388, 57598, 57599, 57606, 57774, 57792    
Bug Blocks:    
Attachments:
Description Flags
Baby steps: stub out some LayerBackedDrawingArea methods for Windows
none
Patch
none
Make accelerated compositing work in WebKit2 on Windows andersca: review+

Description Simon Fraser (smfr) 2010-09-10 15:35:06 PDT
Need to hook up compositing layers between the web- and UI processes on Windows.
<rdar://problem/8147830>
Comment 1 Simon Fraser (smfr) 2010-09-10 15:43:42 PDT
Created attachment 67249 [details]
Baby steps: stub out some LayerBackedDrawingArea methods for Windows
Comment 2 WebKit Review Bot 2010-09-10 15:47:03 PDT
Attachment 67249 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/win/LayerBackedDrawingAreaProxyWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/WebPage/win/LayerBackedDrawingAreaWin.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/WebPage/win/LayerBackedDrawingAreaWin.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2010-09-10 16:05:51 PDT
Comment on attachment 67249 [details]
Baby steps: stub out some LayerBackedDrawingArea methods for Windows

Need to add some more files to the vcproj
Comment 4 Simon Fraser (smfr) 2010-11-16 16:59:25 PST
Created attachment 74064 [details]
Patch
Comment 5 Simon Fraser (smfr) 2010-11-16 16:59:59 PST
I see some unwanted changes in the vcproj. I'll fix before landing.
Comment 6 WebKit Review Bot 2010-11-16 17:01:30 PST
Attachment 74064 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/UIProcess/LayerBackedDrawingAreaProxy.cpp', u'WebKit2/UIProcess/win/LayerBackedDrawingAreaProxyWin.cpp', u'WebKit2/UIProcess/win/WebView.cpp', u'WebKit2/UIProcess/win/WebView.h', u'WebKit2/WebProcess/WebPage/LayerBackedDrawingArea.cpp', u'WebKit2/WebProcess/WebPage/win/LayerBackedDrawingAreaWin.cpp', u'WebKit2/win/WebKit2.vcproj']" exit_code: 1
WebKit2/UIProcess/win/WebView.cpp:664:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Simon Fraser (smfr) 2010-11-16 17:11:24 PST
Comment on attachment 74064 [details]
Patch

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

> WebKit2/UIProcess/win/WebView.cpp:672
> +        case DrawingAreaProxy::ChunkedUpdateDrawingAreaType: {
> +            newDrawingArea = ChunkedUpdateDrawingAreaProxy::create(this);
> +            break;
> +        }
> +        case DrawingAreaProxy::LayerBackedDrawingAreaType: {
> +            newDrawingArea = LayerBackedDrawingAreaProxy::create(this);
> +            break;

If these proxies took a cross-platform object, this whole function could be in WebPageProxy.
Comment 8 Simon Fraser (smfr) 2010-11-17 09:25:43 PST
Comment on attachment 74064 [details]
Patch

http://trac.webkit.org/changeset/72212
Comment 9 Adam Roben (:aroben) 2011-01-11 15:01:54 PST
I talked with Anders, Simon, and Chris about this a little bit today. They told me about the changes Anders is making to do compositing in the web process into a shared surface on Mac, and for the UI process to handle actually displaying that surface on screen. We also talked about three approaches for Windows:

1) Proxy all GraphicsLayer calls over to the UI process, and do the compositing there. Layer contents would be rendered into shared bitmaps, then copied to VRAM in the UI process by Core Animation.
2) Use the IDirect3D9Ex APIs that were added in Vista to make CA render to a shared texture in the web process, then let the UI process put that texture on screen.
3) Use the IDirect3D9 APIs to make CA render to a shared bitmap in the web process, then let the UI process put that bitmap on screen.

A stumbling block with (1) is how to make <video> work. <video> currently uses image queues, which won't work cross-process. So that option seems unusable for now.

(2) is probably the most similar to the proposed Mac design. It will require some changes in Core Animation for it to work, however. It also won't work on Windows XP.

(3) will work on Windows XP and won't require any changes in Core Animation, but might have poor performance relative to (2) due to copying from VRAM to system memory on every frame.

Simon did some experiments with (2) and (3) and they both seem doable. He said that his initial performance tests of (3) showed no noticeable difference from Safari 5.0.3.

I think the best way forward is to get (3) working first. If performance seems degraded, we can then work on making (2) work on Vista and newer, and decide what to do about XP.
Comment 10 Adam Roben (:aroben) 2011-03-08 14:58:18 PST
(In reply to comment #9)
> 3) Use the IDirect3D9 APIs to make CA render to a shared bitmap in the web process, then let the UI process put that bitmap on screen.

It would be convenient if we could use a CAImageQueue to transport the images cross-process and get the timing correct. But unfortunately CAImageQueue doesn't work cross-process on Windows.
Comment 11 Adam Roben (:aroben) 2011-03-08 14:59:44 PST
Seems like we'll need to have the web process render a little bit into the future, if possible, to account for the delay between rendering in the web process and displaying in the UI process. Otherwise we could get audio/video sync issues.
Comment 12 Adam Roben (:aroben) 2011-03-08 15:20:25 PST
Perhaps rendering future frames should be done after the initial patch. It sounds tricky!
Comment 13 Adam Roben (:aroben) 2011-03-16 14:21:07 PDT
I just talked to Anders about the copy-into-bitmap approach. Here's what we came up with:

1) Add a "render into a bitmap" mode to WKCACFView. This mode consists of three parts:
  a) A way to put a WKCACFView into this mode
  b) A call to tell WKCACFView to render into a specific HBITMAP
  c) A callback that WKCACFView will call when rendering is complete
WKCACFView will not render automatically when in this mode. It will render a single frame when a bitmap is given to it. Maybe the callback in (c) should be passed the next time it thinks it needs to render.

2) Add a way for LayerTreeHost to tell DrawingAreaImpl to send a given ShareableBitmap in an Update message to the UI process. We'll use this to send the bitmap that WKCACFView rendered into to the UI process.

3) Add a way to tell DrawingAreaImpl not to send Enter/ExitAcceleratedCompositing messages to the UI process. With the above rendering model, the UI process won't need to know that we're in accelerated compositing mode at all.
Comment 14 Adam Roben (:aroben) 2011-03-27 14:48:37 PDT
I have things working pretty well. Jotting down some remaining bugs I've noticed:

1) Scrolling on <http://neography.com/experiment/circles/solarsystem/> is laggy.
2) Animations don't restart when resumePainting is called.
Comment 15 Adam Roben (:aroben) 2011-03-27 14:50:10 PDT
3) The whole view goes blank during resizing.
Comment 16 Adam Roben (:aroben) 2011-03-27 14:57:00 PDT
(In reply to comment #15)
> 3) The whole view goes blank during resizing.

This is happening because the LayerTreeHost isn't getting to participate in the UpdateBackingStoreState message correctly. We need to wait for the LayerTreeHost to render the next frame before sending back the DidUpdateBackingStoreState message.
Comment 17 Adam Roben (:aroben) 2011-03-28 15:32:21 PDT
I was having trouble getting DrawingAreaImpl to play nicely with an asynchronously-painting LayerTreeHost. I decided to make LayerTreeHost paint synchronously. This has the advantage of matching the way we paint in non-composited mode (so DrawingAreaImpl will require few changes), but the disadvantage of blocking the main thread while we wait for the readback from the GPU to happen.

This seems to be working pretty well, though the framerate isn't quite as high as in WK1. However, on Windows 7 everything is being painted black in composited mode. I believe this is due to a difference in behavior between D3D on XP and on 7: GetRenderTargetData is filling the alpha channel with 255 on XP but 0 on 7.
Comment 18 Adam Roben (:aroben) 2011-03-28 15:51:31 PDT
Using synchronous painting fixes bugs 1 and 3 mentioned in comment 14 and comment 15.
Comment 19 Adam Roben (:aroben) 2011-03-28 16:04:32 PDT
(In reply to comment #17)
> However, on Windows 7 everything is being painted black in composited mode. I believe this is due to a difference in behavior between D3D on XP and on 7: GetRenderTargetData is filling the alpha channel with 255 on XP but 0 on 7.

Changing ShareableBitmap::paint to use ::StretchDIBits instead of CG calls fixes this problem. GDI ignores the alpha channel completely, so it doesn't matter that it's 0.
Comment 20 Adam Roben (:aroben) 2011-03-28 16:24:51 PDT
I guess another option would be to use kCGImageAlphaNoneSkipFirst in ShareableBitmap::createGraphicsContext.
Comment 21 Adam Roben (:aroben) 2011-03-28 16:55:02 PDT
::StretchDIBits has its own problems. It doesn't support transparency, and it has a wacky bug that needs to be worked around: <http://wiki.allegro.cc/index.php?title=StretchDIBits>
Comment 22 Adam Roben (:aroben) 2011-03-30 23:01:39 PDT
(In reply to comment #14)
> 2) Animations don't restart when resumePainting is called.

I'm still running into this. In addition, the web process and UI process get into an Update/DidUpdate loop.
Comment 23 Adam Roben (:aroben) 2011-04-05 08:18:43 PDT
Created attachment 88238 [details]
Make accelerated compositing work in WebKit2 on Windows
Comment 24 Anders Carlsson 2011-04-05 08:34:33 PDT
Comment on attachment 88238 [details]
Make accelerated compositing work in WebKit2 on Windows

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

> Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.cpp:190
> +void LayerTreeHostCAWin::sizeDidChange(const WebCore::IntSize& newSize)

No need for WebCore:: here.
Comment 25 Build Bot 2011-04-05 08:54:05 PDT
Attachment 88238 [details] did not build on win:
Build output: http://queues.webkit.org/results/8336326
Comment 26 Adam Roben (:aroben) 2011-04-05 09:43:07 PDT
(In reply to comment #25)
> Attachment 88238 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8336326

Looks like the build failed because the patch for bug 57792 hasn't landed yet.
Comment 27 Adam Roben (:aroben) 2011-04-05 11:03:09 PDT
Committed r82960: <http://trac.webkit.org/changeset/82960>
Comment 28 Adam Roben (:aroben) 2011-04-05 11:45:27 PDT
(In reply to comment #14)
> 2) Animations don't restart when resumePainting is called.

This is now bug 57868.