Bug 94417 – [GTK] [WebKit2] Use XComposite window for accelerated compositing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Like WebKit1, WebKit2 should use an XComposite window to implement accelerated compositing. This will allow rendering to fit into the GTK+ drawing loop and also fix GtkOverlay widgets atop the WebView.

[Martin Robinson]

Created attachment 159279 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 159279 [details]
Patch

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

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.h:31
> +#include "GRefPtrGtk.h"

What is this for?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:285
> +    if (webViewBase->priv->redirectedWindow && layerTreeContext.windowHandle != webViewBase->priv->redirectedWindow->windowHandle())
> +        webViewBase->priv->redirectedWindow = nullptr;
> +
> +    if (!webViewBase->priv->redirectedWindow)
> +        webViewBase->priv->redirectedWindow = WebCore::RedirectedXCompositeWindow::create(layerTreeContext.windowHandle);
> +
> +    return webViewBase->priv->redirectedWindow.get();

I think this method would be a bit easier to read if you get the priv pointer first:

WebKitWebViewBasePrivate* priv = webViewBase->priv;

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:319
> +#if USE(TEXTURE_MAPPER_GL) 
> +    if (drawingArea->isInAcceleratedCompositingMode()) {
> +        webkitWebViewBaseDrawAcceleratedCompositingResults(webViewBase, drawingArea, cr);
> +        return FALSE;
> +    }
> +#endif

Shouldn't we pass also de clipRect? and use it in webkitWebViewBaseDrawAcceleratedCompositingResults?

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:182
> +#
> +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> +    InvalidateWidget()
> +#endif

You should also remove the WidgetMapped meesage.

> Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:115
> +void WebPageProxy::invalidateWidget()
>  {
> -    process()->send(Messages::WebPage::WidgetMapped(nativeWindowId), m_pageID);
> +    gtk_widget_queue_draw(static_cast<PageClientImpl*>(m_pageClient)->viewWidget());

hmm, do we always have to redraw the whole widget? don't we have a damaged rect? Maybe we could call this redrawWidget to make it clear we want the widget to be redrawn.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:193
> +void WebPage::invalidateWidget()
> +{
> +    send(Messages::WebPageProxy::InvalidateWidget());
> +}

The messages is protected by USE(TEXTURE_MAPPER_GL), so this should be inside a USE(TEXTURE_MAPPER_GL) block I guess. You should also remove the implementation of widgetMapped

[Martin Robinson]

(In reply to comment #2)

Thanks for the review!

> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.h:31
> > +#include "GRefPtrGtk.h"
> 
> What is this for?

This was left over from a previous iteration. Removed.

> I think this method would be a bit easier to read if you get the priv pointer first:

Fair enough.

> Shouldn't we pass also de clipRect? and use it in webkitWebViewBaseDrawAcceleratedCompositingResults?

Sure. That's a little faster when painting damage events.

> > Source/WebKit2/UIProcess/WebPageProxy.messages.in:182
> > +#
> > +#if PLATFORM(GTK) && USE(TEXTURE_MAPPER_GL)
> > +    InvalidateWidget()
> > +#endif
> 
> You should also remove the WidgetMapped meesage.

Good catch. Removed.

> > Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:115
> > +void WebPageProxy::invalidateWidget()
> >  {
> > -    process()->send(Messages::WebPage::WidgetMapped(nativeWindowId), m_pageID);
> > +    gtk_widget_queue_draw(static_cast<PageClientImpl*>(m_pageClient)->viewWidget());
> 
> hmm, do we always have to redraw the whole widget? don't we have a damaged rect? Maybe we could call this redrawWidget to make it clear we want the widget to be redrawn.

Yes, OpenGL doesn't always repaints the entire scene. I dislike the name "redrawWidget" because that's not actually what happens on the UI side. Instead, it queues a redraw by invalidating the widget. The actual redraw happens at some point later.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:193
> > +void WebPage::invalidateWidget()
> > +{
> > +    send(Messages::WebPageProxy::InvalidateWidget());
> > +}
> 
> The messages is protected by USE(TEXTURE_MAPPER_GL), so this should be inside a USE(TEXTURE_MAPPER_GL) block I guess. You should also remove the implementation of widgetMapped

Fixed.

[Martin Robinson]

Created attachment 159340 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 159340 [details]
Patch

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

Patch looks good to me in general, but I think this should be reviewed by Alex.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:280
> +    if (priv->redirectedWindow && layerTreeContext.windowHandle != webViewBase->priv->redirectedWindow->windowHandle())

webViewBase->priv -> priv

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:363
> +    DrawingAreaProxy* drawingArea = webViewBase->priv->pageProxy->drawingArea();
> +    if (drawingArea) {
>          priv->pageProxy->drawingArea()->setSize(viewRect.size(), IntSize());
>  
> +        if (static_cast<DrawingAreaProxyImpl*>(drawingArea)->isInAcceleratedCompositingMode() && webViewBase->priv->redirectedWindow)
> +            webViewBase->priv->redirectedWindow->notifyResized();
> +    }

You already have a priv variable in this method, so you could use it to make the lines a bit shorter and easier to read.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:99
>      if (!context) {
> -        m_isValid = false;
> +         m_isValid = false;
>          return;

what is this change?

[Martin Robinson]

Created attachment 159424 [details]
Patch

[Gwang Yoon Hwang]

Good job!

I have a question about this.

In Webkit1 case, It seems sufficient to use Single Buffer + glFlush operation.
But WebKit2, I think RedirectedXCompositeWindow needs double buffering and swap-like operation, as like GrapihicsSurfaceGLX.
If we try to share a single pixmap between two process, we will encounter a number of synchronization issues.

For example, Single Buffer + glFlush operation didn't work for compositing. But Double Buffer + swapBuffers() works in my environment.
(OS : Ubuntu 12.04 64bit, GPU : nVidia GeForce GTS 450, Driver : nVidia proprietary driver 295.71)

Maybe this is a driver specific issue, maybe not.

[Martin Robinson]

(In reply to comment #7)

> For example, Single Buffer + glFlush operation didn't work for compositing. But Double Buffer + swapBuffers() works in my environment.
> (OS : Ubuntu 12.04 64bit, GPU : nVidia GeForce GTS 450, Driver : nVidia proprietary driver 295.71)

I found that single buffered contexts work poorly with Nvidia in general. We'll almost certainly be using double-buffered contexts.

[Martin Robinson]

Created attachment 159865 [details]
Patch

[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

[Gyuyoung Kim]

Comment on attachment 159865 [details]
Patch

Attachment 159865 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13570114

[Gwang Yoon Hwang]

310299    context->swapBuffers();
 300
 301    // FIXME: It seems that when using double-buffering (and on some drivers single-buffering)
 302    // and XComposite window redirection, two swap buffers are required to force the pixmap
 303    // to update. This isn't a problem during animations, because swapBuffer is continuously 
 304    // called. For non-animation situations we use this terrible hack until we can get to the
 305    // bottom of the issue.
 306    if (!toTextureMapperLayer(m_rootLayer.get())->descendantsOrSelfHaveRunningAnimations()) {
 307        context->swapBuffers();
 308        context->swapBuffers();
 309    }

It seems like there will be three swapBuffers operations on non-animation situations.

[Martin Robinson]

(In reply to comment #12)
> 310299    context->swapBuffers();
>  300
>  301    // FIXME: It seems that when using double-buffering (and on some drivers single-buffering)
>  302    // and XComposite window redirection, two swap buffers are required to force the pixmap
>  303    // to update. This isn't a problem during animations, because swapBuffer is continuously 
>  304    // called. For non-animation situations we use this terrible hack until we can get to the
>  305    // bottom of the issue.
>  306    if (!toTextureMapperLayer(m_rootLayer.get())->descendantsOrSelfHaveRunningAnimations()) {
>  307        context->swapBuffers();
>  308        context->swapBuffers();
>  309    }
> 
> It seems like there will be three swapBuffers operations on non-animation situations.

Yes, the comment tries to explain what's happening here. This is a work-around until the real problem is found. Do you have any idea what's causing this?

[Gwang Yoon Hwang]

(In reply to comment #13)
> (In reply to comment #12)
> > 310299    context->swapBuffers();
> >  300
> >  301    // FIXME: It seems that when using double-buffering (and on some drivers single-buffering)
> >  302    // and XComposite window redirection, two swap buffers are required to force the pixmap
> >  303    // to update. This isn't a problem during animations, because swapBuffer is continuously 
> >  304    // called. For non-animation situations we use this terrible hack until we can get to the
> >  305    // bottom of the issue.
> >  306    if (!toTextureMapperLayer(m_rootLayer.get())->descendantsOrSelfHaveRunningAnimations()) {
> >  307        context->swapBuffers();
> >  308        context->swapBuffers();
> >  309    }
> > 
> > It seems like there will be three swapBuffers operations on non-animation situations.
> 
> Yes, the comment tries to explain what's happening here. This is a work-around until the real problem is found. Do you have any idea what's causing this?

As I remember, nVidia's driver didn't support 32bit ARGB pixmap surface with XComposite extension. But we creates GLContextGLX with GL_RGBA, right?
I think that can cause this situation.

It's mid-night on my timezone, I cannot test/verify my assumption right now. If you found any idea, plz let me know.

[Martin Robinson]

Created attachment 161947 [details]
Try to fix the EFL build

[Carlos Garcia Campos]

I've tried the patch and I see a lot of flickering while resizing the window with MiniBrowser. I also got a crash in the web process, I don't have a debug build but this is the bt just in case it helps:

Program received signal SIGSEGV, Segmentation fault.
0xb75af163 in WebCore::GraphicsContext3D::makeContextCurrent() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
(gdb) bt
#0  0xb75af163 in WebCore::GraphicsContext3D::makeContextCurrent() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#1  0xb710194b in WebCore::GraphicsContext3D::deleteTexture(unsigned int) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#2  0xb75b10ac in WebCore::BitmapTextureGL::~BitmapTextureGL() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#3  0xb75b1102 in WebCore::BitmapTextureGL::~BitmapTextureGL() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#4  0xb75bb540 in WebCore::TextureMapperTile::~TextureMapperTile() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#5  0xb75bb60b in WebCore::TextureMapperTiledBackingStore::~TextureMapperTiledBackingStore() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#6  0xb75be7b8 in WebCore::TextureMapperLayer::~TextureMapperLayer() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#7  0xb75be802 in WebCore::TextureMapperLayer::~TextureMapperLayer() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#8  0xb75b6289 in WebCore::GraphicsLayerTextureMapper::~GraphicsLayerTextureMapper() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#9  0xb75b6402 in WebCore::GraphicsLayerTextureMapper::~GraphicsLayerTextureMapper() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#10 0xb7299e31 in WebCore::RenderLayerBacking::destroyGraphicsLayers() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#11 0xb729ad1f in WebCore::RenderLayerBacking::~RenderLayerBacking() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#12 0xb729adf2 in WebCore::RenderLayerBacking::~RenderLayerBacking() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#13 0xb728b71c in WebCore::RenderLayer::clearBacking(bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#14 0xb728d421 in WebCore::RenderLayer::~RenderLayer() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#15 0xb728d5b2 in WebCore::RenderLayer::~RenderLayer() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#16 0xb72833d9 in WebCore::RenderLayer::destroy(WebCore::RenderArena*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#17 0xb7236125 in WebCore::RenderBoxModelObject::destroyLayer() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#18 0xb72d0956 in WebCore::RenderObject::willBeDestroyed() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#19 0xb72450da in WebCore::RenderBoxModelObject::willBeDestroyed() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#20 0xb7233402 in WebCore::RenderBox::willBeDestroyed() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#21 0xb71f4018 in WebCore::RenderBlock::willBeDestroyed() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#22 0xb72cd3e5 in WebCore::RenderObject::destroy() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#23 0xb72cd272 in WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#24 0xb6c825ad in WebCore::Node::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#25 0xb6c1edb1 in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#26 0xb6c63ca7 in WebCore::Element::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#27 0xb6c1ed9b in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#28 0xb6c63ca7 in WebCore::Element::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#29 0xb6c1ed9b in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#30 0xb6c63ca7 in WebCore::Element::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#31 0xb6c1ed9b in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#32 0xb6c63ca7 in WebCore::Element::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#33 0xb6c1ed9b in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#34 0xb6c63ca7 in WebCore::Element::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#35 0xb6c1ed9b in WebCore::ContainerNode::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#36 0xb6c43052 in WebCore::Document::detach() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#37 0xb6c3145f in WebCore::Document::prepareForDestruction() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#38 0xb7070859 in WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#39 0xb6fce113 in WebCore::FrameLoader::closeAndRemoveChild(WebCore::Frame*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#40 0xb6fd2097 in WebCore::FrameLoader::detachFromParent() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#41 0xb6fd2268 in WebCore::FrameLoader::detachChildren() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#42 0xb6fd230c in WebCore::FrameLoader::setDocumentLoader(WebCore::DocumentLoader*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#43 0xb6fd2a6e in WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#44 0xb6fd56ef in WebCore::FrameLoader::commitProvisionalLoad() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#45 0xb6fb9f13 in WebCore::DocumentLoader::commitIfReady() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#46 0xb6fb9f58 in WebCore::DocumentLoader::commitLoad(char const*, int) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#47 0xb6fba0b0 in WebCore::DocumentLoader::receivedData(char const*, int) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#48 0xb6ff2a86 in WebCore::MainResourceLoader::addData(char const*, int, bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#49 0xb7007bd6 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#50 0xb6ff2b6b in WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
---Type <return> to continue, or q <return> to quit---
#51 0xb7007209 in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#52 0xb716c65a in WebCore::readCallback(_GObject*, _GAsyncResult*, void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#53 0xb56089d8 in async_ready_callback_wrapper (source_object=0xaf602cd0, res=0xb0002590, user_data=0xacd3fae0) at ginputstream.c:529
#54 0xb561f8c0 in g_simple_async_result_complete (simple=0xb0002590) at gsimpleasyncresult.c:775
#55 0xb561f9ec in complete_in_idle_cb (data=0xb0002590) at gsimpleasyncresult.c:787
#56 0xb5157310 in g_idle_dispatch (source=0x87466c0, callback=0xb561f9c0 <complete_in_idle_cb>, user_data=0xb0002590) at gmain.c:4797
#57 0xb515a036 in g_main_dispatch (context=0x80785e0) at gmain.c:2707
#58 g_main_context_dispatch (context=0x80785e0) at gmain.c:3211
#59 0xb515a3d5 in g_main_context_iterate (dispatch=1, block=-1256817280, context=0x80785e0, self=<optimized out>) at gmain.c:3282
#60 g_main_context_iterate (context=0x80785e0, block=-1256817280, dispatch=1, self=<optimized out>) at gmain.c:3219
#61 0xb515a81b in g_main_loop_run (loop=0x81127d8) at gmain.c:3476
#62 0xb7b65758 in WebCore::RunLoop::run() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#63 0xb699a53d in WebProcessMainGtk () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/.libs/libwebkit2gtk-3.0.so.0
#64 0x0804853b in main ()

[Martin Robinson]

How do you reproduce the crash?

[Carlos Garcia Campos]

1.- Open MiniBrowser
2.- Go to any website
3.- Go to http://www.webkit.org/blog/386/3d-transforms/
4.- Use the bf list to go to webkitgtk page for example or the other one I think it doesn't matter
5.- Use the bf list to go to http://www.webkit.org/blog/386/3d-transforms/ again
6.- Reload the page by pressing enter in the location bar
CRASH
Try to reload again or repeat the steps if you don't get the crash

[Martin Robinson]

(In reply to comment #18)
> 1.- Open MiniBrowser
> 2.- Go to any website
> 3.- Go to http://www.webkit.org/blog/386/3d-transforms/
> 4.- Use the bf list to go to webkitgtk page for example or the other one I think it doesn't matter
> 5.- Use the bf list to go to http://www.webkit.org/blog/386/3d-transforms/ again
> 6.- Reload the page by pressing enter in the location bar
> CRASH
> Try to reload again or repeat the steps if you don't get the crash

I was able to reproduce this crash with both WebKit1 and WebKit2 without the patch, so it's likely fallout from http://trac.webkit.org/changeset/127063 and unrelated to this patch.

I'll try to address the flickering, which is, again, probably fallout from the TextureMapper port to GC3D.

[Martin Robinson]

Comment on attachment 161947 [details]
Try to fix the EFL build

Should have a new patch soon fixing the flickering and removing the need for the double swap buffer in most cases.

[Martin Robinson]

Created attachment 164442 [details]
Patch

[Early Warning System Bot]

Comment on attachment 164442 [details]
Patch

Attachment 164442 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13883155

[Martin Robinson]

Created attachment 164468 [details]
Try to fix the Qt build

[Carlos Garcia Campos]

Comment on attachment 164468 [details]
Try to fix the Qt build

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:281
> +    webkitWebViewBase->priv->redirectedWindow = RedirectedXCompositeWindow::create(IntSize(1, 1));
> +    webkitWebViewBase->priv->readyToRenderAcceleratedCompositingResults = false;

Use priv instead of webkitWebViewBase->priv.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:361
> +    if (webViewBase->priv->redirectedWindow)
> +        webViewBase->priv->redirectedWindow->resize(IntSize(allocation->width, allocation->height));
> +
> +    if (DrawingAreaProxyImpl* drawingArea = static_cast<DrawingAreaProxyImpl*>(webViewBase->priv->pageProxy->drawingArea()))
> +        drawingArea->setSize(viewRect.size(), IntSize());

Ditto, you have priv here too already.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:375
> +        if (!gtk_widget_get_mapped(GTK_WIDGET(webViewBase)) && !webViewBase->priv->pageProxy->drawingArea()->size().isEmpty()) {

You don't need to cast this back to GtkWidget again, simply use widget !gtk_widget_get_mapped(widget)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:865
> +    gpointer* webViewBasePointer = static_cast<gpointer*>(fastMalloc(sizeof(gpointer)));
> +    g_object_add_weak_pointer(G_OBJECT(webViewBase), webViewBasePointer);
> +    *webViewBasePointer = webViewBase;
> +    g_timeout_add(1000 / 60, reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults), webViewBasePointer);

You can save the handler id of g_timeout_add() and destroy the source in WebKitWebViewBase::dispose, so that queueAnotherDrawOfAcceleratedCompositingResults() will never be called if the widget is destroyed.

[Carlos Garcia Campos]

Comment on attachment 164468 [details]
Try to fix the Qt build

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:285
> +static bool renderAcceleratedCompositingResults(WebKitWebViewBase* webViewBase, cairo_t* cr, DrawingAreaProxyImpl* drawingArea)

In previous patches this received the clip rect also.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:357
> +    if (webViewBase->priv->redirectedWindow)

This is created unconditionally in _init() so I don't think you need to check it's != NULL here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> +        webViewBase->priv->redirectedWindow->resize(IntSize(allocation->width, allocation->height));

This will always be done, shouldn't you check first that there's a drawing area in AC mode?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:383
> +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->size_allocate(widget, allocation);

Why have you moved this here? Are you sure you don't need to call the parent first, if you return early the allocation won't be called on the parent.

[Martin Robinson]

(In reply to comment #24)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:865
> > +    gpointer* webViewBasePointer = static_cast<gpointer*>(fastMalloc(sizeof(gpointer)));
> > +    g_object_add_weak_pointer(G_OBJECT(webViewBase), webViewBasePointer);
> > +    *webViewBasePointer = webViewBase;
> > +    g_timeout_add(1000 / 60, reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults), webViewBasePointer);
> 
> You can save the handler id of g_timeout_add() and destroy the source in WebKitWebViewBase::dispose, so that queueAnotherDrawOfAcceleratedCompositingResults() will never be called if the widget is destroyed.

I considered that, but there may be more than one handler pending and the task of figuring out to cancel the existing one or not is a bit more complicated than simply using a weak reference.

(In reply to comment #25)
> (From update of attachment 164468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164468&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:285
> > +static bool renderAcceleratedCompositingResults(WebKitWebViewBase* webViewBase, cairo_t* cr, DrawingAreaProxyImpl* drawingArea)
> 
> In previous patches this received the clip rect also.

Okay.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> > +        webViewBase->priv->redirectedWindow->resize(IntSize(allocation->width, allocation->height));
> 
> This will always be done, shouldn't you check first that there's a drawing area in AC mode?

The issue is then once we enter AC mode we need to resize the window and there's no good way to track that in the WebView at this moment.


> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:383
> > +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->size_allocate(widget, allocation);
> 
> Why have you moved this here? Are you sure you don't need to call the parent first, if you return early the allocation won't be called on the parent.

I moved it back to the top. Nice catch.

[Martin Robinson]

Created attachment 164565 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 164565 [details]
Patch

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

Setting r- because it breaks the attached inspector.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:281
> +    priv->redirectedWindow = RedirectedXCompositeWindow::create(IntSize(1, 1));
> +    priv->readyToRenderAcceleratedCompositingResults = false;

These are now only defined when USE(TEXTURE_MAPPER_GL)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> +    webViewBase->priv->redirectedWindow->resize(IntSize(allocation->width, allocation->height));

Ditto, this should be protected by #if USE(TEXTURE_MAPPER_GL) #endif block. I think this breaks the inspector when attached to the view. you should probably use the view size, not the whole window size.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:371
> +    GtkAllocation oldAllocation;
> +    gtk_widget_get_allocation(widget, &oldAllocation);

Since you are only interested in the size and not the position, you could create an IntSize oldSize(gtk_widget_get_allocated_width(widget), gtk_widget_get_allocated_height(widget));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:375
> +    if (oldAllocation.width != allocation->width || oldAllocation.height != allocation->height) {

And you could create an IntRect for the new allocation and here compare oldSize != viewRect.size().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:382
> +        resizeWebKitWebViewBaseFromAllocation(webViewBase, allocation);

And here you pass the viewRect instead of the GtkAllocation and then you don't need to create the viewRect in resizeWebKitWebViewBaseFromAllocation

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:865
> +    gpointer* webViewBasePointer = static_cast<gpointer*>(fastMalloc(sizeof(gpointer)));
> +    g_object_add_weak_pointer(G_OBJECT(webViewBase), webViewBasePointer);
> +    *webViewBasePointer = webViewBase;
> +    g_timeout_add(1000 / 60, reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults), webViewBasePointer);

If you need something to be created and destroyed for the life of a GSource, you can use g_time_add_full and provide a GDestroyNotify, that way you don't need to care about destroying the data in the idle callback, and the data will be destroyed even if the source is destroyed before the callback is called. In this case I would use a helper struct, something like this:

struct DelayedQueueDraw {
    WebKitWebViewBase* view;
};

or a better name for the struct, I don't know.

static DelayedQueueDraw* delayed_queue_draw_create(WebKitWebViewBase* view)
{
    DelayedQueueDraw* delayedDraw = g_slice_new0(DelayedQueueDraw); // Or new instead of g_slice
    delayedDraw->view = view;
    g_object_add_weak_pointer(G_OBJECT(webViewBase), &delayedDraw->view);
    return delayedDraw;
}

static void delayed_queue_draw_free(DelayedQueueDraw* delayedDraw)
{
    if (delayedDraw->view)
        g_object_remove_weak_pointer(G_OBJECT(delayedDraw->view), &delayedDraw->view);
    g_slice_free(DelayedQueueDraw, delayedDraw);
}

then you can use g_timeout_add_full this way:

g_timeout_add_full(G_PRIORITY_DEFAULT, 1000 / 60, reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults), delayed_queue_draw_create(webViewBase), delayed_queue_draw_free);

[Martin Robinson]

Created attachment 164582 [details]
Fix embedded inspector and build errors for non TM builds

[Carlos Garcia Campos]

Comment on attachment 164582 [details]
Fix embedded inspector and build errors for non TM builds

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

Looks good to me, thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:367
> +    if (DrawingAreaProxyImpl* drawingArea = static_cast<DrawingAreaProxyImpl*>(priv->pageProxy->drawingArea()))
> +        drawingArea->setSize(viewRect.size(), IntSize());

setSize is in the parent class, so you don't need the cast, you can simply use the code that we are currently using:

if (priv->pageProxy->drawingArea())
    priv->pageProxy->drawingArea()->setSize(viewRect.size(), IntSize());

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:179
> +#

Extra # here

[Martin Robinson]

Committed r128907: <http://trac.webkit.org/changeset/128907>

[Carlos Garcia Campos]

See https://bugs.webkit.org/show_bug.cgi?id=97092