Closed Bug 1289251 Opened 3 years ago Closed 3 years ago

Create a CompositorWidget for GTK+X11 Platform

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Attachment #8774540 - Flags: review?(andrew)
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.
Attachment #8774540 - Flags: review?(andrew) → review-
Attachment #8774540 - Flags: review?(jmuizelaar)
(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.
Attachment #8774540 - Flags: review?(jmuizelaar)
Attachment #8774540 - Attachment is obsolete: true
Attachment #8780340 - Attachment is obsolete: true
Attachment #8780340 - Flags: review?(dvander)
Comment on attachment 8780340 [details]
Bug 1289251 - Replace GetDefaultContentBackend in nShmImage to use gfxVar.

https://reviewboard.mozilla.org/r/71068/#review71268
Attachment #8780340 - Flags: review?(dvander) → review+
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.
Attachment #8781317 - Flags: review?(andrew) → review+
Attachment #8781317 - Flags: review?(jmuizelaar)
Attachment #8781326 - Flags: review?(jgilbert)
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.
Attachment #8781326 - Flags: review?(andrew) → review+
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
Attachment #8781326 - Flags: review?(jgilbert) → review+
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.
Summary: Create an CompositorWidget for GTK+X11 Platform → Create a CompositorWidget for GTK+X11 Platform
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
Attachment #8781317 - Flags: review?(jmuizelaar) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
Flags: needinfo?(ryanhunt)
It looks like the builds are green again, and I updated the patches in mozreview.
Keywords: checkin-needed
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
Keywords: checkin-needed
Depends on: 1304199
Depends on: 1304659
You need to log in before you can comment on or make changes to this bug.