Bug 86037 - [GTK] Add an accelerated compositing implementation for WebKit2
: [GTK] Add an accelerated compositing implementation for WebKit2
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Martin Robinson
:
Depends on: 80509
Blocks: 74087 87053
  Show dependency treegraph
 
Reported: 2012-05-09 16:43 PDT by Martin Robinson
Modified: 2012-06-13 17:13 PDT (History)
8 users (show)

See Also:


Attachments
Patch (32.67 KB, patch)
2012-05-09 17:04 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Enable the layer flush scheduling by default (32.67 KB, patch)
2012-05-09 22:28 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch rebased and now using GLib timer to avoid starvation due to event handling (33.35 KB, patch)
2012-05-23 13:41 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
The same patch minus a GSource bomb (32.11 KB, patch)
2012-05-24 11:48 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Remove one more GSource leak (33.59 KB, patch)
2012-05-29 17:14 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Stacktrace using 20120529 patch (8.81 KB, text/plain)
2012-05-31 09:14 PDT, José Dapena Paz
no flags Details
Patch (33.55 KB, patch)
2012-06-08 16:53 PDT, Martin Robinson
alex: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-05-09 16:43:43 PDT
We should add an accelerated compositing implementation for WebKit that uses the OpenGL TextureMapper by default and later add a fallback to the ImageBuffer TextureMapper.
Comment 1 Martin Robinson 2012-05-09 17:04:15 PDT
Created attachment 141054 [details]
Patch
Comment 2 Martin Robinson 2012-05-09 22:28:44 PDT
Created attachment 141086 [details]
Enable the layer flush scheduling by default
Comment 3 xpert 2012-05-09 23:03:41 PDT

1. As per guidelines of using refptr , passrefptr is as for this case

If lifetime of any object fixed and scope is also known then use refptr instead passrefptr.
Comment 4 Martin Robinson 2012-05-09 23:37:52 PDT
(In reply to comment #3)
> 
> 1. As per guidelines of using refptr , passrefptr is as for this case
> 
> If lifetime of any object fixed and scope is also known then use refptr instead passrefptr.

This patch uses PassRefPtr for a factory method, which is precisely a time when it should be used. Unless I'm misunderstanding your comment...
Comment 5 Carlos Garcia Campos 2012-05-10 00:40:44 PDT
Comment on attachment 141086 [details]
Enable the layer flush scheduling by default

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:322
> +#if USE(TEXTURE_MAPPER_GL) && defined(GDK_WINDOWING_X11)
> +    GdkWindow* gdkWindow = gtk_widget_get_window(widget);
> +    if (gdkWindow && gdk_window_has_native(gdkWindow))
> +        WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->widgetMapped(GDK_WINDOW_XID(gdkWindow));
> +#endif

Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here. 
Do we need to send the window id every time the widget is mapped? The window is created in realize() and it never changes. Instead of sending it when mapped, we could send it when the page is created as part for the page creation parameters. We could add a nativeWindowHandle() method to our PageClient that realizes the widget if it's not yet realized and returns the window id. This way we also save a new IPC message.

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp:43
> +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> +#include "gtk/LayerTreeHostGtk.h"
> +#endif

WebPage/gtk is already in the includes path, so you can just #include "LayerTreeHostGtk.h"
Comment 6 Martin Robinson 2012-05-10 08:35:17 PDT
(In reply to comment #5)
> (From update of attachment 141086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141086&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:322
> > +#if USE(TEXTURE_MAPPER_GL) && defined(GDK_WINDOWING_X11)
> > +    GdkWindow* gdkWindow = gtk_widget_get_window(widget);
> > +    if (gdkWindow && gdk_window_has_native(gdkWindow))
> > +        WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->widgetMapped(GDK_WINDOW_XID(gdkWindow));
> > +#endif
> 
> Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here. 
> Do we need to send the window id every time the widget is mapped? The window is created in realize() and it never changes. Instead of sending it when mapped, we could send it when the page is created as part for the page creation parameters. We could add a nativeWindowHandle() method to our PageClient that realizes the widget if it's not yet realized and returns the window id. This way we also save a new IPC message.

The reason for doing it in map, is that later I may send another message in unmap. This could allow us to stop rendering or slow down the animation speed when then widget isn't on the screen.

I also cannot find whether it is valid to render to a GLX window while the wrapped X window is invalid. The introductory example wait until the window is map to make the context active: http://www.opengl.org/sdk/docs/man/xhtml/glXIntro.xml

> > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp:43
> > +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> > +#include "gtk/LayerTreeHostGtk.h"
> > +#endif
> 
> WebPage/gtk is already in the includes path, so you can just #include "LayerTreeHostGtk.h"

Okay. Will fix!
Comment 7 Carlos Garcia Campos 2012-05-10 08:46:42 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 141086 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141086&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:322
> > > +#if USE(TEXTURE_MAPPER_GL) && defined(GDK_WINDOWING_X11)
> > > +    GdkWindow* gdkWindow = gtk_widget_get_window(widget);
> > > +    if (gdkWindow && gdk_window_has_native(gdkWindow))
> > > +        WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->widgetMapped(GDK_WINDOW_XID(gdkWindow));
> > > +#endif
> > 
> > Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here. 
> > Do we need to send the window id every time the widget is mapped? The window is created in realize() and it never changes. Instead of sending it when mapped, we could send it when the page is created as part for the page creation parameters. We could add a nativeWindowHandle() method to our PageClient that realizes the widget if it's not yet realized and returns the window id. This way we also save a new IPC message.
> 
> The reason for doing it in map, is that later I may send another message in unmap. This could allow us to stop rendering or slow down the animation speed when then widget isn't on the screen.

But the window id will still be the same, so we could send the window id once when creating the page and then simply send notification messages Mapped/Unmapped to the web process when the widget is mapped/unmapped

> I also cannot find whether it is valid to render to a GLX window while the wrapped X window is invalid. The introductory example wait until the window is map to make the context active: http://www.opengl.org/sdk/docs/man/xhtml/glXIntro.xml

I have no idea about OpenGL, so I can't help here :-P
Comment 8 Martin Robinson 2012-05-10 08:54:58 PDT
Comment on attachment 141086 [details]
Enable the layer flush scheduling by default

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

>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:322
>>>> +#endif
>>> 
>>> Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here. 
>>> Do we need to send the window id every time the widget is mapped? The window is created in realize() and it never changes. Instead of sending it when mapped, we could send it when the page is created as part for the page creation parameters. We could add a nativeWindowHandle() method to our PageClient that realizes the widget if it's not yet realized and returns the window id. This way we also save a new IPC message.
>> 
>> The reason for doing it in map, is that later I may send another message in unmap. This could allow us to stop rendering or slow down the animation speed when then widget isn't on the screen.
>> 
>> I also cannot find whether it is valid to render to a GLX window while the wrapped X window is invalid. The introductory example wait until the window is map to make the context active: http://www.opengl.org/sdk/docs/man/xhtml/glXIntro.xml
> 
> But the window id will still be the same, so we could send the window id once when creating the page and then simply send notification messages Mapped/Unmapped to the web process when the widget is mapped/unmapped

The idea was to clear the native window handle on unmapping and reinitialize it on mapping, using only two IPC messages instead of one.
Comment 9 Martin Robinson 2012-05-23 13:41:26 PDT
Created attachment 143639 [details]
Patch rebased and now using GLib timer to avoid starvation due to event handling
Comment 10 WebKit Review Bot 2012-05-23 13:46:53 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 11 Martin Robinson 2012-05-24 11:48:59 PDT
Created attachment 143864 [details]
The same patch minus a GSource bomb
Comment 12 Martin Robinson 2012-05-24 19:00:49 PDT
Comment on attachment 143864 [details]
The same patch minus a GSource bomb

Clearing review flag, because this one still needs a bit of work.
Comment 13 Martin Robinson 2012-05-29 17:14:07 PDT
Created attachment 144650 [details]
Remove one more GSource leak
Comment 14 Carlos Garcia Campos 2012-05-30 00:01:33 PDT
Comment on attachment 144650 [details]
Remove one more GSource leak

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:351
> +    GdkWindow* gdkWindow = gtk_widget_get_window(widget);
> +    if (gdkWindow && gdk_window_has_native(gdkWindow))

Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:352
> +        WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->widgetMapped(GDK_WINDOW_XID(gdkWindow));

No need to cast again, just webViewBase->priv

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp:43
> +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> +#include "gtk/LayerTreeHostGtk.h"
> +#endif

WebPage/gtk is already in the includes path, so you can just #include "LayerTreeHostGtk.h"
Comment 15 Martin Robinson 2012-05-30 08:07:29 PDT
Comment on attachment 144650 [details]
Remove one more GSource leak

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:351
>> +    if (gdkWindow && gdk_window_has_native(gdkWindow))
> 
> Widgets are realized before map is emitted, so gtk_widget_get_window() will never return NULL here.

Great. I can just change this to an ASSERT.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:352
>> +        WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->widgetMapped(GDK_WINDOW_XID(gdkWindow));
> 
> No need to cast again, just webViewBase->priv

Okay.

>> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp:43
>> +#endif
> 
> WebPage/gtk is already in the includes path, so you can just #include "LayerTreeHostGtk.h"

Okay.
Comment 16 José Dapena Paz 2012-05-31 09:14:07 PDT
Created attachment 145101 [details]
Stacktrace using 20120529 patch

This trace shows a crash due to an assert in FrameView. From gdb session needsLayout() is true because m_layoutTimer is active.

The use case:
* Wk2 with debug
* Run minibrowser with webgl and opengl AC enabled, plugins disabled.
* Go to http://www.chromeexperiments.com/detail/bvh-player/?f=webgl and launch the demo
* The new window appears, and crash happens after this.
Comment 17 Martin Robinson 2012-06-08 16:53:51 PDT
Created attachment 146664 [details]
Patch
Comment 18 Martin Robinson 2012-06-08 16:59:20 PDT
The latest patch fixes both issues mentioned above, I believe.
Comment 19 Philippe Normand 2012-06-08 17:17:49 PDT
Is a clean build required? http://queues.webkit.org/patch/146664
Comment 20 Martin Robinson 2012-06-08 17:25:46 PDT
(In reply to comment #19)
> Is a clean build required? http://queues.webkit.org/patch/146664

It looks like the bots need a clean build because of another patch committed to master.
Comment 21 Alejandro G. Castro 2012-06-12 10:02:02 PDT
Comment on attachment 146664 [details]
Patch

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

Looks good to me, I've tested and it works great :), let's try it! I've added some nits.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:547
> +#if PLATFORM(GTK)
> +    uint64_t nativeWindowHandle() { return m_nativeWindowHandle; }
> +#endif

Why aren't we protecting here with the USE(TEXTURE_MAPPER_GL) define?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:765
> +    // Our view's window in the UI process.
> +    uint64_t m_nativeWindowHandle;

Same here.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.h:74
> +    // GraphicsLayerClient

Dot at the end.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.h:105
> +    // The root layer.
> +    OwnPtr<WebCore::GraphicsLayer> m_rootLayer;

Probably we can remove this comment, I think the variable name is correctly saying what it is. I think there was some discussion at some point in the list about these comments, do not recall the conclusion though :). In case you decide to remove it there are others like this in the file.
Comment 22 Martin Robinson 2012-06-13 14:10:01 PDT
(In reply to comment #21)

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:547
> > +#if PLATFORM(GTK)
> > +    uint64_t nativeWindowHandle() { return m_nativeWindowHandle; }
> > +#endif
> 
> Why aren't we protecting here with the USE(TEXTURE_MAPPER_GL) define?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:765
> > +    // Our view's window in the UI process.
> > +    uint64_t m_nativeWindowHandle;
> 
> Same here.

I'll add the TEXTURE_MAPPER_GL define to these locations.

> > Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.h:74
> > +    // GraphicsLayerClient
> 
> Dot at the end.

Skipping this since the comment isn't a sentence. :)
 
> > Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.h:105
> > +    // The root layer.
> > +    OwnPtr<WebCore::GraphicsLayer> m_rootLayer;
> 
> Probably we can remove this comment, I think the variable name is correctly saying what it is. I think there was some discussion at some point in the list about these comments, do not recall the conclusion though :). In case you decide to remove it there are others like this in the file.

Okay. I copied this kind of commenting from the CA version of the file, but I agree they don't add much  beyond the naming.
Comment 23 Martin Robinson 2012-06-13 17:13:56 PDT
Committed r120262: <http://trac.webkit.org/changeset/120262>