Create a CompositorWidget to enable the GPU process on the 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 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.

(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?)

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

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

(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 on attachment 8780340 [details] Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar. https://reviewboard.mozilla.org/r/71068/#review71268

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 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 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 on attachment 8781326 [details] Bug 1289251 - Allow GLContextProviderGLX to create a GLContext out of process. https://reviewboard.mozilla.org/r/71740/#review71566

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 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 on attachment 8781317 [details] Bug 1289251 - Create a CompositorWidget for GTK+X11 platform. https://reviewboard.mozilla.org/r/71724/#review73228

Pushed by archaeopteryx@coole-files.de: 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

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

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.

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

Pushed by ryanvm@gmail.com: 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

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