Bug 142865 – [GTK] Add a configure option to build without Redirected XComposite Window

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 249031 [details]
Patch

[Gwang Yoon Hwang]

This feature looks okay for me.
How about to use "USE" instead of "ENABLE"?

[Carlos Garcia Campos]

(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]

(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]

What happens when the widget shares a window with another widget?

[Martin Robinson]

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]

(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]

(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]

(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]

(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]

Created attachment 249095 [details]
Rebased to apply on trunk

[Gwang Yoon Hwang]

(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]

I'm in favor of USE as well.

[Carlos Garcia Campos]

Created attachment 249107 [details]
Updated patch

Rebased to current git master again and switched to USE

[Gwang Yoon Hwang]

Looks good for me. Good job!

[Carlos Garcia Campos]

Committed r181847: <http://trac.webkit.org/changeset/181847>