Bug 980770 (enable-omt-animations)

get off main thread animations (OMT Animations, OMTA) good enough to ship

RESOLVED FIXED in Firefox 41

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on: 9 bugs, {meta})

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, relnote-firefox 41+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Tracking bug for what we need to make off main thread animations (OMT Animations, OMTA) good enough to ship.
(Assignee)

Updated

3 years ago
Blocks: 788522
Depends on: 980769, 847287, 964646, 788549
(Assignee)

Updated

3 years ago
Depends on: 828173, 978712
(Assignee)

Comment 1

3 years ago
(Note that it's already shipping on B2G, but with some pretty hideous bugs, and poor test coverage.)
(Assignee)

Updated

3 years ago
Depends on: 975261

Updated

3 years ago
Priority: -- → P4
(Assignee)

Updated

3 years ago
Priority: P4 → P3

Updated

3 years ago
Blocks: 759252
Depends on: 1018862
Depends on: 995591
(Assignee)

Updated

3 years ago
Depends on: 1039799
(Assignee)

Updated

3 years ago
Depends on: 969222
Depends on: 1045930
Depends on: 875209
No longer depends on: 1045930
No longer depends on: 995591

Updated

3 years ago
Blocks: 1059557
(Assignee)

Updated

3 years ago
Depends on: 1108690
(Assignee)

Updated

2 years ago
Alias: ship-omta-animations
(Assignee)

Updated

2 years ago
Alias: ship-omta-animations → enable-omt-animations
Depends on: 1129245
Depends on: 1122526
No longer depends on: 1122526
Depends on: 1122526
(Assignee)

Comment 2

2 years ago
Let's see how things look on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a310d156884
(Assignee)

Updated

2 years ago
Depends on: 1090555
(Assignee)

Comment 3

2 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

2 years ago
A push with bug 1090555:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5d5324ba171
(Assignee)

Comment 5

2 years ago
Created attachment 8586449 [details] [diff] [review]
Enable off-main-thread animations on all platforms with off-main-thread compositing
Attachment #8586449 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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

2 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

2 years ago
(Also, it's not clear to me that bug 1122526 is even related to OMT Animations.)
(Assignee)

Comment 9

2 years ago
Created attachment 8586491 [details] [diff] [review]
Enable off-main-thread animatinos on all platforms with off-main-thread compositing, for nightly/aurora
Attachment #8586491 - Flags: review?(bbirtles)
Depends on: 1149865
No longer depends on: 1122526
(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.
Attachment #8586491 - Flags: review?(bbirtles) → review+
Attachment #8586449 - Flags: review?(bbirtles)
(Assignee)

Comment 11

2 years ago
Landed enabling on nightly/aurora:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a765547ac232
Keywords: leave-open
(Assignee)

Comment 12

2 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

2 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

2 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
(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.
(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.
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.
Depends on: 1149906
(In reply to Brian Birtles (:birtles) from comment #17)
> Bug and patch coming up.

That would be bug 1149906.
(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)

Updated

2 years ago
Depends on: 1150288
(Assignee)

Comment 20

2 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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/288ad0365a5e
(Assignee)

Comment 22

2 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

2 years ago
Second followup, to requestLongerTimeout(2):
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f72af9fc50
(Assignee)

Comment 24

2 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
https://hg.mozilla.org/mozilla-central/rev/d2f72af9fc50
(Assignee)

Updated

2 years ago
Depends on: 1150535
(Assignee)

Comment 26

2 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

2 years ago
Backed out again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9e98d2658a
Depends on: 1150619
Duplicate of this bug: 788522
(Assignee)

Comment 29

2 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
https://hg.mozilla.org/mozilla-central/rev/eed5d2d610e2
Depends on: 947753
Depends on: 1105509
Depends on: 1103207
Depends on: 869129
Depends on: 775629

Comment 31

2 years ago
The PanelUI contents appear washed out for a split second when displayed with the pref enabled on Windows 8.1.
(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
Depends on: 1122526
No longer depends on: 1149865
(Assignee)

Comment 33

2 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

2 years ago
Bugs asking for additional features definitely don't block shipping the ones we have.
No longer depends on: 775629, 869129
(Assignee)

Comment 35

2 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
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)]:
status-firefox40: --- → affected
tracking-firefox40: --- → +
relnote-firefox: --- → ?

Comment 37

2 years ago
The problem appears to be this one....  https://bugzilla.mozilla.org/show_bug.cgi?id=1122526
status-firefox40: affected → ---
tracking-firefox40: + → ---
relnote-firefox: ? → ---
(Assignee)

Comment 38

2 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

2 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).

Updated

2 years ago
Depends on: 1153539
(Assignee)

Updated

2 years ago
Depends on: 1153426
(Assignee)

Comment 40

2 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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/76671894fe90
Bug 994541 just turned on OMTC on Linux by default, even without e10s. It should ride the trains on 40.
https://hg.mozilla.org/mozilla-central/rev/76671894fe90

Updated

2 years ago
Depends on: 1156456
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: --- → ?
(Assignee)

Updated

2 years ago
Depends on: 1161049
Depends on: 1165196
Target Milestone: --- → mozilla40
Added to the release notes with Lawrence's wording.
relnote-firefox: ? → 40+
(Assignee)

Comment 46

2 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)
You don't think it is worth mentioning for the aurora release notes?
I can disable it after the merge.
Flags: needinfo?(sledru)
Depends on: 1176969
(Assignee)

Comment 48

2 years ago
Created attachment 8627751 [details] [diff] [review]
Fully enable (for RELEASE_BUILD) off-main-thread animations on all platforms with off-main-thread compositing
Attachment #8627751 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8586449 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → dbaron
Attachment #8627751 - Flags: review?(bbirtles) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 49

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9808ed3cacf
(Assignee)

Comment 50

2 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?
https://hg.mozilla.org/mozilla-central/rev/b9808ed3cacf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Depends on: 1180509
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+

Updated

2 years ago
status-firefox41: --- → affected
Target Milestone: mozilla40 → mozilla42
https://hg.mozilla.org/releases/mozilla-aurora/rev/95ff4ba30e93
status-firefox41: affected → fixed

Updated

2 years ago
Depends on: 1186061
We should probably relnote this for 41 now that it's going to release. Same wording as in comment 44 should be ok.
relnote-firefox: 40+ → ?
Indeed. I did it for 41! Thanks
relnote-firefox: ? → 41+
Target Milestone: mozilla42 → mozilla41
(Assignee)

Comment 56

2 years ago
Blog post about shipping: http://dbaron.org/log/20150916-compositor-animations
(Assignee)

Updated

2 years ago
Depends on: 706179

Updated

2 years ago
Depends on: 1210882
Depends on: 1211487

Updated

2 years ago
Depends on: 1213663

Updated

2 years ago
Depends on: 1204336
Depends on: 1229283
No longer depends on: 1229283
Depends on: 1245075

Updated

a year ago
Depends on: 1255646

Updated

a year ago
Depends on: 1255928

Updated

a year ago
Depends on: 1252984

Updated

11 months ago
Depends on: 1280093

Updated

10 months ago
Depends on: 1284720
Depends on: 1305976
Depends on: 1330627
You need to log in before you can comment on or make changes to this bug.