Commit ac51db75 authored by Xida Chen's avatar Xida Chen Committed by Commit Bot

Fix main thread compositable animation UMA

I had this CL that implements animation specific UMA:
https://chromium-review.googlesource.com/c/chromium/src/+/636305
In particular, we record frame rate when there is main thread / compositor /
main thread compositable animations in this frame. However, the main
thread compositable animations is not calculated correctly in the above
CL. The result of that is a crash which is tracked in crbug.com/781305.
In order to solve the crash, we temporary disabled the main thread
compositable animation UMA in here:
https://chromium-review.googlesource.com/c/chromium/src/+/753405

Now it is time to re-enable that UMA. This is how we check whether an
animation is main thread compositable or not: we first check whether the
effects can start on compositor or not. For example, a transform animation
will be able to start on compositor. Then we check whether the target
element can start on compositor or not. If the target element is not
paint into its own backing, we know that this is due to a running
experiment.

The problem with the above logic is SVG element. For example, if we
have a transform animation on an SVG element, the effect can be started
on compositor but the SVG element cannot start on compositor. In order
to tell whether this is a main thread compositable, we have to exclude
the SVG element.

This CL fixed the problem. It re-enables a unit test and added another
test for SVG element.

Bug: 781305
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I030f2e7d2061589ccb0a62f12744b1097855f2f7
Reviewed-on: https://chromium-review.googlesource.com/895409Reviewed-by: default avatarChris Harrelson <chrishtr@chromium.org>
Reviewed-by: default avatarStephen McGruer <smcgruer@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534820}
parent 56fafb98
...@@ -633,11 +633,18 @@ size_t AnimationHost::CompositedAnimationsCount() const { ...@@ -633,11 +633,18 @@ size_t AnimationHost::CompositedAnimationsCount() const {
void AnimationHost::SetAnimationCounts( void AnimationHost::SetAnimationCounts(
size_t total_animations_count, size_t total_animations_count,
size_t main_thread_compositable_animations_count) { size_t main_thread_compositable_animations_count) {
size_t composited_animations_count = CompositedAnimationsCount(); // The |total_animations_count| is the total number of blink::Animations.
// A blink::Animation holds a CompositorAnimationPlayerHolder, which holds
// a CompositorAnimationPlayer, which holds a cc::AnimationPlayer. In other
// words, if a blink::Animation can be accelerated on compositor, it would
// have a 1:1 mapping to a cc::AnimationPlayer.
// So to check how many main thread animations there are, we subtract the
// number of cc::AnimationPlayer from |total_animations_count|.
size_t ticking_players_count = ticking_players_.size();
if (main_thread_animations_count_ != if (main_thread_animations_count_ !=
total_animations_count - composited_animations_count) { total_animations_count - ticking_players_count) {
main_thread_animations_count_ = main_thread_animations_count_ =
total_animations_count - composited_animations_count; total_animations_count - ticking_players_count;
DCHECK_GE(main_thread_animations_count_, 0u); DCHECK_GE(main_thread_animations_count_, 0u);
SetNeedsPushProperties(); SetNeedsPushProperties();
} }
......
...@@ -290,25 +290,35 @@ CompositorAnimations::CheckCanStartElementOnCompositor( ...@@ -290,25 +290,35 @@ CompositorAnimations::CheckCanStartElementOnCompositor(
} }
} }
} else { } else {
LayoutObject* layout_object = target_element.GetLayoutObject();
bool paints_into_own_backing = bool paints_into_own_backing =
target_element.GetLayoutObject() && layout_object &&
target_element.GetLayoutObject()->GetCompositingState() == layout_object->GetCompositingState() == kPaintsIntoOwnBacking;
kPaintsIntoOwnBacking;
// This function is called in CheckCanStartAnimationOnCompositor(), after // This function is called in CheckCanStartAnimationOnCompositor(), after
// CheckCanStartEffectOnCompositor returns code.Ok(), which means that we // CheckCanStartEffectOnCompositor returns code.Ok(), which means that we
// know this animation could be accelerated. If |!paints_into_own_backing|, // know the effect (such as transform) could be accelerated. If the target
// then we know that the animation is not composited due to certain check, // element is SVG, then we should not return code.Ok() because a SVG
// such as the ComputedStyle::ShouldCompositeForCurrentAnimations(), for // animation cannot be composited. If the target element is not SVG, and
// a running experiment. // |!paints_into_own_backing|, then we know that the animation is not
// composited due to certain check, such as the
// ComputedStyle::ShouldCompositeForCurrentAnimations(), for a running
// experiment.
bool is_svg_element = layout_object && layout_object->IsSVG();
if (!paints_into_own_backing) { if (!paints_into_own_backing) {
return FailureCode::NotPaintIntoOwnBacking( if (!is_svg_element) {
"Acceleratable animation not accelerated due to an experiment"); return FailureCode::AcceleratableAnimNotAccelerated(
"Acceleratable animation not accelerated due to an experiment");
} else {
return FailureCode::NonActionable(
"Element does not paint into own backing");
}
} }
} }
return FailureCode::None(); return FailureCode::None();
} }
// TODO(crbug.com/809685): consider refactor this function.
CompositorAnimations::FailureCode CompositorAnimations::FailureCode
CompositorAnimations::CheckCanStartAnimationOnCompositor( CompositorAnimations::CheckCanStartAnimationOnCompositor(
const Timing& timing, const Timing& timing,
......
...@@ -68,7 +68,7 @@ class CORE_EXPORT CompositorAnimations { ...@@ -68,7 +68,7 @@ class CORE_EXPORT CompositorAnimations {
static FailureCode NonActionable(const String& reason) { static FailureCode NonActionable(const String& reason) {
return FailureCode(false, false, false, reason); return FailureCode(false, false, false, reason);
} }
static FailureCode NotPaintIntoOwnBacking(const String& reason) { static FailureCode AcceleratableAnimNotAccelerated(const String& reason) {
return FailureCode(true, false, false, reason); return FailureCode(true, false, false, reason);
} }
...@@ -160,6 +160,8 @@ class CORE_EXPORT CompositorAnimations { ...@@ -160,6 +160,8 @@ class CORE_EXPORT CompositorAnimations {
canStartElementOnCompositorEffectSPv2); canStartElementOnCompositorEffectSPv2);
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest, FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
canStartElementOnCompositorEffect); canStartElementOnCompositorEffect);
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
cannotStartElementOnCompositorEffectSVG);
FRIEND_TEST_ALL_PREFIXES( FRIEND_TEST_ALL_PREFIXES(
AnimationCompositorAnimationsTest, AnimationCompositorAnimationsTest,
cannotStartElementOnCompositorEffectWithRuntimeFeature); cannotStartElementOnCompositorEffectWithRuntimeFeature);
......
...@@ -1320,9 +1320,31 @@ TEST_F(AnimationCompositorAnimationsTest, canStartElementOnCompositorEffect) { ...@@ -1320,9 +1320,31 @@ TEST_F(AnimationCompositorAnimationsTest, canStartElementOnCompositorEffect) {
EXPECT_EQ(host->GetMainThreadCompositableAnimationsCountForTesting(), 0u); EXPECT_EQ(host->GetMainThreadCompositableAnimationsCountForTesting(), 0u);
EXPECT_EQ(host->GetCompositedAnimationsCountForTesting(), 1u); EXPECT_EQ(host->GetCompositedAnimationsCountForTesting(), 1u);
} }
// TODO(xidachen): test temporary disabled due to crbug.com/781305.
// Regression test for crbug.com/781305. When we have a transform animation
// on a SVG element, the effect can be started on compositor but the element
// itself cannot. The animation should not be a main thread compositable
// animation.
TEST_F(AnimationCompositorAnimationsTest,
cannotStartElementOnCompositorEffectSVG) {
LoadTestData("transform-animation-on-svg.html");
Document* document = GetFrame()->GetDocument();
Element* target = document->getElementById("dots");
CompositorAnimations::FailureCode code =
CompositorAnimations::CheckCanStartElementOnCompositor(*target);
EXPECT_EQ(code, CompositorAnimations::FailureCode::NonActionable(
"Element does not paint into own backing"));
EXPECT_EQ(document->Timeline().PendingAnimationsCount(), 4u);
EXPECT_EQ(document->Timeline().MainThreadCompositableAnimationsCount(), 0u);
CompositorAnimationHost* host =
document->View()->GetCompositorAnimationHost();
EXPECT_EQ(host->GetMainThreadAnimationsCountForTesting(), 4u);
EXPECT_EQ(host->GetMainThreadCompositableAnimationsCountForTesting(), 0u);
EXPECT_EQ(host->GetCompositedAnimationsCountForTesting(), 0u);
}
TEST_F(AnimationCompositorAnimationsTest, TEST_F(AnimationCompositorAnimationsTest,
DISABLED_cannotStartElementOnCompositorEffectWithRuntimeFeature) { cannotStartElementOnCompositorEffectWithRuntimeFeature) {
ScopedTurnOff2DAndOpacityCompositorAnimationsForTest ScopedTurnOff2DAndOpacityCompositorAnimationsForTest
turn_off_2d_and_opacity_compositors_animation(true); turn_off_2d_and_opacity_compositors_animation(true);
LoadTestData("transform-animation.html"); LoadTestData("transform-animation.html");
...@@ -1335,7 +1357,7 @@ TEST_F(AnimationCompositorAnimationsTest, ...@@ -1335,7 +1357,7 @@ TEST_F(AnimationCompositorAnimationsTest,
CompositorAnimations::CheckCanStartElementOnCompositor(*target); CompositorAnimations::CheckCanStartElementOnCompositor(*target);
EXPECT_EQ( EXPECT_EQ(
code, code,
CompositorAnimations::FailureCode::NotPaintIntoOwnBacking( CompositorAnimations::FailureCode::AcceleratableAnimNotAccelerated(
"Acceleratable animation not accelerated due to an experiment")); "Acceleratable animation not accelerated due to an experiment"));
EXPECT_EQ(document->Timeline().PendingAnimationsCount(), 1u); EXPECT_EQ(document->Timeline().PendingAnimationsCount(), 1u);
EXPECT_EQ(document->Timeline().MainThreadCompositableAnimationsCount(), 1u); EXPECT_EQ(document->Timeline().MainThreadCompositableAnimationsCount(), 1u);
......
...@@ -193,7 +193,11 @@ void DocumentTimeline::DocumentTimelineTiming::Trace(blink::Visitor* visitor) { ...@@ -193,7 +193,11 @@ void DocumentTimeline::DocumentTimelineTiming::Trace(blink::Visitor* visitor) {
size_t DocumentTimeline::MainThreadCompositableAnimationsCount() const { size_t DocumentTimeline::MainThreadCompositableAnimationsCount() const {
size_t main_thread_compositable_animations_count = 0; size_t main_thread_compositable_animations_count = 0;
// TODO(crbug.com/781305): Restore the calculation here. for (Animation* animation : animations_needing_update_) {
if (animation->IsNonCompositedCompositable() &&
animation->PlayStateInternal() != Animation::kFinished)
main_thread_compositable_animations_count++;
}
return main_thread_compositable_animations_count; return main_thread_compositable_animations_count;
} }
......
<!DOCTYPE html>
<html>
<button id="dotsbutton" style="border: 0; background: transparent; padding: 0;">
<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 9 9" id="dots">
<rect x="4" y="4" width="1" height="1" rx=".5" ry=".5" fill="#4285f4" style="transform-origin: 50%; will-change: transform;"></rect>
<rect x="4" y="4" width="1" height="1" rx=".5" ry=".5" fill="#ea4335" style="transform-origin: 50%; will-change: transform;"></rect>
<rect x="4" y="4" width="1" height="1" rx=".5" ry=".5" fill="#faBB05" style="transform-origin: 50%; will-change: transform;"></rect>
<rect x="4" y="4" width="1" height="1" rx=".5" ry=".5" fill="#34a853" style="transform-origin: 50%; will-change: transform;"></rect>
</svg>
</button>
<script>
var dots = Array.prototype.slice.call(document.getElementsByTagName('rect'));
function getLineOffsetX(dotIndex) {
return -3 + dotIndex * 2;
}
function transitionTo(dotIndex, transform, duration = 150, delay = 0) {
const dot = dots[dotIndex];
const originalTransform = getComputedStyle(dot).getPropertyValue('transform');
const animation = dot.animate([
{ transform: originalTransform },
{ transform },
], {
easing: 'cubic-bezier(.0,.0,.2,1)',
fill: 'both',
delay,
duration,
});
}
function showListening() {
const LATENCY_PER_DOT_MS = 200;
for (let i = 0; i < dots.length; i++) {
const offsetX = getLineOffsetX(i);
transitionTo(i, `translateX(${offsetX}px)`, 150 + i * LATENCY_PER_DOT_MS);
}
}
showListening();
</script>
</html>
...@@ -188,6 +188,10 @@ CompositingReasons CompositingReasonFinder::CompositingReasonsForAnimation( ...@@ -188,6 +188,10 @@ CompositingReasons CompositingReasonFinder::CompositingReasonsForAnimation(
reasons |= CompositingReason::kActiveFilterAnimation; reasons |= CompositingReason::kActiveFilterAnimation;
if (RequiresCompositingForBackdropFilterAnimation(style)) if (RequiresCompositingForBackdropFilterAnimation(style))
reasons |= CompositingReason::kActiveBackdropFilterAnimation; reasons |= CompositingReason::kActiveBackdropFilterAnimation;
// TODO(crbug.com/754471): remove the next two lines when the experiment is
// completed.
if (!style.ShouldCompositeForCurrentAnimations())
reasons = CompositingReason::kNone;
return reasons; return reasons;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment