Closed
Bug 1176969
Opened 9 years ago
Closed 9 years ago
part of 3d transform animated chessboard updates only when mouse moves, if OMTA (off-main-thread/async animations) enabled
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox39 | --- | unaffected |
firefox40 | --- | affected |
firefox41 | --- | fixed |
firefox42 | --- | fixed |
People
(Reporter: arni2033, Assigned: dbaron)
References
()
Details
(Keywords: regression)
Attachments
(4 files)
2.76 MB,
video/webm
|
Details | |
641 bytes,
text/html
|
Details | |
741 bytes,
text/html
|
Details | |
7.93 KB,
patch
|
mattwoodrow
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1. Open URL above 2. See the choppy animation Note 1: If layers.offmainthreadcomposition.async-animations = false then animation is fine Note 2: If I start constantly moving mouse, animation becomes almost as smooth as on Release version (that has async-animations = false).
Comment 1•9 years ago
|
||
I can reproduce this.
Blocks: enable-omt-animations
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Updated•9 years ago
|
status-firefox39:
--- → unaffected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
Updated•9 years ago
|
Keywords: regression
Assignee | ||
Updated•9 years ago
|
Summary: Spin animation is choppy if Async-Animations are ON → part of 3d transform animated chessboard updates only when mouse moves, if OMTA (off-main-thread/async animations) enabled
Assignee | ||
Comment 2•9 years ago
|
||
Simple testcase, based on poking around at the demo in inspector.
Assignee | ||
Comment 3•9 years ago
|
||
This is a variant with an OMTA-able animation on the child. In this variant the child never moves at all.
Assignee | ||
Comment 4•9 years ago
|
||
from #layout, just now: [2015-06-29 15:02:31 -0700] <dbaron> mattwoodrow, I'm trying to remember how preserve-3d works, especially as it relates to the layer tree and transforms set on layers [2015-06-29 15:03:11 -0700] <dbaron> mattwoodrow, is it correct that if there are multiple elements in a preserve-3d scene (say, an element with preserve-3d that's the root of the 3d scene and a child of that element) [2015-06-29 15:03:29 -0700] <mattwoodrow> dbaron: nsDisplayTransform accumulates the transforms from the ‘leaf’ element in the scene up to the root [2015-06-29 15:03:31 -0700] <dbaron> mattwoodrow, then if both of those elements have layers, the layers will not be parent and child [2015-06-29 15:03:40 -0700] <mattwoodrow> yes [2015-06-29 15:03:48 -0700] <mattwoodrow> thinker is working on changing that though [2015-06-29 15:04:12 -0700] <dbaron> mattwoodrow, so do you agree that this implies that we can't do OMT animations of transform on either the parent or the child [2015-06-29 15:04:39 -0700] <dbaron> mattwoodrow, since if we do an OMT animation of transform on the parent, we'd fail to update the child during the animation [2015-06-29 15:04:58 -0700] <dbaron> mattwoodrow, and if we do an OMT animation of transform on the child, we'll simply give it the wrong transform because we won't do the accumulation [2015-06-29 15:05:09 -0700] <mattwoodrow> dbaron: yes, exactly [2015-06-29 15:05:19 -0700] <dbaron> mattwoodrow, ok, good [2015-06-29 15:05:34 -0700] <dbaron> mattwoodrow, I think I need to change the && to an ||, then, and that fixes the bug I'm looking at [2015-06-29 15:05:49 -0700] <dbaron> mattwoodrow, hopefully there isn't something in B2G depending on this for perf... [2015-06-29 15:07:02 -0700] <dbaron> (this is 1176969, if you're interested) [2015-06-29 15:07:52 -0700] <mattwoodrow> dbaron: Which bit of code are you looking at? [2015-06-29 15:08:24 -0700] <dbaron> mattwoodrow, in layout/style/AnimationCommon.cpp, AnimationCollection::CanAnimatePropertyOnCompositor [2015-06-29 15:08:45 -0700] <dbaron> currently frame->Preserves3D() && frame->Preserves3DChildren() [2015-06-29 15:10:12 -0700] <mattwoodrow> I agree that that is the right fix :) [2015-06-29 15:10:56 -0700] <dbaron> ok, I'll write a reftest and then upload...
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b16fa5413a0
Assignee | ||
Comment 6•9 years ago
|
||
frame->Preserves3D() is whether the frame's parent has transform-style: preserve-3d, which means that the frame is part of the same 3-D scene as its parent. frame->Preserves3DChildren() is whether the frame itself has transform-style: preserve-3d, which means that the frame is part of the same 3-D scene as its children. Neither of these are valid cases for doing off-main-thread (OMT) animation because all of the layers in a preserve-3d scene are currently siblings of each other, rather than preserving ancestor/descendant relationships. This means that it's not valid to animate transform of the parent on the compositor because the compositor animation won't update any of its children that have layers. Likewise, it's not valid to animate transform of the child on the compositor because the code that sends transform information to the compositor doesn't handle the accumulation of transforms needed to get the "right" transform for the child (i.e., with the transforms of its ancestors up to the top of the 3-D scene merged in). This means that we do OMT animation for slightly fewer cases with the patch than we did without the patch. This means it's pretty low risk in terms of correctness, although there's a chance it might regress performance on one of the (somewhat limited) set of cases where the optimization was valid. (Bug 779598 covers doing OMT animation for preserve-3d cases, and depends on the work ongoing in bug 1097464.) The animate-preserve3d-parent.html reftest doesn't fail without the patch, since something seems to invalidate in the test; it was testing the testcase that showed correct behavior when the mouse was moving, so this isn't incredibly surprising (although that invalidation from mouse movement is itself worth debugging). The animate-preserve3d-child.html test does fail without the patch, though. (With an initial transform of none instead of the 30deg transform, both tests also show an invalidation bug without the patch.) Both tests require a small amount of intermittent fuzz with the patch; this should be investigated.
Attachment #8627443 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
(I'll need to adjust the test to deal with the intermittents on Android -- or maybe just give up. It seems to be a little tricky to write a reliable automated test for this.)
Updated•9 years ago
|
Attachment #8627443 -
Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/ed913a061dd0 https://hg.mozilla.org/mozilla-central/rev/58274d19225e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8627443 [details] [diff] [review] Disable OMT animation for any frame in a preserve-3d scene rather than only frames whose parent and child are in a preserve-3d scene Approval Request Comment [Feature/regressing bug #]: OMT Animations [User impact if declined]: animations that are wildly incorrect [Describe test coverage new/current, TreeHerder]: reftest in patch [Risks and why]: low risk; it disables OMT animation in a case where it's not yet valid to use, by slightly broadening the preserve-3d cases in which it's disabled [String/UUID change made/needed]: no
Attachment #8627443 -
Flags: approval-mozilla-aurora?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a93bd6a0b911 https://hg.mozilla.org/mozilla-central/rev/cb4cdfca64a2
Assignee | ||
Comment 15•9 years ago
|
||
After disabling the reftest last night, I'm going to reenable it based on this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88819f29a560 (I'm fixing the one orange in that run by bumping the fuzz numbers on mac.)
Comment on attachment 8627443 [details] [diff] [review] Disable OMT animation for any frame in a preserve-3d scene rather than only frames whose parent and child are in a preserve-3d scene Approving for uplift to Aurora. I see that we added tests to verify this fix which is great! Also, this patch has been in m-c for a few days now so it seems stable enough to uplift to Aurora.
Attachment #8627443 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/dff3b0967443
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•