Bug 160389 – [GTK] Move the redirected XComposite window to the web process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

In the current code, the UI process creates the redirected window that the web process uses to render accelerated contents. The redirected window is sent to the web process as native surface handle, and using Xdamage extension the UI process takes a pixmap of the redirected window to render into the widget when there are updates. This requires several points of synchronization between UI and web process. When the web view is resized, the UI process first resizes the redirected window and then sends a new backing store ID to the web process. The time between the redirected window is resized and the web process renders the new contents the UI process keeps rendering the previous contents with the previous size in the new window with the new size. This makes the resize process slow, and it produces rendering artifacts quite often. The redirected window is created when the web view is realized, to be able to inherit the Xvisual from the parent window, and the native window handle is sent to the web process. The time until the window is realized, the web process doesn't have a context to render into, so the UI process simply render an empty page. When the web view is unrealized, for example if the web view is reparented, the redirected window is destroyed, and a sync message is sent to the web process to destroy the current gl context and stop drawing. This needs to happen synchronously, because the UI process can't remove the redirected window until the web process has stopped rendering into it. This makes also the reparenting process quite unstable and risky. To all those synchronization points we now have to add the synchronization with the compositing thread when using the threaded compositor. The threaded compositor made resizing, reparenting, etc. even worse. We can't avoid the synchronization with the threaded compositor, but we can reduce the synchronization points and improve the current ones by moving the redirected window to the web process. In this case is web process who created the redirected window, so we can be sure that it always has a valid native surface handle to render into. This means we no longer need the IPC message to send the native surface handle from the UI process to the web process, nor the sync message to destroy it either. This also means we no longer need to wait until the view is realized to start rendering accelerated contents, and we don't need to stop when it's unrealized either. We don't really need to inherit the XVisual from the parent window if the redirected window always uses always an RGBA visual when available. That way we always render into a transparent window that is composed into the web view widget. And when the web view is resized, we no longer need to destroy the GL context either, because we use the same redirected window as the native handle, but create a new pixmap that is what we send to the UI process as layer tree context ID. The layer tree context ID is already sent to the UI process by the drawing area as part of the backing store update process, so we don't need any new IPC message for this. When the web view is resized, the UI process sends a backing store state update message to the web process that updates its size, relayouts and then renders the new contents, so that when the update backing store state reply gets to the UI process, we already have a new pixmap with the new contents updated. This makes resizing smooth again, and avoids flickering and rendering artifacts. And finally all this also prevents several race conditions that were causing X errors and web process crashes.

[Carlos Garcia Campos]

Created attachment 284964 [details]
Patch

[Michael Catanzaro]

Comment on attachment 284964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284964&action=review

So it makes AC mode not suck? :)

This one really should be looked at by a graphics reviewer; if it gets stuck awaiting review, you can ping me after a few days.

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:66
> +bool PlatformDisplayX11::supportsXComposite()

Can you make it const, as it should be, if you make m_supportsXComposite mutable? Or does the call to XCompositeQueryExtension prevent that?

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:75
> +bool PlatformDisplayX11::supportsXDamage(int& damageEventBase)

Can you make it const, as it should be, if you make m_damageEventBase mutable? Or does the call to XDamageQueryExtension prevent that?

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:79
> +        m_supportsXDamage = XDamageQueryExtension(m_display, &eventBase, &errorBase);

EFL build is failing here:

lib/libwebcore_efl.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/x11/PlatformDisplayX11.cpp.o):PlatformDisplayX11.cpp:function WebCore::PlatformDisplayX11::supportsXDamage(int&): error: undefined reference to 'XDamageQueryExtension'
collect2: error: ld returned 1 exit status

We link with ${X11_Xdamage_LIB} in WebCore/PlatformGtk.cmake, but EFL does not; you'll need to add it to WebCore_LIBRARIES.

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:75
> +GdkFilterReturn XDamageNotifier::filterXDamageEvent(GdkXEvent* event, GdkEvent*, XDamageNotifier* notifier)

Since it doesn't access any member variables, I would move it outside the class.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 284964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284964&action=review
> 
> So it makes AC mode not suck? :)

Yes.

> This one really should be looked at by a graphics reviewer; if it gets stuck
> awaiting review, you can ping me after a few days.

Thanks!

> > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:66
> > +bool PlatformDisplayX11::supportsXComposite()
> 
> Can you make it const, as it should be, if you make m_supportsXComposite
> mutable? Or does the call to XCompositeQueryExtension prevent that?

I'll try.

> > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:75
> > +bool PlatformDisplayX11::supportsXDamage(int& damageEventBase)
> 
> Can you make it const, as it should be, if you make m_damageEventBase
> mutable? Or does the call to XDamageQueryExtension prevent that?
> 
> > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:79
> > +        m_supportsXDamage = XDamageQueryExtension(m_display, &eventBase, &errorBase);
> 
> EFL build is failing here:

I'll fix the build.

> lib/libwebcore_efl.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/
> graphics/x11/PlatformDisplayX11.cpp.o):PlatformDisplayX11.cpp:function
> WebCore::PlatformDisplayX11::supportsXDamage(int&): error: undefined
> reference to 'XDamageQueryExtension'
> collect2: error: ld returned 1 exit status
> 
> We link with ${X11_Xdamage_LIB} in WebCore/PlatformGtk.cmake, but EFL does
> not; you'll need to add it to WebCore_LIBRARIES.
> 
> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:75
> > +GdkFilterReturn XDamageNotifier::filterXDamageEvent(GdkXEvent* event, GdkEvent*, XDamageNotifier* notifier)
> 
> Since it doesn't access any member variables, I would move it outside the
> class.

Then I would need to make notify public which is not.

[Carlos Garcia Campos]

Created attachment 284987 [details]
Address review comments and try to fix EFL build

[Sergio Villar Senin]

Comment on attachment 284987 [details]
Address review comments and try to fix EFL build

View in context: https://bugs.webkit.org/attachment.cgi?id=284987&action=review

Looking great to me. I will let Zan or Martin review it since there is stuff like what happens when there is no redirected window that is not completely clear to me.

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:80
> +#if PLATFORM(GTK)

Why this? It's just handling X stuff

> Source/WebKit2/ChangeLog:40
> +        all this also prevents several race conditions that were causing X errors and web process crashes.

This is an amazing description, but too much for a single paragraph to my taste. It'd be awesome if you could split it a bit.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:57
> +        m_scene->setActive(!!m_nativeSurfaceHandle);

Do we really need the !! ?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> +        return;

Is this even possible knowing that the whole method is guarded by REDIRECTED_XCOMPOSITE_WINDOW ?

> Source/WebKit2/UIProcess/PageClient.h:-238
> -    virtual void willEnterAcceleratedCompositingMode() = 0;

Were we the unique users of this?

> Source/WebKit2/UIProcess/WebPageProxy.h:-827
> -    void willEnterAcceleratedCompositingMode();

Ditto.

> Source/WebKit2/UIProcess/efl/WebView.h:-224
> -    void willEnterAcceleratedCompositingMode() override { }

Ditto.

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:40
> +static int s_damageEventBase = -1;

Time to switch to Optional<int> ?

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:57
> +    if (s_damageEventBase == -1)

...and change this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:67
> +    if (s_damageEventBase == -1)

and this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:77
> +    ASSERT(s_damageEventBase != -1);

and this

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:82
> +    XDamageNotifyEvent* damageEvent = reinterpret_cast<XDamageNotifyEvent*>(xEvent);

You can use auto* there

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:114
>      WebCore::IntPoint m_lastScrollPosition;

I'm having a hard time to understand the difference between this two, perhaps we need better naming?

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 284987 [details]
> Address review comments and try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284987&action=review
> 
> Looking great to me. I will let Zan or Martin review it since there is stuff
> like what happens when there is no redirected window that is not completely
> clear to me.

Thanks for looking at it. The situation when the there's no redirected window is that nothing will be rendered (like with current code). It's not expected to happen, it would mean that we are running in a very very old version of the xserver. In that case nothing will be rendered, but hopefully it won't crash either. Ideally we would not allow to build with that unsupported configuration, and we indeed fail if XComposite extension is not available, but the actual version needs to be checked at runtime. So, I wouldn't worry about such situation.

> > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:80
> > +#if PLATFORM(GTK)
> 
> Why this? It's just handling X stuff

Yes, but only the GTK+ port depends on it. That's what we do in all other places where XDamage is used. I agree we could add a specific build option for that instead, but that would be a separate bug, since we are already using GTK ifdefs form Xdamage everywhere.

> > Source/WebKit2/ChangeLog:40
> > +        all this also prevents several race conditions that were causing X errors and web process crashes.
> 
> This is an amazing description, but too much for a single paragraph to my
> taste. It'd be awesome if you could split it a bit.
> 
> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:57
> > +        m_scene->setActive(!!m_nativeSurfaceHandle);
> 
> Do we really need the !! ?

No, but I think it's a clearer way to convert to bool.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> > +        return;
> 
> Is this even possible knowing that the whole method is guarded by
> REDIRECTED_XCOMPOSITE_WINDOW ?

Please, add more lines to the context, I only see a return here. Ok, that's the display check. Yes, it's needed, because the REDIRECTED_XCOMPOSITE_WINDOW guard is at compile time, but we build wayland and X11 support at the same time, we need to check the actual display at runtime to took one path or the other.

> > Source/WebKit2/UIProcess/PageClient.h:-238
> > -    virtual void willEnterAcceleratedCompositingMode() = 0;
> 
> Were we the unique users of this?

Yes, I added this to fix one of the issues of having the redirected window in the UI process.

> > Source/WebKit2/UIProcess/WebPageProxy.h:-827
> > -    void willEnterAcceleratedCompositingMode();
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/efl/WebView.h:-224
> > -    void willEnterAcceleratedCompositingMode() override { }
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:40
> > +static int s_damageEventBase = -1;
> 
> Time to switch to Optional<int> ?

Not sure it's worth it for a global static variable like this. And I'm not sure I can pass the mem address to the query method to get the value.

> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:57
> > +    if (s_damageEventBase == -1)
> 
> ...and change this
> 
> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:67
> > +    if (s_damageEventBase == -1)
> 
> and this
> 
> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:77
> > +    ASSERT(s_damageEventBase != -1);
> 
> and this
> 
> > Source/WebKit2/UIProcess/gtk/XDamageNotifier.cpp:82
> > +    XDamageNotifyEvent* damageEvent = reinterpret_cast<XDamageNotifyEvent*>(xEvent);
> 
> You can use auto* there

Yes.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:114
> >      WebCore::IntPoint m_lastScrollPosition;
> 
> I'm having a hard time to understand the difference between this two,
> perhaps we need better naming?

Again, single line in the context, let check which two you mean. Ah, you mena prev, and last. I have no idea, I only re-ordered to initialize the redirected window first in the constructor, those have nothing to do with this patch.

[Carlos Garcia Campos]

Created attachment 284991 [details]
Address more review comments

[Andres Gomez Garcia]

I'm using WebKitGtk+ with my own JHBuild setting:
https://github.com/tanty/jhbuild-epiphany/tree/master

Epiphany 3.20.3 and WebKit 2.13.4 with this patch applied.

I'm running Epiphany with the dconf key:

"process-model" = "shared-secondary-process"

The compilation was done with CMake args:

'-DPORT=GTK -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS_RELEASE="-O0 -g1 -DNDEBUG -DG_DISABLE_CAST_CHECKS" -DCMAKE_CXX_FLAGS_RELEASE="-O0 -g1 -DNDEBUG -DG_DISABLE_CAST_CHECKS"'


OBSERVATIONS
============

My saved session has several windows with several tabs each.

Upon re-starting the epiphany, the whole desktop freezes after the appearing of the first(s) window(s)

Slowly, it starts go show that it is not completely frozen but gets excruciatingly slow.

The new Epiphany windows keep popping one by one veeeery slowly. The whole process takes minutes.

After, seemingly, all the windows have been created, the desktop doesn't recover its normal speed, it still moves almost as if it would be frozen.

Finally, I decided to use GDB.

Attaching GDB to Epiphany and the NetworkProcess was OK.

Attaching GDB to the WebProcess froze my whole desktop. There was not recovery after this, only rebooting after waiting for several minutes.

[Zan Dobersek]

Comment on attachment 284991 [details]
Address more review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=284991&action=review

> Source/WebKit2/UIProcess/gtk/XDamageNotifier.h:46
> +    void add(Damage, std::function<void()>&&);

WTF::Function<> is preferable now I think.

[Carlos Garcia Campos]

Committed r204013: <http://trac.webkit.org/changeset/204013>