Closed
Bug 980770
(enable-omt-animations)
Opened 11 years ago
Closed 9 years ago
get off main thread animations (OMT Animations, OMTA) good enough to ship
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: dbaron, Assigned: dbaron)
References
(Depends on 7 open bugs)
Details
(Keywords: meta)
Attachments
(2 files, 1 obsolete file)
1.28 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
birtles
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Tracking bug for what we need to make off main thread animations (OMT Animations, OMTA) good enough to ship.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
(Note that it's already shipping on B2G, but with some pretty hideous bugs, and poor test coverage.)
Updated•11 years ago
|
Priority: -- → P4
Assignee | ||
Updated•11 years ago
|
Priority: P4 → P3
Assignee | ||
Updated•10 years ago
|
Alias: ship-omta-animations
Assignee | ||
Updated•10 years ago
|
Alias: ship-omta-animations → enable-omt-animations
Assignee | ||
Comment 2•10 years ago
|
||
Let's see how things look on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a310d156884
Assignee | ||
Comment 3•10 years ago
|
||
The try run isn't quite done yet, but the main thing it turned up is already bug 1090555 (test was previously disabled on Mulet; also now shows up on Android mochitest and on Linux mochitest-e10s).
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8586449 -
Flags: review?(bbirtles)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
Comment on attachment 8586449 [details] [diff] [review]
Enable off-main-thread animations on all platforms with off-main-thread compositing
I thought we were only turning this on for nightly/aurora until blockers like bug 1122526 were resolved?
Flags: needinfo?(dbaron)
Assignee | ||
Comment 7•10 years ago
|
||
I guess we could do nightly/aurora first, although I'd hope to be able to ship to beta/release this cycle.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 8•10 years ago
|
||
(Also, it's not clear to me that bug 1122526 is even related to OMT Animations.)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8586491 -
Flags: review?(bbirtles)
Comment 10•10 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #8)
> (Also, it's not clear to me that bug 1122526 is even related to OMT
> Animations.)
Sorry, that should be bug 1149865 which should definitely block shipping OMTA.
Updated•10 years ago
|
Attachment #8586491 -
Flags: review?(bbirtles) → review+
Updated•10 years ago
|
Attachment #8586449 -
Flags: review?(bbirtles)
Assignee | ||
Comment 11•10 years ago
|
||
Landed enabling on nightly/aurora:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a765547ac232
Keywords: leave-open
Assignee | ||
Comment 12•10 years ago
|
||
Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1083d2b2217
since there were some test failures that snuck in since my successful try run a few days ago:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6622f9ac9faf
Assignee | ||
Comment 13•10 years ago
|
||
(That successful try run was on top of https://hg.mozilla.org/mozilla-central/rev/e9769c80ee59 plus my patches to bug 847287.)
Assignee | ||
Comment 14•10 years ago
|
||
The relevant changes that are likely to have introduced the test failures are:
bug 1109390 (patches 16 to 18 or patches 20 to 28)
bug 1074630
bug 1145246
Comment 15•10 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #14)
> The relevant changes that are likely to have introduced the test failures
> are:
> bug 1109390 (patches 16 to 18 or patches 20 to 28)
> bug 1074630
> bug 1145246
Debugging locally I can reproduce the failure in test_deferred_start.html.
The first changeset that fails for me is:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded0fc853a7b
i.e. part 22 from bug 1109390
which is odd because that patch shouldn't really do anything (i.e. we shouldn't ever actually end up calling PauseAt without the later patches applied). Perhaps this only intermittently fails and I got lucky when testing the changeset below it.
Comment 16•10 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #15)
> Perhaps this only intermittently fails and I got lucky when testing the changeset below it.
This appears to be the case. I still see the failure with patches 20 to 28 of bug 1109390 backed out.
Comment 17•10 years ago
|
||
I've narrowed down the failure in test_deferred_start.html to this changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4306ea579d9
i.e. part 18 of bug 1109390
The problem appears to be that we no longer treat play-pending animations as eligible compositor animations. This is a pretty significant bug so it's good we found it now. Bug and patch coming up.
Comment 18•10 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #17)
> Bug and patch coming up.
That would be bug 1149906.
Comment 19•10 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #17)
> > Bug and patch coming up.
>
> That would be bug 1149906.
That bug appears to fix the failures in test_deferred_start.html but not the failures in test_animations_omta.html. I'll hopefully be able to look into that tomorrow.
Assignee | ||
Comment 20•10 years ago
|
||
Two new try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a1042706d0 with bug 1150288 and bug 1149906
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbcfd0e9880b with only bug 1150288
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Landed a followup to disable one test under some conditions after discussion with birtles:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea87def95b9
I filed bug 1150351 on reenabling the test.
Assignee | ||
Comment 23•10 years ago
|
||
Second followup, to requestLongerTimeout(2):
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f72af9fc50
Assignee | ||
Comment 24•10 years ago
|
||
Backed out in
https://hg.mozilla.org/integration/mozilla-inbound/rev/bce1203ac470
for high-frequency intermittent Mac talos glterrain crashes:
https://treeherder.mozilla.org/logviewer.html#?job_id=8386168&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=8386163&repo=mozilla-inbound
Assignee: dbaron → nobody
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
I relanded the enabling on platforms other than Mac:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9002c68ad577
but left it disabled on Mac, given bug 1150535.
Assignee | ||
Comment 27•10 years ago
|
||
Backed out again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9e98d2658a
Depends on: 1150619
Assignee | ||
Comment 29•10 years ago
|
||
Given that the Mac problem went away (see bug 1150535 comment 1), I relanded for all platforms other than Linux:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed5d2d610e2
Comment 30•10 years ago
|
||
Comment 31•10 years ago
|
||
The PanelUI contents appear washed out for a split second when displayed with the pref enabled on Windows 8.1.
Comment 32•10 years ago
|
||
(In reply to Gary [:streetwolf] from comment #31)
> The PanelUI contents appear washed out for a split second when displayed
> with the pref enabled on Windows 8.1.
Seeing the same on Win7 x64 m-c builds (tinderbox) win32
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Gary [:streetwolf] from comment #31)
> The PanelUI contents appear washed out for a split second when displayed
> with the pref enabled on Windows 8.1.
Could you file a separate bug on that -- and one that describes steps to reproduce for people who don't know what "PanelUI contents" are?
Assignee | ||
Comment 34•10 years ago
|
||
Bugs asking for additional features definitely don't block shipping the ones we have.
Assignee | ||
Comment 35•10 years ago
|
||
Bug 1105509 is definitely worth fixing, but it's no worse with OMT animations enabled than it was without it, so it definitely doesn't block shipping OMT animations.
No longer depends on: 1105509
Comment 36•10 years ago
|
||
I'm tracking 40 so that we can keep an eye on this work and whether it is ready to ship in 40.
Release Note Request (optional, but appreciated)
[Why is this notable]: Performance improvement for opacity and transform animations.
[Suggested wording]: Better performance of opacity and transform animations
[Links (documentation, blog post, etc)]:
Comment 37•10 years ago
|
||
The problem appears to be this one.... https://bugzilla.mozilla.org/show_bug.cgi?id=1122526
Assignee | ||
Comment 38•10 years ago
|
||
I don't think bug 1103207 blocks shipping OMTA on desktop either. It's something we've lived with on Firefox OS, and OMTA doesn't change the fact that we're doing stuff at 60Hz when there's an animation that requires less frequency. (We've never had lower-rate ticking of the refresh driver just like we don't have lower-rate ticking of the compositor. OMT Animations moves that high-frequency work to different places; more on the compositor thread and less on the main thread.) But I don't think it's a regression that requires us to fix before shipping.
No longer depends on: 1103207
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #36)
> I'm tracking 40 so that we can keep an eye on this work and whether it is
> ready to ship in 40.
So far we've only enabled ifndef RELEASE_BUILD, i.e., for nightly and aurora, so I think this probably isn't needed (yet).
Assignee | ||
Comment 40•10 years ago
|
||
I'm planning to enable on Linux as well now that bug 1150619 has gone away.
(I believe turning it on on Linux will affect only nightly and not other channels, because off-main-thread compositing (OMTC) is enabled on Linux only when Electrolysis (e10s) is enabled, which is only on nightly.)
Comment 41•10 years ago
|
||
Comment 42•10 years ago
|
||
Bug 994541 just turned on OMTC on Linux by default, even without e10s. It should ride the trains on 40.
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: OMTA provide smoother animations
[Suggested wording]: Smoother and more reliable CSS animations via asynchronous animations
[Links (documentation, blog post, etc)]:
https://wiki.mozilla.org/Platform/GFX/OffMainThreadCompositing#CSS_Animations
Happy to have input on better wording for the note. ;)
relnote-firefox:
--- → ?
Updated•10 years ago
|
Target Milestone: --- → mozilla40
Assignee | ||
Comment 46•10 years ago
|
||
The release notes for what? It's currently enabled only for nightly/aurora (i.e., in an #else of #ifdef RELEASE_BUILD).
Flags: needinfo?(sledru)
Comment 47•10 years ago
|
||
You don't think it is worth mentioning for the aurora release notes?
I can disable it after the merge.
Flags: needinfo?(sledru)
Assignee | ||
Comment 48•9 years ago
|
||
Attachment #8627751 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8586449 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
Updated•9 years ago
|
Attachment #8627751 -
Flags: review?(bbirtles) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 49•9 years ago
|
||
Assignee | ||
Comment 50•9 years ago
|
||
Comment on attachment 8627751 [details] [diff] [review]
Fully enable (for RELEASE_BUILD) off-main-thread animations on all platforms with off-main-thread compositing
Approval Request Comment
[Feature/regressing bug #]: OMT animations
[User impact if declined]: better performance of animations of opacity and transform
[Describe test coverage new/current, TreeHerder]: substantial mochitest and reftest coverage
[Risks and why]: it's a somewhat large feature, but I think it's now ready to ride the trains. All this patch does is change it from enabled on nightly and aurora only to being enabled on all channels, since I believe that if bug 1122526 and bug 1176969 are approved, it's in good enough shape to ride the trains for this release.
[String/UUID change made/needed]: no
Attachment #8627751 -
Flags: approval-mozilla-aurora?
Comment 51•9 years ago
|
||
Comment on attachment 8627751 [details] [diff] [review]
Fully enable (for RELEASE_BUILD) off-main-thread animations on all platforms with off-main-thread compositing
Approving for uplift to Aurora. The patch has been in m-c for a few days and hasn't had a negative impact so far.
Attachment #8627751 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox41:
--- → affected
Updated•9 years ago
|
Target Milestone: mozilla40 → mozilla42
Comment 53•9 years ago
|
||
Comment 54•9 years ago
|
||
We should probably relnote this for 41 now that it's going to release. Same wording as in comment 44 should be ok.
Updated•9 years ago
|
Target Milestone: mozilla42 → mozilla41
Assignee | ||
Comment 56•9 years ago
|
||
Blog post about shipping: http://dbaron.org/log/20150916-compositor-animations
You need to log in
before you can comment on or make changes to this bug.
Description
•