Bug 980770 (enable-omt-animations)

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

RESOLVED FIXED in Firefox 41

Status

()

P3
normal
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on: 7 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)

Tracking bug for what we need to make off main thread animations (OMT Animations, OMTA) good enough to ship.
(Note that it's already shipping on B2G, but with some pretty hideous bugs, and poor test coverage.)
Priority: -- → P4
Blocks: 759252
Depends on: 1018862
Depends on: 995591
Depends on: 1045930
Blocks: 1059557
Depends on: 1129245
Depends on: 1122526
Depends on: 1122526
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).
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)
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)
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)
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.
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.
Duplicate of this bug: 788522
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
(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?
Bugs asking for additional features definitely don't block shipping the ones we have.
No longer depends on: 775629, 869129
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: --- → ?
The problem appears to be this one....  https://bugzilla.mozilla.org/show_bug.cgi?id=1122526
status-firefox40: affected → ---
tracking-firefox40: + → ---
relnote-firefox: ? → ---
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
(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

4 years ago
Depends on: 1153539
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.)
Bug 994541 just turned on OMTC on Linux by default, even without e10s. It should ride the trains on 40.
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: --- → ?
Depends on: 1165196
Target Milestone: --- → mozilla40
Added to the release notes with Lawrence's wording.
relnote-firefox: ? → 40+
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
Attachment #8627751 - Flags: review?(bbirtles) → review+
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: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
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
Target Milestone: mozilla40 → mozilla42

Updated

4 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

Updated

4 years ago
Depends on: 1210882
Depends on: 1211487

Updated

4 years ago
Depends on: 1213663

Updated

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

Updated

3 years ago
Depends on: 1255646
Depends on: 1255928

Updated

3 years ago
Depends on: 1252984
Depends on: 1280093

Updated

3 years ago
Depends on: 1284720
Depends on: 1305976
Depends on: 1330627

Updated

2 years ago
Depends on: 1395151
You need to log in before you can comment on or make changes to this bug.