New issue
Advanced search Search tips

Issue 781305 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 754471



Sign in to add a comment

CSS-animation based UMA is not reporting correctly

Project Member Reported by xidac...@chromium.org, Nov 3 2017

Issue description

In this previous CL:
https://chromium-review.googlesource.com/c/chromium/src/+/636305

We added a DCHECK in animation_host.cc to make sure that the main thread animation is >= main thread compositable animation. However, it appears that we could hit this DCHECK in the real world.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38b843161cf67b4b178e7d788ea3b028a2475eef

commit 38b843161cf67b4b178e7d788ea3b028a2475eef
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Nov 03 23:00:46 2017

Temporary disable main thread compositable animation UMA

In AnimationHost::SetAnimationCounts(), we have a DCHECK to make sure
that main_thread_animations_count_ is always >=
main_thread_compositable_animations_count_. It appears that this could
fail in certain cases. In particular, right now this is causing constant
crash when chrome cast team is running some testing with debug version.

Due to the severity of this bug, this CL temporary set the main thread
compositable animations to be always 0 so that it won't report anything.
I will further investigate for the root cause.

Bug:  781305 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I9a4a4f4111b24ef5e5e3d87fd9588402dde142bd
Reviewed-on: https://chromium-review.googlesource.com/753405
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513930}
[modify] https://crrev.com/38b843161cf67b4b178e7d788ea3b028a2475eef/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/38b843161cf67b4b178e7d788ea3b028a2475eef/third_party/WebKit/Source/core/animation/DocumentTimeline.cpp

Blocking: 754471
Components: Blink>Animation
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac51db7592b74c8d627e09b80e0e8af2d347ba36

commit ac51db7592b74c8d627e09b80e0e8af2d347ba36
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Feb 06 22:40:03 2018

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/895409
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534820}
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/cc/animation/animation_host.cc
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/animation/CompositorAnimations.cpp
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/animation/CompositorAnimations.h
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/animation/DocumentTimeline.cpp
[add] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/animation/test_data/transform-animation-on-svg.html
[modify] https://crrev.com/ac51db7592b74c8d627e09b80e0e8af2d347ba36/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinder.cpp

Labels: Hotlist-Metrics
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0e80e68c0a5670217965b3aeedb4783dbf0587d

commit c0e80e68c0a5670217965b3aeedb4783dbf0587d
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Feb 16 18:16:34 2018

Simplify logic of computing main thread compositable animations

A main thread compositable animation means that this animation should
be composited such as an opacity or 2d transform, but it is not due to
certain check such as
RuntimeEnabledFeatures::TurnOff2DAndOpacityCompositorAnimationsEnabled
which is designed to be used for an experiment described in
crbug.com/754471.

Currently our logic is:
1. Check if the effect (such as opacity) can start on compositor or not
2. If 1 is true, and the target element is not paint into its own
   backing + the target element is not a SVG, then it is a main thread
   compositable animation.

This logic is over complicated. We should really just check whether the
run time flag is enabled or not. So this CL simplifies it. We already have
unit test in CompositorAnimationsTest to ensure that this change is
fine.

Bug:  781305 
Change-Id: I8f9a1d82fa60eea79018c6cf777a1b45bd565022
Reviewed-on: https://chromium-review.googlesource.com/919316
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537351}
[modify] https://crrev.com/c0e80e68c0a5670217965b3aeedb4783dbf0587d/third_party/WebKit/Source/core/animation/CompositorAnimations.cpp
[modify] https://crrev.com/c0e80e68c0a5670217965b3aeedb4783dbf0587d/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment