Bug 86037 – [GTK] Add an accelerated compositing implementation for WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 141054 [details]
Patch

[Martin Robinson]

Created attachment 141086 [details]
Enable the layer flush scheduling by default

[xpert]

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.

[Martin Robinson]

(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...

[Carlos Garcia Campos]

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"

[Martin Robinson]

(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!

[Carlos Garcia Campos]

(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

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 143639 [details]
Patch rebased and now using GLib timer to avoid starvation due to event handling

[WebKit Review Bot]

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

[Martin Robinson]

Created attachment 143864 [details]
The same patch minus a GSource bomb

[Martin Robinson]

Comment on attachment 143864 [details]
The same patch minus a GSource bomb

Clearing review flag, because this one still needs a bit of work.

[Martin Robinson]

Created attachment 144650 [details]
Remove one more GSource leak

[Carlos Garcia Campos]

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"

[Martin Robinson]

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.

[José Dapena Paz]

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.

[Martin Robinson]

Created attachment 146664 [details]
Patch

[Martin Robinson]

The latest patch fixes both issues mentioned above, I believe.

[Philippe Normand]

Is a clean build required? http://queues.webkit.org/patch/146664

[Martin Robinson]

(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.

[Alejandro G. Castro]

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.

[Martin Robinson]

(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.

[Martin Robinson]

Committed r120262: <http://trac.webkit.org/changeset/120262>