Closed
Bug 1218620
Opened 9 years ago
Closed 9 years ago
When transform animations cannot run on the compositor, opacity animations on the same element are also forced to run on the main thread
Categories
(Core :: DOM: Animation, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
| Tracking | Status | |
|---|---|---|
| firefox48 | --- | fixed |
People
(Reporter: hiro, Unassigned)
References
Details
Attachments
(4 files, 2 obsolete files)
While I am working on bug 1216030, I noticed that there are some cases that opacity and transform animations are not running on compositor at all.
For example:
<!DOCTYPE html>
<html lang="en">
<meta charset="utf-8">
<style>
#target {
width: 100px; height: 100px;
animation: rotate-opacity linear 1s infinite;
backface-visibility: hidden;
background-color: red;
}
@keyframes rotate-opacity {
from {
opacity: 1;
transform: rotate(0deg);
}
to {
opacity: 0;
transform: rotate(360deg);
}
}
</style>
<div id="target"></div>
In above case, the transform animation can not be run on compositor because of 779598. On the other hand, the opacity animation can be run on compositor, but actually it isn't.
That's because AnimationCollection::CanAnimatePropertyOnCompositor returns false in the above case. I think we should handle this case. Actually we can handle this case in bug 1216030, but I would like to handle this as a separate bug.
|
Reporter | |
Comment 1•9 years ago
|
||
This test depends on patched for bug 1196114.
|
|
Reporter | |
Comment 2•9 years ago
|
||
Attachment #8718704 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 3•9 years ago
|
||
The type name has been changed and re-ordered.
HasAnimationOfProperty in FindAnimationsForCompositor has been moved
before ShouldBlockTransformAnimations because calling
ShouldBlockTransformAnimations against effects which have no transform property
is less efficient.
Review commit: https://reviewboard.mozilla.org/r/38387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38387/
Attachment #8727202 -
Flags: review?(bbirtles)
|
||
Comment 4•9 years ago
|
||
Comment on attachment 8727202 [details]
MozReview Request: Bug 1218620 - Allow opacity animation running on compositor even if the frame has any restricted transforms. r?birtles
https://reviewboard.mozilla.org/r/38387/#review34959
I think this is probably fine (with comments addressed), but there's a chance this will actually make some animations look worse.
It would be interesting to create an animation that benefits from this (e.g. animations left/top and opacity), then introduce some artificial jank on the main thread and see if the animation actually looks better or not. We've already had other bug reports from people complaining about different animation components not lining us due to OMTA.
::: dom/animation/AnimationPerformanceWarning.h:24
(Diff revision 1)
> + TransformWithGeometricProperties,
> TransformFrameInactive,
Were we trying to keep this in alphabetical order? Because it's not quite in order before or after this patch.
(If we do change this, we need to keep the switch in AnimationPerformanceWarning.cpp in sync too.)
::: dom/animation/EffectCompositor.cpp:124
(Diff revision 1)
> - aProperty, AnimationPerformanceWarning(warningType));
> + eCSSProperty_transform, AnimationPerformanceWarning(warningType));
Nit: I'm not sure if this change is necessary.
::: dom/animation/KeyframeEffect.h:321
(Diff revision 1)
> - // on the same element.
> + bool ShouldBlockTransformAnimations(
ShouldBlockAsyncTransformAnimations?
::: dom/animation/KeyframeEffect.cpp:2168
(Diff revision 1)
> - return false;
> + MOZ_ASSERT(hasTransform, "Should have transform property");
This doesn't seem right to me. Isn't it possible we could have two animations running on the same element:
* One animates top/left
* One animates transform
We **should** call this method on the first animation's KeyframeEffect even though it doesn't animate the transform property, right?
We probably need a test for that too. I think our current tests only deal with a single animation?
::: dom/animation/KeyframeEffect.cpp:2171
(Diff revision 1)
> + return !CanAnimateTransformOnCompositor(aFrame, aPerformanceWarning);
As with the previous comment, I'm not sure that this is correct.
::: dom/animation/test/chrome/test_animation_property_state.html:186
(Diff revision 1)
> + desc: 'opacity and transform on compositor with animtion of geometric ' +
animation
Attachment #8727202 -
Flags: review?(bbirtles)
|
|
Reporter | |
Comment 5•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #4)
> Comment on attachment 8727202 [details]
> MozReview Request: Bug 1218620 - Allow opacity animation running on
> compositor even if the frame has any restricted transforms. r?birtles
>
> https://reviewboard.mozilla.org/r/38387/#review34959
>
> I think this is probably fine (with comments addressed), but there's a
> chance this will actually make some animations look worse.
>
> It would be interesting to create an animation that benefits from this (e.g.
> animations left/top and opacity), then introduce some artificial jank on the
> main thread and see if the animation actually looks better or not. We've
> already had other bug reports from people complaining about different
> animation components not lining us due to OMTA.
Exactly. I will check it. Please let me know if you have any examples.
> ::: dom/animation/AnimationPerformanceWarning.h:24
> (Diff revision 1)
> > + TransformWithGeometricProperties,
> > TransformFrameInactive,
>
> Were we trying to keep this in alphabetical order? Because it's not quite in
> order before or after this patch.
Actually it's not alphabetical order, but I wanted to sort depends on its characteristic.
> (If we do change this, we need to keep the switch in
> AnimationPerformanceWarning.cpp in sync too.)
Sure.
> ::: dom/animation/EffectCompositor.cpp:124
> (Diff revision 1)
> > - aProperty, AnimationPerformanceWarning(warningType));
> > + eCSSProperty_transform, AnimationPerformanceWarning(warningType));
>
> Nit: I'm not sure if this change is necessary.
>
> ::: dom/animation/KeyframeEffect.h:321
> (Diff revision 1)
> > - // on the same element.
> > + bool ShouldBlockTransformAnimations(
>
> ShouldBlockAsyncTransformAnimations?
>
> ::: dom/animation/KeyframeEffect.cpp:2168
> (Diff revision 1)
> > - return false;
> > + MOZ_ASSERT(hasTransform, "Should have transform property");
>
> This doesn't seem right to me. Isn't it possible we could have two
> animations running on the same element:
>
> * One animates top/left
> * One animates transform
>
> We **should** call this method on the first animation's KeyframeEffect even
> though it doesn't animate the transform property, right?
Ah, right! I did totally forget multiple animations in the same element. We should write those test cases.
|
|
Reporter | |
Comment 6•9 years ago
|
||
A window on left side is without the patch, The right one is with the patch.
IMHO, both are *bad*. The left one does not change the animation box at all until the jank frame has gone. The opacity animation in the right one is gradually changed but totally *out-of-sync* with the box position.
That being said, as you already know, an advantage of the patch is that it decrease the possibility of janky frames.
Attachment #8721529 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 7•9 years ago
|
||
|
|
||
Comment 8•9 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Created attachment 8727681 [details]
> An typical extreme example with artificial jank frame
>
> A window on left side is without the patch, The right one is with the patch.
>
> IMHO, both are *bad*. The left one does not change the animation box at all
> until the jank frame has gone. The opacity animation in the right one is
> gradually changed but totally *out-of-sync* with the box position.
Thanks for making this. Having seen this, I'm starting to think it's better that the animation of different properties stays in sync?
I think it's easier for an author to understand, "this whole animation is janky" rather than, "it's partially janky"?
Also, I think you most often notice jank when the position of something is changing, less so when color/opacity is changing. So, if we had the opposite case where sometimes opacity can't animated on the compositor, I think we'd still want to run the transform component on the compositor. However, in this case, I'm not sure.
What do you think?
|
|
Reporter | |
Comment 9•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > Created attachment 8727681 [details]
> > An typical extreme example with artificial jank frame
> >
> > A window on left side is without the patch, The right one is with the patch.
> >
> > IMHO, both are *bad*. The left one does not change the animation box at all
> > until the jank frame has gone. The opacity animation in the right one is
> > gradually changed but totally *out-of-sync* with the box position.
>
> Thanks for making this. Having seen this, I'm starting to think it's better
> that the animation of different properties stays in sync?
Yes, I also think the synchronized animations look better actually. Last night I wondered if we could handle multiple animations on compositor or not depends on busyness of the main-thread. E.g,
* when janky frames happen (i.e. main-thread is busy)
* both of opacity and geometric animations run on the main-thread
* while no janky frames
* opacity animation runs on compositor, geometric animation runs on the main-thread
This is something wrong. That's because we have to run the opacity animation on the main-thread because of busyness of the main-thread!
> I think it's easier for an author to understand, "this whole animation is
> janky" rather than, "it's partially janky"?
>
> Also, I think you most often notice jank when the position of something is
> changing, less so when color/opacity is changing. So, if we had the opposite
> case where sometimes opacity can't animated on the compositor, I think we'd
> still want to run the transform component on the compositor.
Yeah, if we met that case, we should run the transform on the compositor.
> What do you think?
It is difficult to decide what we should do actually. But now I have a mitigative solution, it's actually aside from this bug, in case of left/top/right/bottom animations with opacity animation.
Now we can inform the performance warning messages to users (web developers) through devtools (not yet actually), so if there are animations which has opacity and left/top/right/bottom properties on the same element, we can report to users something like this:
'left' property of this animation can be replaced by 'transform' animation, as a result, 'opacity' or 'transform' animation on the same element could run on compositor!
By the way, chrome does run opacity animation anyway, I did check it now.
|
|
Reporter | |
Comment 10•9 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> By the way, chrome does run opacity animation anyway
run opacity animations *on compositor*
|
|
||
Comment 11•9 years ago
|
||
I just checked, and so does Edge. (I'm trying to test on Safari by dynamis' Safari is stuck importing Firefox bookmarks.)
Given that, I think we should be consistent with Chrome and Edge and land this. In future we may introduce an additional CSS property to force animations to be synchronized--I'm pretty sure we've talked about that in other bugs.
|
|
||
Updated•9 years ago
|
Summary: Opacity animation with transform animation which is disabled to running on compositor because of Gecko's bug should be running on compositor → When transform animations cannot run on the compositor, opacity animations on the same element are also forced to run on the main thread
|
|
||
Comment 12•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #11)
> I just checked, and so does Edge. (I'm trying to test on Safari by dynamis'
> Safari is stuck importing Firefox bookmarks.)
Safari does the same as Chrome and Edge.
|
|
Reporter | |
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/38387/#review34959
> Nit: I'm not sure if this change is necessary.
It's definetely needed, I think.
A problem in the previous patch was lying right after this code:
if (aMatches) {
aMatches->Clear();
}
effect->SetPerformanceWarning(...
This should be EffectCompositor::SetPerformanceWarning(...
Otherwise, as you pointed out, the warning message is not set against appropriate frame.
E.g,
div.animate({ transform: ..}, 100;
div.aniamte({ width: [ '100px', '200px' ], opacity: [0, 1] }, 100);
In above case, ShouldBlockAsyncTransformAnimations (this is called against the second animation's effect) surely returns true, but tried to set the warning to the second animation's effect in the previous patch because unfortunately the effect had no 'transform' property.
Adding test cases cover this case.
> This doesn't seem right to me. Isn't it possible we could have two animations running on the same element:
>
> * One animates top/left
> * One animates transform
>
> We **should** call this method on the first animation's KeyframeEffect even though it doesn't animate the transform property, right?
>
> We probably need a test for that too. I think our current tests only deal with a single animation?
One thing is not clear to me is that we should set a performance warning to the first animation? If we do not set the warning to the first animation(top/left), we don't need to call ShouldBlockAsyncTransformAnimations at all.
|
|
Reporter | |
Updated•9 years ago
|
Attachment #8727202 -
Flags: review?(bbirtles)
|
|
Reporter | |
Comment 14•9 years ago
|
||
Comment on attachment 8727202 [details]
MozReview Request: Bug 1218620 - Allow opacity animation running on compositor even if the frame has any restricted transforms. r?birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38387/diff/1-2/
|
|
||
Comment 15•9 years ago
|
||
https://reviewboard.mozilla.org/r/38387/#review34959
> It's definetely needed, I think.
>
> A problem in the previous patch was lying right after this code:
>
> if (aMatches) {
> aMatches->Clear();
> }
> effect->SetPerformanceWarning(...
>
> This should be EffectCompositor::SetPerformanceWarning(...
> Otherwise, as you pointed out, the warning message is not set against appropriate frame.
> E.g,
>
> div.animate({ transform: ..}, 100;
> div.aniamte({ width: [ '100px', '200px' ], opacity: [0, 1] }, 100);
>
> In above case, ShouldBlockAsyncTransformAnimations (this is called against the second animation's effect) surely returns true, but tried to set the warning to the second animation's effect in the previous patch because unfortunately the effect had no 'transform' property.
> Adding test cases cover this case.
Sorry, the comment was about whether we need to replace aProperty with eCSSProperty_transform. Since we know aProperty == eCSSProperty_transform, why make the change?
|
|
Reporter | |
Comment 16•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #15)
> https://reviewboard.mozilla.org/r/38387/#review34959
>
> > It's definetely needed, I think.
> >
> > A problem in the previous patch was lying right after this code:
> >
> > if (aMatches) {
> > aMatches->Clear();
> > }
> > effect->SetPerformanceWarning(...
> >
> > This should be EffectCompositor::SetPerformanceWarning(...
> > Otherwise, as you pointed out, the warning message is not set against appropriate frame.
> > E.g,
> >
> > div.animate({ transform: ..}, 100;
> > div.aniamte({ width: [ '100px', '200px' ], opacity: [0, 1] }, 100);
> >
> > In above case, ShouldBlockAsyncTransformAnimations (this is called against the second animation's effect) surely returns true, but tried to set the warning to the second animation's effect in the previous patch because unfortunately the effect had no 'transform' property.
> > Adding test cases cover this case.
>
> Sorry, the comment was about whether we need to replace aProperty with
> eCSSProperty_transform. Since we know aProperty == eCSSProperty_transform,
> why make the change?
I wanted to make it clear to set the warning about transform property. But actually it failed. As I commented in #13, that sets *transform* against a wrong keyframe effect!
|
|
||
Comment 17•9 years ago
|
||
https://reviewboard.mozilla.org/r/38387/#review34959
> One thing is not clear to me is that we should set a performance warning to the first animation? If we do not set the warning to the first animation(top/left), we don't need to call ShouldBlockAsyncTransformAnimations at all.
We should set it on the second animation, I think. The purpose of the explanation is to say why *this* animation is not running on the compositor--i.e. identify the victim not the culprit.
|
|
Reporter | |
Comment 18•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #17)
> https://reviewboard.mozilla.org/r/38387/#review34959
>
> > One thing is not clear to me is that we should set a performance warning to the first animation? If we do not set the warning to the first animation(top/left), we don't need to call ShouldBlockAsyncTransformAnimations at all.
>
> We should set it on the second animation, I think. The purpose of the
> explanation is to say why *this* animation is not running on the
> compositor--i.e. identify the victim not the culprit.
Thanks for the clarification. I did misread your comment #5.
(In reply to Brian Birtles (:birtles) from comment #5)
> * One animates top/left
> * One animates transform
>
> We **should** call this method on the first animation's KeyframeEffect even
> though it doesn't animate the transform property, right?
|
|
||
Updated•9 years ago
|
Attachment #8727202 -
Flags: review?(bbirtles) → review+
|
|
||
Comment 19•9 years ago
|
||
Comment on attachment 8727202 [details]
MozReview Request: Bug 1218620 - Allow opacity animation running on compositor even if the frame has any restricted transforms. r?birtles
https://reviewboard.mozilla.org/r/38387/#review36085
::: dom/animation/test/chrome/test_animation_property_state.html:545
(Diff revision 2)
> + animations.forEach(t.step_func(function(anim) {
I'm curious, do we actually need to use t.step_func here, or are we already running in the right context?
::: dom/animation/test/chrome/test_animation_property_state.html:614
(Diff revision 2)
> + // Now transform animations is not running on compositor because of
Nit: transform animations are
::: dom/animation/test/chrome/test_animation_property_state.html:625
(Diff revision 2)
> + // Now all animations is running on compositor.
Nit: all animations are
::: dom/locales/en-US/chrome/layout/layout_errors.properties:32
(Diff revision 2)
> +AnimationWarningTransformWithGeometricProperties=Async animation of 'transform' not possible due to animation of geometric properties on the same element
Would you mind filing a bug to update these strings to say "Compositor animation" instead of "Async animation"?
|
|
Reporter | |
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/38387/#review36085
> I'm curious, do we actually need to use t.step_func here, or are we already running in the right context?
What a surprise! I did not know that promise_test does not need step_func at all! Filed bug 1255682.
> Would you mind filing a bug to update these strings to say "Compositor animation" instead of "Async animation"?
Filed bug 1255683.
|
|
Reporter | |
Comment 21•9 years ago
|
||
Comment on attachment 8727202 [details]
MozReview Request: Bug 1218620 - Allow opacity animation running on compositor even if the frame has any restricted transforms. r?birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38387/diff/2-3/
|
|
Reporter | |
Comment 22•9 years ago
|
||
A try run shows us there are two problems.
i) Some of tests fail if test_animation_properties.html is run after other test. (I did run it locally without any other tests)
https://treeherder.mozilla.org/logviewer.html#?job_id=17928478&repo=try
ii) On android, an assertion was hit at [1]
https://treeherder.mozilla.org/logviewer.html#?job_id=17927945&repo=try
[1] https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/gfx/layers/apz/src/HitTestingTreeNode.cpp#87
I am guessing these two problems are not caused by this fix, these new test cases just dug up other underlying problems. I am going to investigate in further detail.
|
|
Reporter | |
Comment 23•9 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> A try run shows us there are two problems.
>
> i) Some of tests fail if test_animation_properties.html is run after other
> test. (I did run it locally without any other tests)
> https://treeherder.mozilla.org/logviewer.html#?job_id=17928478&repo=try
This first problem is not fault of this bug. Filed bug 1255710 for it.
|
|
Reporter | |
Comment 24•9 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> ii) On android, an assertion was hit at [1]
> https://treeherder.mozilla.org/logviewer.html#?job_id=17927945&repo=try
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> dd1abe874252e507b825a0a4e1063b0e13578288/gfx/layers/apz/src/
> HitTestingTreeNode.cpp#87
Filed bug 1255725.
|
|
Reporter | |
Comment 25•9 years ago
|
||
Comment on attachment 8727202 [details]
MozReview Request: Bug 1218620 - Allow opacity animation running on compositor even if the frame has any restricted transforms. r?birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38387/diff/3-4/
|
|
Reporter | |
Comment 26•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39653/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39653/
Attachment #8730073 -
Flags: review?(bbirtles)
|
|
Reporter | |
Comment 27•9 years ago
|
||
Brian, this bug blocks (or will conflict) your work (bug 1254419). The two problems found by the test cases for this bug are basically not related to this bug. So I'd suggest to disable the test cases here and handle those two problems later in each bugs respectively. What do you think?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f4d9b8e81e
|
|
||
Comment 28•9 years ago
|
||
Comment on attachment 8730073 [details]
MozReview Request: Bug 1218620 - Part 2: Skip all 'preserve-3d' tests which breaks other compositor frames. r?birtles
https://reviewboard.mozilla.org/r/39653/#review36311
Attachment #8730073 -
Flags: review?(bbirtles) → review+
|
||
Comment 29•9 years ago
|
||
|
||
Comment 30•9 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a811164e762d
https://hg.mozilla.org/mozilla-central/rev/acbe5c09bbb7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•