1289251 - Create a CompositorWidget for GTK+X11 Platform

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ryan Hunt [:rhunt]]

Create a CompositorWidget to enable the GPU process on the GTK+X11 platform.

Comment 1

[Ryan Hunt [:rhunt]]

Attachment: Create a CompositoWidget for GTK+X11 platform

This patch creates an X11CompositorWidget for use on the GTK+X11 platform. Much of the code has been lifted from gtk/nsWindow. It's behaviour should be identical to what we'd get with an InProcessCompositorWidget, but it should also function in an out of process context. One big implication of this is that we cannot use gfxPlatform when we are out of process.

Comment 2

[Andrew Comminos [:acomminos]]

Comment on attachment 8774540 [details] [diff] [review]
Create a CompositoWidget for GTK+X11 platform

Review of attachment 8774540 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this work!

It's worth noting that we're looking to support Wayland in the future; you may want to reconsider use of Xlib APIs in favour of GDK ones for portability's sake. I'm not sure we want to maintain two different CompositorWidget backends for Linux.

r?jrmuizel for IPDL changes.

::: widget/CompositorWidget.h
@@ +51,5 @@
>  
>  # define MOZ_WIDGET_SUPPORTS_OOP_COMPOSITING
>  #endif
>  
> +#if defined(XP_UNIX) && defined(MOZ_X11)

Is there a need to keep this distinct from the previous defined(XP_WIN) block?

#if defined(XP_WIN) ||
   (defined(MOZ_WIDGET_GTK) && defined(MOZ_X11))

would be best.

::: widget/gtk/X11CompositorWidget.cpp
@@ +26,5 @@
> +      : mWidget(aWindow)
> +      , mXDisplay(nullptr)
> +      , mXWindow((Window)aInitData.XWindow())
> +      , mXVisual(nullptr)
> +      , mXDepth(0)

Shouldn't need to initialize some of these twice in the constructor (mXDisplay, mXVisual, mXDepth).

@@ +40,5 @@
> +  // Grab the window's visual and depth
> +  XWindowAttributes windowAttrs;
> +  XGetWindowAttributes(mXDisplay, mXWindow, &windowAttrs);
> +  mXVisual = windowAttrs.visual;
> +  mXDepth = windowAttrs.depth;

XGetWindowAttributes as well as XOpenDisplay may fail. Even though we probably can't handle that gracefully (especially not in a constructor), it might be a good idea to MOZ_ASSERT their sanity.

@@ +77,5 @@
> +    return MakeUnique<WindowSurfaceXRender>(mXDisplay, mXWindow, mXVisual, mXDepth);
> +  }
> +#endif // MOZ_WIDGET_GTK
> +
> +  Display* display;

Since mXDisplay is set to the widget's compositor display in the constructor, we can always use mXDisplay for WindowSurfaces.

@@ +82,5 @@
> +  if (mWidget && CompositorThreadHolder::IsInCompositorThread()) {
> +    display = gfxPlatformGtk::GetPlatform()->GetCompositorDisplay();
> +  } else {
> +    display = mXDisplay;
> +  }

This code is no longer needed; always use mXDisplay.

@@ +131,5 @@
> +{
> +  if (!mWidget) {
> +    XWindowAttributes windowAttrs;
> +    XGetWindowAttributes(mXDisplay, mXWindow, &windowAttrs);
> +    return LayoutDeviceIntSize(windowAttrs.width, windowAttrs.height);

As this approach requires a round trip to the X server, this is going to have a rather negative impact on our remote X performance depending on how much GetClientSize is used.

Can we have mWidget communicate to the X11CompositorWidget when the widget size changes instead, and cache it?

::: widget/gtk/X11CompositorWidget.h
@@ +8,5 @@
> +
> +#include "mozilla/widget/CompositorWidget.h"
> +#include "mozilla/widget/WindowSurface.h"
> +
> +#include <gdk/gdkx.h> // for Window, Display, Visual, etc.

Not too important, but <X11/Xlib.h> may be a better choice here.

::: widget/gtk/nsWindow.cpp
@@ -6561,5 @@
>  }
>  #endif
>  
> -already_AddRefed<DrawTarget>
> -nsWindow::StartRemoteDrawingInRegion(LayoutDeviceIntRegion& aInvalidRegion, BufferMode* aBufferMode)

nsWindow::StartRemoteDrawingInRegion is also used in nsWindow::OnExposeEvent for painting to windows with the basic layer manager- the removal of this will break it. The WindowSurface instantiation code should be refactored to be accessible to both X11CompositorWidget and nsWindow::OnExposeEvent.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Andrew Comminos [:acomminos] from comment #2)

> As this approach requires a round trip to the X server, this is going to
> have a rather negative impact on our remote X performance depending on how
> much GetClientSize is used.
> 
> Can we have mWidget communicate to the X11CompositorWidget when the widget
> size changes instead, and cache it?

Assuming we only need to call GetClientSize once per composite, how would this compare to a bunch of IPC traffic during a resize operation? (Does Gtk send a bunch of resize requests or does it just send one when the resize stops?)

Comment 4

[Andrew Comminos [:acomminos]]

(In reply to David Anderson [:dvander] from comment #3)
> (In reply to Andrew Comminos [:acomminos] from comment #2)
> 
> > As this approach requires a round trip to the X server, this is going to
> > have a rather negative impact on our remote X performance depending on how
> > much GetClientSize is used.
> > 
> > Can we have mWidget communicate to the X11CompositorWidget when the widget
> > size changes instead, and cache it?
> 
> Assuming we only need to call GetClientSize once per composite, how would
> this compare to a bunch of IPC traffic during a resize operation? (Does Gtk
> send a bunch of resize requests or does it just send one when the resize
> stops?)

Over a local X connection (the common case, using a unix domain socket) this shouldn't be much more expensive than any other IPC call given that X traffic is fairly minimal. However, if the user is using X over the network (which is a surprisingly common use of Firefox) this can be very substantial; on the magnitude of milliseconds and above.

To answer your second question- GTK reports a bunch of resize requests as the user resizes a window.

Comment 5

[Ryan Hunt [:rhunt]]

(In reply to Andrew Comminos [:acomminos] from comment #2)
> ::: widget/gtk/nsWindow.cpp
> @@ -6561,5 @@
> >  }
> >  #endif
> >  
> > -already_AddRefed<DrawTarget>
> > -nsWindow::StartRemoteDrawingInRegion(LayoutDeviceIntRegion& aInvalidRegion, BufferMode* aBufferMode)
> 
> nsWindow::StartRemoteDrawingInRegion is also used in nsWindow::OnExposeEvent
> for painting to windows with the basic layer manager- the removal of this
> will break it. The WindowSurface instantiation code should be refactored to
> be accessible to both X11CompositorWidget and nsWindow::OnExposeEvent.

Hmm. I'm not sure how we should handle this. When the CompWidget is in process that's fine, we can just share the WindowSurface with nsWindow. But if the CompWidget is out of process we would need two WindowSurfaces, one in the main process, and one in gpu process? And they'd both render asyncronously through their own WindowSurface? I honestly don't know whether that would work or not. 

If I understand the code right, ExposeEvent triggers a redraw of a region. Would it make sense in OMTC situations to just post a composite to the compositor and have that go through CompositorWidget code to StartRemoteDrawing?

Comment 6

[Andrew Comminos [:acomminos]]

(In reply to Ryan Hunt [:rhunt] from comment #5)
> Hmm. I'm not sure how we should handle this. When the CompWidget is in
> process that's fine, we can just share the WindowSurface with nsWindow. But
> if the CompWidget is out of process we would need two WindowSurfaces, one in
> the main process, and one in gpu process? And they'd both render
> asyncronously through their own WindowSurface? I honestly don't know whether
> that would work or not. 

A widget will either draw in the expose event handler, or via OMTC- not both. We just need to ensure that for widgets that aren't composited using OMTC (i.e. not using OOP composition), we still can paint to them.

There is one catch, however- if the transparency state of a window changes after creation, we'll destroy our layer manager and switch to basic layers (which we require in order to readback the alpha channel into XSHAPE). I don't think it matters too much if this breaks, :jrmuizel has told me that he's looking to make a widget's transparency state fixed after creation on Windows anyway.

> If I understand the code right, ExposeEvent triggers a redraw of a region.
> Would it make sense in OMTC situations to just post a composite to the
> compositor and have that go through CompositorWidget code to
> StartRemoteDrawing?

Right, that's what we do now; we exit early from OnExposeEvent and schedule a composite if using client layers / OMTC.

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review commit: https://reviewboard.mozilla.org/r/71068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71068/

Comment 8

[Ryan Hunt [:rhunt]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=130852c26470&selectedJob=25592873

Comment 9

[Ryan Hunt [:rhunt]]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71068/diff/1-2/

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

Review commit: https://reviewboard.mozilla.org/r/71724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71724/

Comment 11

[Ryan Hunt [:rhunt]]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71068/diff/2-3/

Comment 12

[Ryan Hunt [:rhunt]]

Attachment: Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

Review commit: https://reviewboard.mozilla.org/r/71740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71740/

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

https://reviewboard.mozilla.org/r/71068/#review71268

Comment 14

[Andrew Comminos [:acomminos]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

https://reviewboard.mozilla.org/r/71724/#review71502

Looks good to me, thanks! r?jrmuizel for ipdl changes.

::: widget/CompositorWidget.h:43
(Diff revision 1)
>  // this functionality is platform-dependent, it is only forward declared
>  // here.
>  class CompositorWidgetDelegate;
>  
>  // Platforms that support out-of-process widgets.
> -#if defined(XP_WIN)
> +#if defined(XP_WIN) || (defined(XP_UNIX) && defined(MOZ_X11))

I don't see how XP_UNIX is relevant here (just a nit).

::: widget/gtk/WindowSurfaceProvider.cpp:40
(Diff revision 1)
> +{
> +  // We should not be initialized
> +  MOZ_ASSERT(mXDisplay == nullptr);
> +
> +  // This should also be a valid initialization
> +  MOZ_ASSERT(aDisplay && aVisual);

Include `aWindow != None` in this assert.

::: widget/gtk/WindowSurfaceProvider.cpp:54
(Diff revision 1)
> +  mWindowSurface = nullptr;
> +}
> +
> +UniquePtr<WindowSurface>
> +WindowSurfaceProvider::CreateWindowSurface()
> +{

Should assert that SetSource has been called here.

Comment 15

[Andrew Comminos [:acomminos]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

https://reviewboard.mozilla.org/r/71740/#review71508

Looks good to me, but jgilbert should review gfx/gl changes.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

https://reviewboard.mozilla.org/r/71740/#review71564

::: gfx/gl/GLContextProviderGLX.cpp:1135
(Diff revision 1)
>  }
>  
> +already_AddRefed<GLContext>
> +GLContextProviderGLX::CreateForCompositorWidget(CompositorWidget* aCompositorWidget, bool aForceAccelerated)
> +{
> +    X11CompositorWidget* compWidget = aCompositorWidget->AsX();

Why not just pass in the correct type to begin with?

::: gfx/gl/GLContextProviderGLX.cpp:1136
(Diff revision 1)
>  
> +already_AddRefed<GLContext>
> +GLContextProviderGLX::CreateForCompositorWidget(CompositorWidget* aCompositorWidget, bool aForceAccelerated)
> +{
> +    X11CompositorWidget* compWidget = aCompositorWidget->AsX();
> +    MOZ_ASSERT(compWidget != nullptr);

Don't use MOZ_ASSERT(ptr != nullptr). Just do MOZ_ASSERT(ptr).

::: gfx/gl/GLContextProviderGLX.cpp:1139
(Diff revision 1)
> +{
> +    X11CompositorWidget* compWidget = aCompositorWidget->AsX();
> +    MOZ_ASSERT(compWidget != nullptr);
> +
> +    return CreateForWidget(
> +                    compWidget->XDisplay(),

Overflow the args on this normally.

::: widget/gtk/X11CompositorWidget.h:53
(Diff revision 1)
>    nsIWidget* RealWidget() override;
>    X11CompositorWidget* AsX() override { return this; }
>    CompositorWidgetDelegate* AsDelegate() override { return this; }
>  
> +  Display* XDisplay() { return mXDisplay; }
> +  Window XWindow() { return mXWindow; }

These should both be const methods.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

https://reviewboard.mozilla.org/r/71740/#review71566

Comment 18

[Ryan Hunt [:rhunt]]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71068/diff/3-4/

Comment 19

[Ryan Hunt [:rhunt]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71724/diff/1-2/

Comment 20

[Ryan Hunt [:rhunt]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71740/diff/1-2/

Comment 21

[Ryan Hunt [:rhunt]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

https://reviewboard.mozilla.org/r/71740/#review71564

> Why not just pass in the correct type to begin with?

We can't do that because it's a method on GLContextProvider which takes in just a CompositorWidget.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

https://reviewboard.mozilla.org/r/71724/#review73226

::: widget/CompositorWidget.h:239
(Diff revision 2)
>    virtual RefPtr<VsyncObserver> GetVsyncObserver() const;
>  
>    virtual WinCompositorWidget* AsWindows() {
>      return nullptr;
>    }
> +  virtual X11CompositorWidget* AsX() {

Let's make this AsX11 to make it a little less ambiguous.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

https://reviewboard.mozilla.org/r/71724/#review73228

Comment 24

[Ryan Hunt [:rhunt]]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71068/diff/4-5/

Comment 25

[Ryan Hunt [:rhunt]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71724/diff/2-3/

Comment 26

[Ryan Hunt [:rhunt]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71740/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2467398cd958
Replace GetDefaultContentBackend in nShmImage to use gfxVar. r=dvander
https://hg.mozilla.org/integration/autoland/rev/c0285428a8a0
Create a CompositorWidget for GTK+X11 platform. r=acomminos,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/2df13967b746
Allow GLContextProviderGLX to create a GLContext out of process. r=acomminos,jgilbert

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage in WindowSurfaceX11.cpp:

https://hg.mozilla.org/integration/autoland/rev/84bab1407100af7215a1f19df176295fc2287410
https://hg.mozilla.org/integration/autoland/rev/f35a9b2b20910e76bdd784070c245bef40ece975
https://hg.mozilla.org/integration/autoland/rev/3e28c1a68c2475acd925f233379f8dae8b5ddd7a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2df13967b7466c92cb629e3600a101a1826dfad8
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=2967541&repo=autoland

> build/src/widget/gtk/WindowSurfaceX11.cpp:22:9: error: 'None' was not declared in this scope 
Please provide a completed Try run the next time you set checkin-needed. Thanks

Comment 29

[Ryan Hunt [:rhunt]]

It looks like it was an issue with the landing of bug 1288686 and unified sources.

I did do a try run, but it looks like the timing was just bad, https://treeherder.mozilla.org/#/jobs?repo=try&revision=3056e384359d.

Here's another try run with the fixes applied, https://treeherder.mozilla.org/#/jobs?repo=try&revision=48f1df5390c9.

Comment 30

[Ryan Hunt [:rhunt]]

Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71068/diff/5-6/

Comment 31

[Ryan Hunt [:rhunt]]

Comment on attachment 8781317 [details]
Bug 1289251 - Create a CompositorWidget for GTK+X11 platform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71724/diff/3-4/

Comment 32

[Ryan Hunt [:rhunt]]

Comment on attachment 8781326 [details]
Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71740/diff/3-4/

Comment 33

[Ryan Hunt [:rhunt]]

It looks like the builds are green again, and I updated the patches in mozreview.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66d2073839bb
Replace GetDefaultContentBackend in nShmImage to use gfxVar. r=dvander
https://hg.mozilla.org/integration/autoland/rev/be4e5ce4a8a4
Create a CompositorWidget for GTK+X11 platform. r=acomminos,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/148543c54d26
Allow GLContextProviderGLX to create a GLContext out of process. r=acomminos,jgilbert

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66d2073839bb
https://hg.mozilla.org/mozilla-central/rev/be4e5ce4a8a4
https://hg.mozilla.org/mozilla-central/rev/148543c54d26