RESOLVED FIXED 142865
[GTK] Add a configure option to build without Redirected XComposite Window
https://bugs.webkit.org/show_bug.cgi?id=142865
Summary [GTK] Add a configure option to build without Redirected XComposite Window
Carlos Garcia Campos
Reported 2015-03-19 03:31:58 PDT
The Redirected XComposite Window was added to support some features like GtkOverlay, but in cases where we don't need such features, it's more efficient to use the XID of the WebKitWebView window as the native surface handle for the accelerated compositing. We have noticed also some memory issues with the Redirected XComposite Window in some embedded environments, so having the option to disable it would be very useful.
Attachments
Patch (8.76 KB, patch)
2015-03-19 03:37 PDT, Carlos Garcia Campos
no flags
Rebased to apply on trunk (9.29 KB, patch)
2015-03-20 00:58 PDT, Carlos Garcia Campos
no flags
Updated patch (8.84 KB, patch)
2015-03-20 04:58 PDT, Carlos Garcia Campos
zan: review+
Carlos Garcia Campos
Comment 1 2015-03-19 03:37:07 PDT
Gwang Yoon Hwang
Comment 2 2015-03-19 03:46:43 PDT
This feature looks okay for me. How about to use "USE" instead of "ENABLE"?
Carlos Garcia Campos
Comment 3 2015-03-19 03:49:45 PDT
(In reply to comment #2) > This feature looks okay for me. > How about to use "USE" instead of "ENABLE"? hmm, I really don't mind to use USE, is there any policy or convention?
Gwang Yoon Hwang
Comment 4 2015-03-19 04:00:07 PDT
(In reply to comment #3) > (In reply to comment #2) > > This feature looks okay for me. > > How about to use "USE" instead of "ENABLE"? > > hmm, I really don't mind to use USE, is there any policy or convention? Because I'm outside right now, i cannot find a reference. as far as I understand, ENABLE used for feature, but this patch provides Another way for a same purpose. So i thought USE is more appropriate.
Martin Robinson
Comment 5 2015-03-19 07:53:16 PDT
What happens when the widget shares a window with another widget?
Martin Robinson
Comment 6 2015-03-19 07:55:14 PDT
I'm also a bit curious about the memory usage, because a double-buffer window framebuffer and a window + a single-buffer pixmap framebuffer should take about the same amount of memory. Have you tried simply making the redirected X Composite window a single-buffer framebuffer?
Gwang Yoon Hwang
Comment 7 2015-03-19 18:44:17 PDT
(In reply to comment #5) > What happens when the widget shares a window with another widget? I don't know other cases which shares a window with another widget other than GtkOverlay. In that case, the rendered result of GtkOverlay will be erased if there is another update from WebProcess. I have a another idea to provide a cairo surface to GtkOverlay and composite its surface in our accelerated compositing loop at same time like PageOverlay.
Gwang Yoon Hwang
Comment 8 2015-03-19 19:01:52 PDT
(In reply to comment #6) > I'm also a bit curious about the memory usage, because a double-buffer > window framebuffer and a window + a single-buffer pixmap framebuffer should > take about the same amount of memory. Have you tried simply making the > redirected X Composite window a single-buffer framebuffer? Well, I didn't thought to make redirected X Composite window as a single-buffered. I think the target frame buffer of GLContext should be double buffered. If it is not, we need to make sure the frame buffer of redirected X Composite Window copied to WebView's window before SwapBuffer call in WebProcess. And this approach should reduce memory copy overhead. Current redirected X Composite window approach has a overhead to copy frame buffer to cairo surface of WebView, but in this approach, we can simply flip the double-buffer window frame buffer. I need to investigate more detailed GtkWidget and GdkWindow's behavior to remove other possible overhead such as gdk_window_begin_paint_region / gdk_window_end_paint[1]. [1] https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-begin-paint-region
Carlos Garcia Campos
Comment 9 2015-03-20 00:31:36 PDT
(In reply to comment #5) > What happens when the widget shares a window with another widget? This option is disabled by default, and it's thought for users who know what they are doing. There are many cases where you don't need the functionality provided by the redirected window, so you can just opt out at your own risk.
Carlos Garcia Campos
Comment 10 2015-03-20 00:54:14 PDT
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > This feature looks okay for me. > > > How about to use "USE" instead of "ENABLE"? > > > > hmm, I really don't mind to use USE, is there any policy or convention? > > Because I'm outside right now, i cannot find a reference. > as far as I understand, ENABLE used for feature, but this patch provides > Another way for a same purpose. So i thought USE is more appropriate. In Platform.h /* USE() - use a particular third-party library or optional OS service */ /* ENABLE() - turn on a specific feature of WebKit */ I would probably use USE if we could decide whether to use it or not depending only on the dependencies or other command line options (for example disable it for wayland or when using the threaded compositor), but since it's an option exposed to the user as a command line option, I followed what other options do. CMake configure options are already confusing enough, having -DNABLE_FOO and -DUSE_FOO options could make it even more confusing. But still, if you guys think USE fit better for this case I don't mind to change it.
Carlos Garcia Campos
Comment 11 2015-03-20 00:58:50 PDT
Created attachment 249095 [details] Rebased to apply on trunk
Gwang Yoon Hwang
Comment 12 2015-03-20 01:11:57 PDT
(In reply to comment #10) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > This feature looks okay for me. > > > > How about to use "USE" instead of "ENABLE"? > > > > > > hmm, I really don't mind to use USE, is there any policy or convention? > > > > Because I'm outside right now, i cannot find a reference. > > as far as I understand, ENABLE used for feature, but this patch provides > > Another way for a same purpose. So i thought USE is more appropriate. > > In Platform.h > > /* USE() - use a particular third-party library or optional OS service */ > /* ENABLE() - turn on a specific feature of WebKit */ > > I would probably use USE if we could decide whether to use it or not > depending only on the dependencies or other command line options (for > example disable it for wayland or when using the threaded compositor), but > since it's an option exposed to the user as a command line option, I > followed what other options do. CMake configure options are already > confusing enough, having -DNABLE_FOO and -DUSE_FOO options could make it > even more confusing. But still, if you guys think USE fit better for this > case I don't mind to change it. Thanks for clarify the convention. :) Still I'm prefer to use "USE" because XCompositeWindow relies on the XComposite and XDamage which is a optional OS service. To match the convention, it is also good to use USE(REDIRECT_COMPOSITING) and enable it as a default, but I think just replacing it to USE is also okay.
Zan Dobersek
Comment 13 2015-03-20 02:48:45 PDT
I'm in favor of USE as well.
Carlos Garcia Campos
Comment 14 2015-03-20 04:58:27 PDT
Created attachment 249107 [details] Updated patch Rebased to current git master again and switched to USE
Gwang Yoon Hwang
Comment 15 2015-03-20 05:01:31 PDT
Looks good for me. Good job!
Carlos Garcia Campos
Comment 16 2015-03-23 01:06:53 PDT
Note You need to log in before you can comment on or make changes to this bug.