Closed Bug 947753 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

29 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox39 --- unaffected
firefox40 + verified

People

(Reporter: elad, Assigned: dbaron)

References

Details

(Keywords: perf, Whiteboard: [bugday-20131209][Australis:P-])

Attachments

(4 files, 3 obsolete files)

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.
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
Whiteboard: [bugday-20131209]
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 28 Branch → 29 Branch
Keywords: perf
I'm curious to know if this is true with other arrow panels, like the downloads panel?
Flags: needinfo?(elad)
The downloads panel doesn't seem affected, but for "Display your bookmarks" panel the animation is slower.
Flags: needinfo?(elad)
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.
With non-default pref settings I don't think we can track this for Australis.
Whiteboard: [bugday-20131209] → [bugday-20131209][Australis:P-]
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.
Flags: needinfo?(milan)
Adding Brian; did we do any timings for OMTA?
Flags: needinfo?(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?
Flags: needinfo?(birtles)
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.
Flags: needinfo?(milan)
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.
Flags: needinfo?(elad)
(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.
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
Blocks: 1149865
[Tracking Requested - why for this release]: Regression
Summary: With layers.offmainthreadcomposition.enabled set to true, Australis menu animation is painfully slow → Australis menu animation is painfully slow with off main thread animations (OMTA) enabled
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.)
Today's windows nightly shows this problem, as expected I think.
(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).
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?
Attached patch patch? (obsolete) — Splinter Review
Does this fix it? I can't really tell on Mac because panel animations on Mac are awful anyway (bug 1036929).
(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/ahalachmi@mozilla.com-9f67a2f1e3df

and here, respectively: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ahalachmi@mozilla.com-5a636aaa61d2
(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/
> ahalachmi@mozilla.com-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/
> ahalachmi@mozilla.com-5a636aaa61d2

Good.

But now bug 1122526 is much more obvious. ;)
(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/
> > ahalachmi@mozilla.com-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.
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.
... 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?
... 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 ...)
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).
There's a try build with both patches (and a few others) at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-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 .)
(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/
> dbaron@mozilla.com-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.
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.
Attachment #8593186 - Flags: review?(bbirtles)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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.
Attachment #8590597 - Attachment is obsolete: true
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)?
Attachment #8593186 - Flags: feedback?(mstange)
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 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.
Attachment #8593186 - Flags: feedback?(mstange) → feedback+
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.
Attachment #8593187 - Flags: review?(bbirtles)
Attachment #8593186 - Flags: review?(bbirtles) → review+
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(mstange)
Flags: needinfo?(elad)
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
(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 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.
Attachment #8596196 - Flags: review?(smichaud) → review+
Looks like there's more work to do here: In the try push, test_drawWindow_widget_layers.html fails completely.
Tracking for 40.
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.)
Attachment #8600122 - Flags: review?(bbirtles)
Attachment #8593187 - Attachment is obsolete: true
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.
Attachment #8600151 - Flags: review?(dbaron)
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?
Attachment #8600151 - Flags: review?(dbaron) → review+
Attachment #8600122 - Flags: review?(bbirtles) → review+
Good idea.

I have a greenish try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=db62fdf4a3fe , I think this can land now.
Attachment #8600151 - Attachment is obsolete: true
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.
You need to log in before you can comment on or make changes to this bug.