947753 - Australis menu animation is painfully slow with off main thread animations (OMTA) enabled

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Elad Alfassa]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131208030204

Steps to reproduce:

Set the following values to True in about:config:

layers.offmainthreadcomposition.async-animations
layers.offmainthreadcomposition.enabled
layers.offmainthreadcomposition.testing.enabled

restart nightly
open the Australis menu-thing on the top right


Actual results:

The fade-in animation was painfully slow.


Expected results:

It shouldn't be slow.

Comment 1

[Petruta Horea [:phorea], Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
On 20140108030203 build the fade-in animation is not smooth with the prefs from Comment 0.

I'm changing the Version to 29 since Australis remained in Nightly builds.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm curious to know if this is true with other arrow panels, like the downloads panel?

Comment 3

[Petruta Horea [:phorea], Desktop Test Engineering]

The downloads panel doesn't seem affected, but for "Display your bookmarks" panel the animation is slower.

Comment 4

[Elad Alfassa]

What's interesting that on my machine it's still a bit too slow after I turned the setting off and restarted the browser... Which is odd considering this machine has a powerful GPU (radeon 7700 series)  and a VERY powerful processor (i7-4770) and 16GB of RAM as well.

Comment 5

[Justin Dolske [:Dolske]]

With non-default pref settings I don't think we can track this for Australis.

Comment 6

[Andreas Gal :gal]

This might likely become a problem once we ship with OMTC everywhere. Milan we should understand whats going on here with the right priority/timeline.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Adding Brian; did we do any timings for OMTA?

Comment 8

[Brian Birtles (:birtles)]

(In reply to Milan Sreckovic [:milan] from comment #7)
> Adding Brian; did we do any timings for OMTA?

Timings? I'm working on testing correctness of OMTA for CSS Animations (bug 964646) but there are no performance tests there.

I've noticed that--at least on Windows--sometimes I need to wait a long time before animated values start showing up on the compositor thread. If it's layerization that's causing the delay, does will-change help here?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

We do know about performance regressions with OMTC on Windows in the first place, they show up in the tests; this is why we're not enabling it on by default right now.  I was just wondering if there are any animation specific issues we know about.  We'll track this when we take care of the "other" performance issues and see what the results are like.

Comment 10

[Brian Birtles (:birtles)]

A quick question, does the animation seem more smooth to you if you move the mouse *immediately* after clicking the button?

In bug 975261 I'm fixing a problem where we don't start OMTA animations that have a delay unless there are UI events that flush pending notifications. I have a sneaking suspicion that we can encounter a similar situation even without a delay. It's hard to tell but it *seems* like the animation is smoother if I move the mouse immediately after clicking the menu button.

Comment 11

[Lily Kim]

(In reply to Brian Birtles (:birtles) from comment #10)
> A quick question, does the animation seem more smooth to you if you move the
> mouse *immediately* after clicking the button?

Yes it does. Without moving my mouse the animation is very choppy and slow, but when moving my mouse, the animation becomes silky smooth.

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

layers.offmainthreadcomposition.async-animations is not default set to true
Is this issue still happens when you use default settings and can you still reproduce the issue with clean new fresh profile without any addons and plugins?
Post your Graphic section from about:support will also helps

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I wonder if this is related related to the callers of nsLayoutUtils::GetFrameTransparency not getting rerun as opacity changes.  Though that should only affect whether the window has opacity or not; I don't see any place we transfer an actual opacity number to the widget.

(Changing nsLayoutUtils::GetFrameTransparency to also consider opacity animations, perhaps by using nsIFrame::HasOpacity or HasVisualOpacity, would be an interesting experiment, though.)

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Today's windows nightly shows this problem, as expected I think.

Comment 17

[Avi Halachmi (:avih)]

(In reply to Virtual_ManPL [:Virtual] from comment #12)
> layers.offmainthreadcomposition.async-animations is not default set to true ...

It is set to true by default from today I think, and yes, the issue is now on nightlies and very noticeable. From close enough to 60fps animation, it now animates at what seems like 5-10 fps (so maybe 2-3 frames for the entire animation).

Comment 18

[Markus Stange [:mstange]]

The popup uses a retaining layer manager but doesn't use OMTC. When we are enabling OMTA for an element, are we only checking for a layer, but not whether that layer's layer manager is a ClientLayerManager?

Comment 19

[Markus Stange [:mstange]]

Attachment: patch?

Does this fix it? I can't really tell on Mac because panel animations on Mac are awful anyway (bug 1036929).

Comment 20

[Avi Halachmi (:avih)]

(In reply to Markus Stange [:mstange] from comment #19)
> Created attachment 8590597 [details] [diff] [review]
> patch?
> 
> Does this fix it? I can't really tell on Mac because panel animations on Mac
> are awful anyway (bug 1036929).

I submitted try pushes for people to test:

- current m-c as base: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f67a2f1e3df

- same as above with the patch from comment 19 applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a636aaa61d2

The builds should be available for testing in the next few hours, here:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9f67a2f1e3df

and here, respectively: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5a636aaa61d2

Comment 21

[Elbart]

(In reply to Avi Halachmi (:avih) from comment #20)

Win7 32bit:

> - current m-c as base:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f67a2f1e3df
> 
> The builds should be available for testing in the next few hours, here:
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-9f67a2f1e3df

Not good.

> - same as above with the patch from comment 19 applied:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a636aaa61d2
> 
> and here, respectively:
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-5a636aaa61d2

Good.

But now bug 1122526 is much more obvious. ;)

Comment 22

[Avi Halachmi (:avih)]

(In reply to Elbart from comment #21)
> > - same as above with the patch from comment 19 applied:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a636aaa61d2
> > 
> > and here, respectively:
> > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> > [e-mail]-5a636aaa61d2
> 
> Good.

Confirmed.


> But now bug 1122526 is much more obvious. ;)

TBH I don't think it's terrible, and despite my general attention to visual details, I'm not sure I can notice it during actual animation. That's not to say that it's not a bug though.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, what the patch in comment 19 does is make us stop suppressing the ticking of the animation on the main thread while it's running on the compositor.  It doesn't actually make us stop trying to run it on the compositor.

It seems like we probably want a test in CommonAnimationManager::GetAnimationsForCompositor or something that it calls -- although I think that code is also used when we're deciding whether to force creation of a layer, which means it doesn't necessarily have a layer object at hand.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and by something that it calls, I mean AnimationPlayerCollection::CanPerformOnCompositorThread, which may make more sense.

Would testing frame->GetNearestWidget()->GetLayerManager() there be the right layer manager to check?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... although given the complexity of the various nsIWidget::GetLayerManager implementations it's not clear to me if that's 100% accurate, in which case it would be worth keeping both checks.

(Though the most complicated implementation is the metro one, which ...)

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I've written that additional patch:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/8c1079e3cdd8/omta-disable-layers-backend
and I'll push a try run once I've tested it locally (on Linux, where the bug isn't visible).

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's a try build with both patches (and a few others) at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d6567e7dcb4/try-win32/
if somebody who runs Windows wants to try it and let me know whether it fixes both this and bug 1122526.  (There's both an installer and a .zip .)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d6567e7dcb4 was the try run

Comment 29

[Elbart]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #27)
> There's a try build with both patches (and a few others) at:
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-0d6567e7dcb4/try-win32/
> if somebody who runs Windows wants to try it and let me know whether it
> fixes both this and bug 1122526.  (There's both an installer and a .zip .)

Pixelation and slowness looks fixed to me.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: / Bug 1122526 - Don't claim to support off-main-thread animations when the nearest widget is not using OMT compositing

This moves the test for whether off-main-thread compositor is enabled
*earlier* in CanPerformOnCompositorThread, since
CanAnimatePropertyOnCompositor is called only from
CanPerformOnCompositorThread.  This change means we're using a more
accurate test for whether we actually have off-main-thread compositing
than the pref, since in some cases we won't use off-main-thread
compositing for certain widgets (e.g., transparent widgets on Windows)
even when the pref is enabled.

I'm not convinced that this test is 100% accurate (i.e., in terms of
predicting the layer manager that will be used for a frame for this
layer), so I think it's also worth landing the patch that doesn't
throttle the main-thread execution of the animation when the layer to
which we'd send the animations isn't in an layer manager that's doing
off-main-thread compositing.  I'm open to being convinced of that,
though.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Don't throttle main-thread execution of animations when the layer is in a non-OMTC layer tree

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8590597 [details] [diff] [review]
patch?

Attachment 8593187 [details] [diff] is the same as this patch, with just the commit message revised to reflect more accurately what it does.

(Thanks to Markus for spotting the underlying problem here.)

I should probably file a followup graphics bug on the pixelation that happens when we create the extra layer here.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8593186 [details] [diff] [review]
/ Bug 1122526 - Don't claim to support off-main-thread animations when the nearest widget is not using OMT compositing

Markus -- I'm curious if you think the test here (getting the nearest widget and getting its layer manager) is accurate enough that there's no point actually testing whether the layer that ends up created actually matches that result (in the next patch)?

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And to clarify the difference between the tests:

 * the test in attachment 8593186 [details] [diff] [review] influences whether we force the creation of a layer because we want to send animations to it, and whether we send the animations to the layer

 * the test in attachment 8590597 [details] [diff] [review] / attachment 8593187 [details] [diff] [review] influences whether, given that we have animations on a layer, we skip running style updates on the main thread

Comment 35

[Markus Stange [:mstange]]

Comment on attachment 8593186 [details] [diff] [review]
/ Bug 1122526 - Don't claim to support off-main-thread animations when the nearest widget is not using OMT compositing

Sorry I didn't respond to the earlier comments, it looks like I hadn't CC'd myself.

This looks good. Per window, there is only one layer manager that will retain layers, and that's the widget's layer manager. I think the layer manager of a widget can change during the widget's lifetime, but it will never change between OMTC and non-OMTC.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8593187 [details] [diff] [review]
Don't throttle main-thread execution of animations when the layer is in a non-OMTC layer tree

OK,thanks.  Cancelling review on the second (original) patch, then.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60b7888a323b

Comment 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a51b749299f

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a51b749299f


I'm guessing this failure is because this patch is making us call GetLayerManager earlier than before, given that I can't see any other explanation for that test failing as a result of the patch.

I'll probably need some help looking into that failure.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There were also some browser-chrome failures on Mac.

Markus, any ideas what's going on here?

I'm also confused by nsCocoaWindow::GetLayerManager returning null most of the time -- and why that wouldn't have caused crashes here.

Comment 41

[Markus Stange [:mstange]]

I'll look into it more tomorrow, but I think I can already answer the nsCocoaWindow question: On OS X, the main window has two widgets: the "outer" one (which is nsCocoaWindow) that gets created by nsXULWindow, and the "inner" one (nsChildView) which is the widget for the window's view manager. The layer manager is managed by the nsChildView, and the nsChildView is also what you'd get as an nsIFrame's nearest widget.

Comment 42

[Markus Stange [:mstange]]

Comment 41 is only true for top level windows. For popup windows, the nsIWidget is the popup's nsCocoaWindow which manages its own nsChildView internally in mPopupContentView.

I didn't get a chance to look into the OS X failures today. For my own reference, this was the failing push:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=60b7888a323b

And the failures are:
 - test_acceleration.html (as part of mochitest-4)
 - "invalid context 0x0" CGContext error messages in the 10.10 debug bc1 run
 - lots of timeouts in the 10.10 opt bc2 run

Comment 43

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #39)
> I'm guessing this failure is because this patch is making us call
> GetLayerManager earlier than before

This guess was correct. We call it earlier and end up without acceleration because nsChildView::ComputeShouldAccelerate returns false at that time.

When nsCocoaWindow::CreateNativeWindow creates the native NSWindow, it makes that NSWindow start out transparent (setOpaque:NO). After the first reflow of the window, nsContainerFrame::SyncWindowProperties lets the window know whether it's actually transparent - in almost all cases it's not. So from that point on the NSWindow is opaque. And then at some point later we call nsBaseWidget::GetLayerManager on the child widget for the first time, which calls into nsChildView::ComputeShouldAccelerate, which checks the NSWindow's opaqueness, and usually returns true because it's opaque.

Now with this patch, we call GetLayerManager before nsContainerFrame::SyncWindowProperties runs, so the NSWindow is not marked as opaque, so we don't accelerate.

This is kind of broken anyway - the opaqueness of a window can change, but we never detect that and recreate the appropriate layer manager. So I think we should just make all non-popup windows start out opaque and accelerate them.
Here's a try push with that change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35a5e8edd7ad

B2G desktop has run into a very similar problem before in bug 923961.

Comment 44

[Markus Stange [:mstange]]

Attachment: always accelerate non-popup windows

Comment 45

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8596196 [details] [diff] [review]
always accelerate non-popup windows

This looks fine to me.

This may result in visible glitches as windows are created.  But those glitches will be bugs in their own right, which we've only papered over until now.

Comment 46

[Markus Stange [:mstange]]

Looks like there's more work to do here: In the try push, test_drawWindow_widget_layers.html fails completely.

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 40.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Don't throttle main-thread execution of animations when the layer is in a non-OMTC layer tree

Given the difficulty (related to creating the layer manager to early) of
landing the patch to avoid sending animations to non-OMTC layers, it
seems worth landing this.

(The patch is by mstange; the comments by dbaron.)

Comment 49

[Markus Stange [:mstange]]

I've found the issue with test_drawWindow_widget_layers.html a few minutes ago. This test opens a window that it drawWindows into the testing canvas, but the drawWindow calls start before the window has actually been shown. As a result, we used to get a BasicLayerManager for it and the tests passed, but with my patch we accelerate it and try to use OMTC, but since the window isn't visible at that point, the compositor refuses to paint. (This happens due to a check in -[ChildView preRender:].)
I'm going to change the test to wait for the window to actually be visible, and then I think we can land the other patch.

Comment 50

[Markus Stange [:mstange]]

Attachment: fix test_drawWindow_widget_layers.html

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8600151 [details] [diff] [review]
fix test_drawWindow_widget_layers.html

Maybe move the CANVAS_WIDTH and CANVAS_HEIGHT out into the global scope of file_drawWindow_common.js instead of repeating them here?

Comment 52

[Markus Stange [:mstange]]

Attachment: fix test_drawWindow_widget_layers.html. r=dbaron

Good idea.

I have a greenish try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=db62fdf4a3fe , I think this can land now.

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9321d83d7d8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d27812986528
https://hg.mozilla.org/integration/mozilla-inbound/rev/962e15b81684

Comment 54

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9321d83d7d8f
https://hg.mozilla.org/mozilla-central/rev/d27812986528
https://hg.mozilla.org/mozilla-central/rev/962e15b81684

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just landed bug 1161049 on mozilla-inbound, which changes how this bug is fixed.  Once it hits mozilla-central and then hits nightly, it might be worth retesting this to confirm.