Bug 86037 - [GTK] Add an accelerated compositing implementation for WebKit2
: [GTK] Add an accelerated compositing implementation for WebKit2
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 80509
: 74087 87053
  Show dependency treegraph
 
Reported: 2012-05-09 16:43 PST by
Modified: 2012-06-13 17:13 PST (History)


Attachments
Patch (32.67 KB, patch)
2012-05-09 17:04 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Enable the layer flush scheduling by default (32.67 KB, patch)
2012-05-09 22:28 PST, Martin Robinson
no flags Review Patch | 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 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
The same patch minus a GSource bomb (32.11 KB, patch)
2012-05-24 11:48 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Remove one more GSource leak (33.59 KB, patch)
2012-05-29 17:14 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Stacktrace using 20120529 patch (8.81 KB, text/plain)
2012-05-31 09:14 PST, José Dapena Paz
no flags Details
Patch (33.55 KB, patch)
2012-06-08 16:53 PST, Martin Robinson
alex: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-09 16:43:43 PST
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 From 2012-05-09 17:04:15 PST -------
Created an attachment (id=141054) [details]
Patch
------- Comment #2 From 2012-05-09 22:28:44 PST -------
Created an attachment (id=141086) [details]
Enable the layer flush scheduling by default
------- Comment #3 From 2012-05-09 23:03:41 PST -------

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 From 2012-05-09 23:37:52 PST -------
(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 From 2012-05-10 00:40:44 PST -------
(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.

> 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 From 2012-05-10 08:35:17 PST -------
(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.

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 From 2012-05-10 08:46:42 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 141086 [details] [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 From 2012-05-10 08:54:58 PST -------
(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
>>>> +#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 From 2012-05-23 13:41:26 PST -------
Created an attachment (id=143639) [details]
Patch rebased and now using GLib timer to avoid starvation due to event handling
------- Comment #10 From 2012-05-23 13:46:53 PST -------
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 From 2012-05-24 11:48:59 PST -------
Created an attachment (id=143864) [details]
The same patch minus a GSource bomb
------- Comment #12 From 2012-05-24 19:00:49 PST -------
(From update of attachment 143864 [details])
Clearing review flag, because this one still needs a bit of work.
------- Comment #13 From 2012-05-29 17:14:07 PST -------
Created an attachment (id=144650) [details]
Remove one more GSource leak
------- Comment #14 From 2012-05-30 00:01:33 PST -------
(From update of attachment 144650 [details])
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 From 2012-05-30 08:07:29 PST -------
(From update of attachment 144650 [details])
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 From 2012-05-31 09:14:07 PST -------
Created an attachment (id=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 From 2012-06-08 16:53:51 PST -------
Created an attachment (id=146664) [details]
Patch
------- Comment #18 From 2012-06-08 16:59:20 PST -------
The latest patch fixes both issues mentioned above, I believe.
------- Comment #19 From 2012-06-08 17:17:49 PST -------
Is a clean build required? http://queues.webkit.org/patch/146664
------- Comment #20 From 2012-06-08 17:25:46 PST -------
(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 From 2012-06-12 10:02:02 PST -------
(From update of attachment 146664 [details])
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 From 2012-06-13 14:10:01 PST -------
(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 From 2012-06-13 17:13:56 PST -------
Committed r120262: <http://trac.webkit.org/changeset/120262>