Bug 142865 - [GTK] Add a configure option to build without Redirected XComposite Window
Summary: [GTK] Add a configure option to build without Redirected XComposite Window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2015-03-19 03:31 PDT by Carlos Garcia Campos
Modified: 2015-03-23 01:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.76 KB, patch)
2015-03-19 03:37 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased to apply on trunk (9.29 KB, patch)
2015-03-20 00:58 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (8.84 KB, patch)
2015-03-20 04:58 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 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.
Comment 1 Carlos Garcia Campos 2015-03-19 03:37:07 PDT
Created attachment 249031 [details]
Patch
Comment 2 Gwang Yoon Hwang 2015-03-19 03:46:43 PDT
This feature looks okay for me.
How about to use "USE" instead of "ENABLE"?
Comment 3 Carlos Garcia Campos 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?
Comment 4 Gwang Yoon Hwang 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.
Comment 5 Martin Robinson 2015-03-19 07:53:16 PDT
What happens when the widget shares a window with another widget?
Comment 6 Martin Robinson 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?
Comment 7 Gwang Yoon Hwang 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.
Comment 8 Gwang Yoon Hwang 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
Comment 9 Carlos Garcia Campos 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.
Comment 10 Carlos Garcia Campos 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.
Comment 11 Carlos Garcia Campos 2015-03-20 00:58:50 PDT
Created attachment 249095 [details]
Rebased to apply on trunk
Comment 12 Gwang Yoon Hwang 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.
Comment 13 Zan Dobersek 2015-03-20 02:48:45 PDT
I'm in favor of USE as well.
Comment 14 Carlos Garcia Campos 2015-03-20 04:58:27 PDT
Created attachment 249107 [details]
Updated patch

Rebased to current git master again and switched to USE
Comment 15 Gwang Yoon Hwang 2015-03-20 05:01:31 PDT
Looks good for me. Good job!
Comment 16 Carlos Garcia Campos 2015-03-23 01:06:53 PDT
Committed r181847: <http://trac.webkit.org/changeset/181847>