Create a CompositorWidget for GTK+X11 Platform

RESOLVED FIXED in Firefox 51

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Create a CompositorWidget to enable the GPU process on the GTK+X11 platform.
(Assignee)

Comment 1

2 years ago
Created attachment 8774540 [details] [diff] [review]
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.
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.
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8774540 - Flags: review?(jmuizelaar)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8774540 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780340 - Attachment is obsolete: true
Attachment #8780340 - Flags: review?(dvander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 14

2 years ago
mozreview-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 15

2 years ago
mozreview-review
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 16

2 years ago
mozreview-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 17

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
mozreview-review-reply
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.
(Assignee)

Updated

2 years ago
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 27

2 years ago
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
(Assignee)

Comment 29

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

2 years ago
It looks like the builds are green again, and I updated the patches in mozreview.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 34

2 years ago
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

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66d2073839bb
https://hg.mozilla.org/mozilla-central/rev/be4e5ce4a8a4
https://hg.mozilla.org/mozilla-central/rev/148543c54d26
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1304659
You need to log in before you can comment on or make changes to this bug.