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.
Created attachment 141054 [details] Patch
Created attachment 141086 [details] Enable the layer flush scheduling by default
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.
(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 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"
(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!
(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 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.
Created attachment 143639 [details] Patch rebased and now using GLib timer to avoid starvation due to event handling
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
Created attachment 143864 [details] The same patch minus a GSource bomb
Comment on attachment 143864 [details] The same patch minus a GSource bomb Clearing review flag, because this one still needs a bit of work.
Created attachment 144650 [details] Remove one more GSource leak
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 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.
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.
Created attachment 146664 [details] Patch
The latest patch fixes both issues mentioned above, I believe.
Is a clean build required? http://queues.webkit.org/patch/146664
(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 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.
(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.
Committed r120262: <http://trac.webkit.org/changeset/120262>