897502 - Force OMTC when electrolysis is enabled

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: PreferBsicPrefs.patch

This patch forces OMTC on whenever browser.tabs.remote is true. It also moves prefer-basic logic to nsBaseWidget so it can be used everywhere.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 780428 [details] [diff] [review]
PreferBsicPrefs.patch

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

::: gfx/thebes/gfxPlatform.cpp
@@ +342,3 @@
>  
> +    if (!requireOMTC)
> +      useOffMainThreadCompositing &= GetPlatform()->SupportsOffMainThreadCompositing();

The problem with ignoring this is that we sometimes really can't handle OMTC.

For OSX 10.6 we're possibly setting the user up for random crashes. On linux, if the user hasn't specified MOZ_USE_OMTC in their env, then Xlib hasn't been initialized in threaded mode and we're going to crash more or less immediately.

That's not a good experience.

I think we can fix the former by adding code to widget to make sure the 10.6 users with e10s enabled get the basic compositor (even if ComputeShouldAccelerate returns true). 10.6 users with OMTC enabled but not e10s should get OMTC disabled (same as current behaviour).

For linux I think we need to make MOZ_USE_OMTC enabled always (http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3379).

Then this function can go away entirely and all is well.

The alternative is to refuse e10s in these situations instead.

Comment 2

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> For OSX 10.6 we're possibly setting the user up for random crashes. On
> linux, if the user hasn't specified MOZ_USE_OMTC in their env, then Xlib
> hasn't been initialized in threaded mode and we're going to crash more or
> less immediately.

Are the problems with 10.6 with OpenGL, or with OMTC at all? If it's with OGL, it seems like the logic in SupportsOMTC should just be duplicated into ComputeShouldAccelerate.

> For linux I think we need to make MOZ_USE_OMTC enabled always
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.
> cpp#3379).

Okay.

Comment 3

[Matt Woodrow (:mattwoodrow)]

It's probably with OpenGL, but it's specific to the way we use it with OMTC.

In-thread GL is working fine for those users, so we don't want to have ComputeShouldAccelerate return false.

Comment 4

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

Attachment: PreferBsicPrefs.patch

Like this?

Comment 5

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

Attachment: v2 qref'd

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 780500 [details] [diff] [review]
v2 qref'd

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

::: gfx/thebes/gfxPlatform.cpp
@@ +342,3 @@
>  
> +    if (!OffMainThreadCompositionRequired())
> +      useOffMainThreadCompositing &= GetPlatform()->SupportsOffMainThreadCompositing();

The only implementation of this function is now the gfxPlatformMac, and we handle that in nsChildView.mm. Get rid of it and this check.

@@ +1886,5 @@
>  
> +bool gfxPlatform::OffMainThreadCompositionRequired()
> +{
> +  InitLayersAccelerationPrefs();
> +  return sPrefLayersAccelerationForceEnabled || sPrefBrowserTabsRemote;

Force enabling acceleration shouldn't also force OMTC?

Comment 7

[Nick Cameron [:nrc]]

Repeated comment from https://bugzilla.mozilla.org/show_bug.cgi?id=901382#c4

Why do we need a new pref for basic compositor? Isn't this implied by OMTC on and layers.acceleration off?

I also think having these lines in nsBaseWidget is wrong. GetPreferredCompositorBackend should return basic if it can't use HW compositors (that is, it should check if layers acceleration is enabled).

Comment 8

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

Attachment: v3

w/ prefer-basic pref removed

Comment 9

[Nick Cameron [:nrc]]

Comment on attachment 785928 [details] [diff] [review]
v3

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

karl: do you recall the motivation for not using |XInitThreads()| by default?

::: gfx/thebes/gfxPlatform.cpp
@@ +354,2 @@
>  
> +    if (!OffMainThreadCompositionRequired())

nit: {}

@@ +1925,5 @@
>  
> +bool gfxPlatform::OffMainThreadCompositionRequired()
> +{
> +  InitLayersAccelerationPrefs();
> +  return sPrefLayersAccelerationForceEnabled || sPrefBrowserTabsRemote;

Why is OMTC required if we force on HWA? I think we might want main thread there, for (e.g.) OGL on Linux.

::: toolkit/xre/nsAppRunner.cpp
@@ +3372,5 @@
>    // Init X11 in thread-safe mode. Must be called prior to the first call to XOpenDisplay 
>    // (called inside gdk_display_open). This is a requirement for off main tread compositing.
> +  // We initialize this eagerly since we don't have access to prefs at this point, and can't
> +  // determine whether OMTC is required.
> +  XInitThreads();

I'm not sure about this change, we looked at doing it before and there were reasons not to. CCing karl for this.

Comment 10

[Nick Cameron [:nrc]]

Also, Matt's comment about SupportsOffMainThreadCompositing() - I wasn't sure if we needed the widget and gfxPlatform versions for Mac, but it sounds like we don't. If it is right to always call XInitThreads, then we should remove SupportsOffMainThreadCompositing.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 785928 [details] [diff] [review]
v3

(In reply to Nick Cameron [:nrc] from comment #9)
> karl: do you recall the motivation for not using |XInitThreads()| by default?

Bug 760228 describes the issues.  I think we no longer need to be concerned with the XSynchronize lock up.  However, there is still a performance cost and some risk.  This is only worthwhile if there is significant benefit.  I haven't seen any movement on bug 722012 or its list of dependencies for a while, and so I assume that the user benefit is not near on the horizon and it is not yet time to turn on this cost.

The environment variable still seems to be the right approach for the X11 platform.

Comment 12

[Karl Tomlinson (:karlt)]

I wonder whether it is possible to move the Display opening code from
nsAppRunner to widget code, perhaps running it when the first window is
created, so that prefs are available.

IIRC there is an invisible window created early in startup, which
would trigger this.  Hopefully that is before any canvas or anything else would want to use X11.

gtk_widget_set_default_colormap(), InstallX11ErrorHandler(), and the GNOME
stuff in nsNativeAppSupportUnix.cpp would all need to move.

XRemoteClient would need to open its own Display.

Comment 13

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

Attachment: omtc.patch

Consensus was to only call XInitThreads() by default on nightly builds, since it had about a ~5% tpaint regression on 32-bit Ubuntu.

Comment 14

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

Attachment: omtc.patch

err, with the correct patch

Comment 15

[Nick Cameron [:nrc]]

Comment on attachment 790467 [details] [diff] [review]
omtc.patch

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

Mostly looks good. There are a couple of nits in comments. But I'm not sure about the changes to nsChildView.mm - I think with the recent changes from bug 901382, we don't to change nsChildView, but I might be wrong.

::: toolkit/xre/nsAppRunner.cpp
@@ +3370,5 @@
>    // (called inside gdk_display_open). This is a requirement for off main tread compositing.
>    // This is done only on X11 platforms if the environment variable MOZ_USE_OMTC is set so 
>    // as to avoid overhead when omtc is not used. 
> +  //
> +  // On nightly builds, we call this by default to enable OMTC for Electrolysis dtesting. On

nit: dtesting

@@ +3372,5 @@
>    // as to avoid overhead when omtc is not used. 
> +  //
> +  // On nightly builds, we call this by default to enable OMTC for Electrolysis dtesting. On
> +  // aurora, beta, and release builds, there is a small tpaint regression from enabling this
> +  // call, so it sits behind a runtime pref.

Looks like an env var not a runtime pref

::: widget/cocoa/nsChildView.mm
@@ +1530,5 @@
>      return false;
>  
> +  // Don't accelerate if we're on 10.6 and are requiring OMTC.
> +  if (gfxPlatformMac::GetPlatform()->OSXVersion() < 0x1070 &&
> +      gfxPlatform::GetPlatform()->OffMainThreadCompositionRequired()) {

Can we add this check to gfxPlatformMac::SupportsOffMainThreadCompositing, and then not change nsChildView at all? I don't think these changes are necessary to force OMTC.

Comment 16

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

Attachment: patch

Yeah I think you're right, the nsChildView.mm changes aren't needed at all.

Comment 17

[Nick Cameron [:nrc]]

Comment on attachment 790939 [details] [diff] [review]
patch

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

lgtm

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/275a38cd4caf

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/275a38cd4caf