Summary: | [GTK] [WebKit2] Use XComposite window for accelerated compositing | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | alex, cgarcia, cwhong893, dev, dongseong.hwang, gustavo, skyul, webkit.review.bot, yoon | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 90085 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Martin Robinson
2012-08-18 13:38:28 PDT
Created attachment 159279 [details]
Patch
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 (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. Created attachment 159340 [details]
Patch
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? Created attachment 159424 [details]
Patch
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. (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. Created attachment 159865 [details]
Patch
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 on attachment 159865 [details] Patch Attachment 159865 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13570114 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. (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? (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. Created attachment 161947 [details]
Try to fix the EFL build
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 () How do you reproduce the crash? 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 (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. 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.
Created attachment 164442 [details]
Patch
Comment on attachment 164442 [details] Patch Attachment 164442 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13883155 Created attachment 164468 [details]
Try to fix the Qt build
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. 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. (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. Created attachment 164565 [details]
Patch
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); Created attachment 164582 [details]
Fix embedded inspector and build errors for non TM builds
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 Committed r128907: <http://trac.webkit.org/changeset/128907> |