| Summary: | [GTK] Use XDamage to simplify RedirectedXCompositeWindow | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Martin Robinson <mrobinson> |
| Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | alex, cgarcia, gustavo, webkit.review.bot |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Martin Robinson
2012-09-20 15:42:40 PDT
Created attachment 165009 [details]
Patch
Created attachment 165174 [details]
Rebased on top of trunk
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 165174 [details] Rebased on top of trunk View in context: https://bugs.webkit.org/attachment.cgi?id=165174&action=review Great patch, added some comment. Also I found some issue that I could reproduce reliably with the following steps: 1. Open and load MiniBrowser with: http://www.webkit.org/blog/386/3d-transforms 2. Go to www.google.com 3. Go back to the transforms webpage 4. Flip the card and wait for a moment, the webpage is replaced with grey background. After that even when reloading layered content and scrolling does not work any more I hope this helps. > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:93 > + gdk_window_add_filter(0, reinterpret_cast<GdkFilterFunc>(filterXDamageEvent), 0); Should we remove the filter when we are not redirecting the window anymore to avoid filtering events? Checking the API I also wonder if we could call this function in the constructor and use the first parameter to filter with m_window, maybe we can avoid using the hash, not sure if I'm missing some part. Comment on attachment 165174 [details] Rebased on top of trunk View in context: https://bugs.webkit.org/attachment.cgi?id=165174&action=review > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:63 > + i->second->callDamageNotifyCallback(); We could get the damaged region and pass it to the callback so that they can use gtk_widget_queue_draw_region() or gtk_widget_queue_draw_area() and update only the damaged region. >> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:93 >> + gdk_window_add_filter(0, reinterpret_cast<GdkFilterFunc>(filterXDamageEvent), 0); > > Should we remove the filter when we are not redirecting the window anymore to avoid filtering events? Checking the API I also wonder if we could call this function in the constructor and use the first parameter to filter with m_window, maybe we can avoid using the hash, not sure if I'm missing some part. We can't filter per window, because it expects a GdkWindow, but I agree that instead of using a global filter we could install a filter for every window in the constructor passing the RedirectedXCompositeWindow as user_data, so that we don't need the global hash map. And of course the filter should be removed in the destructor. > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:79 > m_redirectedWindow = RedirectedXCompositeWindow::create(pageSize); > - else > + m_redirectedWindow->setDamageNotifyCallback(redirectedWindowDamagedCallback, m_webView); m_redirectedWindow can be null now if X extensions are not available. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:937 > + gtk_widget_queue_draw(GTK_WIDGET(data)); The implementation of the callback is exactly the same in wk1 and wk2, I wonder if we could simply pass the widget to the redirected window object and call gtk_widget_queue_draw_region from the GdkFilter function callback. It's simpler than using a callback + callback_data (In reply to comment #5) > (From update of attachment 165174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165174&action=review > > > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:63 > > + i->second->callDamageNotifyCallback(); > > We could get the damaged region and pass it to the callback so that they can use gtk_widget_queue_draw_region() or gtk_widget_queue_draw_area() and update only the damaged region. The damaged region will always be the entire pixmap, so this will bring no benefit at this point. > > >> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:93 > >> + gdk_window_add_filter(0, reinterpret_cast<GdkFilterFunc>(filterXDamageEvent), 0); > > > > Should we remove the filter when we are not redirecting the window anymore to avoid filtering events? Checking the API I also wonder if we could call this function in the constructor and use the first parameter to filter with m_window, maybe we can avoid using the hash, not sure if I'm missing some part. > > We can't filter per window, because it expects a GdkWindow, but I agree that instead of using a global filter we could install a filter for every window in the constructor passing the RedirectedXCompositeWindow as user_data, so that we don't need the global hash map. And of course the filter should be removed in the destructor. I did use this approach at the beginning, but I opted for a global filter, because if we opened 20 WebViews running AC or WebGL all of those would have their own global event filter. I was a lot more certain of the performance impact of a single event filter. The more I think about this, the more I agree with myself. I also considered removing the event filter when the last RedirectedXComposite window was destroyed. The logic seemed a bit more complicated, but I can do that. > > > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:79 > > m_redirectedWindow = RedirectedXCompositeWindow::create(pageSize); > > - else > > + m_redirectedWindow->setDamageNotifyCallback(redirectedWindowDamagedCallback, m_webView); > > m_redirectedWindow can be null now if X extensions are not available. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:937 > > + gtk_widget_queue_draw(GTK_WIDGET(data)); > > The implementation of the callback is exactly the same in wk1 and wk2, I wonder if we could simply pass the widget to the redirected window object and call gtk_widget_queue_draw_region from the GdkFilter function callback. It's simpler than using a callback + callback_data I like this approach because it's more general. It allows the user of RedirectedXCompositeWindow to do more than just queue a redraw. It also reduces the GTK+ dependencies for RedirectedXCompositeWindow, which is a good thing, because one day it may be used by more than just GTK+. (In reply to comment #6) > I like this approach because it's more general. It allows the user of RedirectedXCompositeWindow to do more than just queue a redraw. It also reduces the GTK+ dependencies for RedirectedXCompositeWindow, which is a good thing, because one day it may be used by more than just GTK+. Granted, we are increasing them in this patch, but I still like the flexibility of just passing a callback. (In reply to comment #4) > (From update of attachment 165174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165174&action=review > > Great patch, added some comment. Also I found some issue that I could reproduce reliably with the following steps: > > 1. Open and load MiniBrowser with: http://www.webkit.org/blog/386/3d-transforms > 2. Go to www.google.com > 3. Go back to the transforms webpage > 4. Flip the card and wait for a moment, the webpage is replaced with grey background. > > After that even when reloading layered content and scrolling does not work any more I think this is https://bugs.webkit.org/show_bug.cgi?id=97394. I confirmed that I see it with and without the patch. > I hope this helps. > > > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:93 > > + gdk_window_add_filter(0, reinterpret_cast<GdkFilterFunc>(filterXDamageEvent), 0); > > Should we remove the filter when we are not redirecting the window anymore to avoid filtering events? Checking the API I also wonder if we could call this function in the constructor and use the first parameter to filter with m_window, maybe we can avoid using the hash, not sure if I'm missing some part. Going to upload a new patch that adds and removes the filter at the appropriate times. (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 165174 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=165174&action=review > > > > > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:63 > > > + i->second->callDamageNotifyCallback(); > > > > We could get the damaged region and pass it to the callback so that they can use gtk_widget_queue_draw_region() or gtk_widget_queue_draw_area() and update only the damaged region. > > The damaged region will always be the entire pixmap, so this will bring no benefit at this point. Why is always the entire pixmap? > > > > >> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:93 > > >> + gdk_window_add_filter(0, reinterpret_cast<GdkFilterFunc>(filterXDamageEvent), 0); > > > > > > Should we remove the filter when we are not redirecting the window anymore to avoid filtering events? Checking the API I also wonder if we could call this function in the constructor and use the first parameter to filter with m_window, maybe we can avoid using the hash, not sure if I'm missing some part. > > > > We can't filter per window, because it expects a GdkWindow, but I agree that instead of using a global filter we could install a filter for every window in the constructor passing the RedirectedXCompositeWindow as user_data, so that we don't need the global hash map. And of course the filter should be removed in the destructor. > > I did use this approach at the beginning, but I opted for a global filter, because if we opened 20 WebViews running AC or WebGL all of those would have their own global event filter. I was a lot more certain of the performance impact of a single event filter. The more I think about this, the more I agree with myself. Did you notice performance issues using a global filter per window? The loop is broken as soon as the filter for the window is called because it returns GDK_FILTER_REMOVE, so not all filter callbacks are always called. > > The implementation of the callback is exactly the same in wk1 and wk2, I wonder if we could simply pass the widget to the redirected window object and call gtk_widget_queue_draw_region from the GdkFilter function callback. It's simpler than using a callback + callback_data > > I like this approach because it's more general. It allows the user of RedirectedXCompositeWindow to do more than just queue a redraw. It also reduces the GTK+ dependencies for RedirectedXCompositeWindow, which is a good thing, because one day it may be used by more than just GTK+. Fair enough. Created attachment 165262 [details]
Remove the filter when the last RedirectedXComposite window is destructed
Comment on attachment 165262 [details] Remove the filter when the last RedirectedXComposite window is destructed View in context: https://bugs.webkit.org/attachment.cgi?id=165262&action=review > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:114 > - Display* display = GLContextGLX::sharedDisplay(); > + Display* display = GDK_DISPLAY_XDISPLAY(gdk_display_get_default()); Btw, just curiosity, why is this change required now? > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:79 > m_redirectedWindow = RedirectedXCompositeWindow::create(pageSize); > - else > + m_redirectedWindow->setDamageNotifyCallback(redirectedWindowDamagedCallback, m_webView); As I said, this would crash if RedirectedXCompositeWindow::create() returns NULL. > > The damaged region will always be the entire pixmap, so this will bring no benefit at this point. > > Why is always the entire pixmap? We are using OpenGL in a double-buffered manner, to avoid redraw artifacts. When you update the scene, you must then swap the buffer, which updates the entire window and, in turn, the entire XComposite pixmap. > Did you notice performance issues using a global filter per window? The loop is broken as soon as the filter for the window is called because it returns GDK_FILTER_REMOVE, so not all filter callbacks are always called. I'll break down my reasoning here. The performance implications of installing a single GDK event filter and accessing data from a HashMap is well understood. I have some reasonable confidence that installing a single filter will work well through all versions of GDK. I believe this because there's no warning in the documentation and I've seen other projects install global GDK event filters to accomplish this very task. The only additional complexity of my approach (in the big O sense of the word) is in accessing the HashMap. I'm also reasonably confident that the HashMap implementation is efficient enough that adding some windows is not going to drastically alter the big O of the lookup. When I'm using libraries like GDK, my philosophy is that as much as I can (and it's not always possible, because documentation or APIs are poorly written) I use them as a black boxes. Thus, even if I opened up the GDK source on my computer and verified that, indeed, it's not expensive to install 20 global event filters, I would be doing a slight disservice to the users. They might be using some other version of GDK for which this wasn't the case. This introduces slightly more uncertainty into the approach you are suggesting. Now, admittedly the danger here is low. A user is not likely to open 20 WebViews with accelerated compositing turned on. On the other hand, why not write more resilient code? (In reply to comment #11) > (From update of attachment 165262 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165262&action=review > > > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:114 > > - Display* display = GLContextGLX::sharedDisplay(); > > + Display* display = GDK_DISPLAY_XDISPLAY(gdk_display_get_default()); > > Btw, just curiosity, why is this change required now? It's safer to create the window, call XDamageCreate, and install the global event filter into a display that GDK controls and this improves the code in general. Created attachment 165264 [details]
Handle another situation where the creation of the RedirectedXCompositeWindow can fail
(In reply to comment #12) > > > The damaged region will always be the entire pixmap, so this will bring no benefit at this point. > > > > Why is always the entire pixmap? > > We are using OpenGL in a double-buffered manner, to avoid redraw artifacts. When you update the scene, you must then swap the buffer, which updates the entire window and, in turn, the entire XComposite pixmap. Ah, understood, thanks. > > Did you notice performance issues using a global filter per window? The loop is broken as soon as the filter for the window is called because it returns GDK_FILTER_REMOVE, so not all filter callbacks > are always called. > > I'll break down my reasoning here. The performance implications of installing a single GDK event filter and accessing data from a HashMap is well understood. I have some reasonable confidence that installing a single filter will work well through all versions of GDK. I believe this because there's no warning in the documentation and I've seen other projects install global GDK event filters to accomplish this very task. The only additional complexity of my approach (in the big O sense of the word) is in accessing the HashMap. I'm also reasonably confident that the HashMap implementation is efficient enough that adding some windows is not going to drastically alter the big O of the lookup. > > When I'm using libraries like GDK, my philosophy is that as much as I can (and it's not always possible, because documentation or APIs are poorly written) I use them as a black boxes. Thus, even if I opened up the GDK source on my computer and verified that, indeed, it's not expensive to install 20 global event filters, I would be doing a slight disservice to the users. They might be using some other version of GDK for which this wasn't the case. This introduces slightly more uncertainty into the approach you are suggesting. > > Now, admittedly the danger here is low. A user is not likely to open 20 WebViews with accelerated compositing turned on. On the other hand, why not write more resilient code? The idea was to simplify the code and allow to easily remove the filter in the destructor, but the approach of removing the filter when the window hash map is empty is simple enough anyway. (In reply to comment #15) > The idea was to simplify the code and allow to easily remove the filter in the destructor, but the approach of removing the filter when the window hash map is empty is simple enough anyway. Okay. Thanks for the reviews. Any other issues with this patch? (In reply to comment #16) > (In reply to comment #15) > > > The idea was to simplify the code and allow to easily remove the filter in the destructor, but the approach of removing the filter when the window hash map is empty is simple enough anyway. > > Okay. Thanks for the reviews. Any other issues with this patch? Nope, patch looks good to me, but Alex was playing with it, so I prefer he gives a final review. Comment on attachment 165264 [details]
Handle another situation where the creation of the RedirectedXCompositeWindow can fail
LGTM! Great work.
Comment on attachment 165264 [details] Handle another situation where the creation of the RedirectedXCompositeWindow can fail Landed at http://trac.webkit.org/changeset/129651 |