Closed Bug 897502 Opened 8 years ago Closed 8 years ago

Force OMTC when electrolysis is enabled

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch PreferBsicPrefs.patch (obsolete) — Splinter Review
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.
Attachment #780428 - Flags: review?(matt.woodrow)
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.
(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.
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.
Attached patch PreferBsicPrefs.patch (obsolete) — Splinter Review
Like this?
Attachment #780428 - Attachment is obsolete: true
Attachment #780428 - Flags: review?(matt.woodrow)
Attachment #780498 - Flags: review?(matt.woodrow)
Attached patch v2 qref'd (obsolete) — Splinter Review
Attachment #780498 - Attachment is obsolete: true
Attachment #780498 - Flags: review?(matt.woodrow)
Attachment #780500 - Flags: review?(matt.woodrow)
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?
Attachment #780500 - Flags: review?(matt.woodrow) → review+
Component: Graphics → Graphics: Layers
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).
Attached patch v3 (obsolete) — Splinter Review
w/ prefer-basic pref removed
Attachment #780500 - Attachment is obsolete: true
Attachment #785928 - Flags: review?(ncameron)
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.
Attachment #785928 - Flags: feedback?(karlt)
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 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.
Attachment #785928 - Flags: feedback?(karlt) → feedback-
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.
Attached patch omtc.patch (obsolete) — Splinter Review
Consensus was to only call XInitThreads() by default on nightly builds, since it had about a ~5% tpaint regression on 32-bit Ubuntu.
Attachment #785928 - Attachment is obsolete: true
Attachment #785928 - Flags: review?(ncameron)
Attachment #790466 - Flags: review?(ncameron)
Attached patch omtc.patch (obsolete) — Splinter Review
err, with the correct patch
Attachment #790466 - Attachment is obsolete: true
Attachment #790466 - Flags: review?(ncameron)
Attachment #790467 - Flags: review?(ncameron)
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.
Attached patch patchSplinter Review
Yeah I think you're right, the nsChildView.mm changes aren't needed at all.
Attachment #790467 - Attachment is obsolete: true
Attachment #790467 - Flags: review?(ncameron)
Attachment #790939 - Flags: review?(ncameron)
Comment on attachment 790939 [details] [diff] [review]
patch

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

lgtm
Attachment #790939 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/275a38cd4caf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.