| Summary: | Don't service animations on both the animation frame callback and animation timer at the same time | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | dstockwell | ||||||||
| Component: | Animations | Assignee: | dstockwell | ||||||||
| Status: | RESOLVED LATER | ||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, ap, bdakin, bfulgham, buildbot, dglazkov, dino, graouts, graouts, jamesr, nduca, rniwa, simon.fraser, syoichi, vollick, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 107609, 108582 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
dstockwell
2013-01-22 01:29:23 PST
Created attachment 183926 [details]
Patch
Created attachment 183931 [details]
Patch
Comment on attachment 183931 [details] Patch Attachment 183931 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16041503 New failing tests: compositing/reflections/animation-inside-reflection.html compositing/layer-creation/overlap-animation-clipping.html fast/css-generated-content/pseudo-element-events.html css3/filters/custom/custom-filter-transforms-animation.html animations/3d/matrix-transform-type-animation.html animations/animation-add-events-in-handler.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/visible-rect/animated-from-none.html compositing/repaint/become-overlay-composited-layer.html css3/masking/clip-path-animation.html css3/filters/composited-during-animation-layertree.html animations/animation-border-overflow.html compositing/animation/state-at-end-event-transform-layer.html compositing/animation/animation-compositing.html compositing/layer-creation/overlap-animation.html animations/3d/change-transform-in-end-event.html css3/filters/filter-animation-from-none-multi.html css3/filters/filter-animation-from-none.html css3/filters/filter-animation-from-none-multi-hw.html css3/filters/custom/custom-filter-array-blending.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html css3/filters/filter-animation-from-none-hw.html css3/filters/composited-during-animation.html css3/filters/filter-animation-hw.html compositing/geometry/partial-layout-update.html compositing/visible-rect/animated.html compositing/layer-creation/overlap-animation-container.html compositing/layer-creation/animation-overlap-with-children.html compositing/reflections/nested-reflection-animated.html Servicing of animations in the animation frame callback was added in http://trac.webkit.org/changeset/107575, however I don't believe it ever worked as intended. In the case of accelerated animations every animation frame callback requested another (see http://trac.webkit.org/browser/trunk/Source/WebCore/page/animation/AnimationController.cpp#L232). In the case of software animations calls to updateAnimations(..) are triggered by both the animation timer and the animation frame callback. I believe the failures we're seeing with my patch are because DumpRenderTree (at least for chromium, I will test mac tomorrow) doesn't generate frames unless requested (eg. testRunner.display()) By adding the following to the layout tests I can cause them to pass, however I'm not sure whether it's appropriate to do so: setInterval(function() { testRunner.display() }, 16); Comment on attachment 183931 [details] Patch Attachment 183931 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16011798 New failing tests: compositing/reflections/animation-inside-reflection.html compositing/layer-creation/overlap-animation-clipping.html animations/3d/matrix-transform-type-animation.html animations/animation-direction.html animations/3d/state-at-end-event-transform.html animations/animation-add-events-in-handler.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/repaint/become-overlay-composited-layer.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html compositing/animation/state-at-end-event-transform-layer.html animations/animation-direction-reverse-fill-mode.html compositing/animation/animation-compositing.html compositing/layer-creation/overlap-animation.html animations/3d/change-transform-in-end-event.html animations/animation-direction-reverse-hardware.html animations/animation-direction-reverse-timing-functions.html http/tests/eventsource/eventsource-cors-with-credentials.html animations/3d/transform-perspective.html animations/animation-drt-api-multiple-keyframes.html animations/animation-direction-reverse-non-hardware.html animations/animation-direction-normal.html animations/3d/transform-origin-vs-functions.html animations/animation-direction-reverse.html compositing/geometry/partial-layout-update.html animations/animation-direction-reverse-fill-mode-hardware.html compositing/layer-creation/overlap-animation-container.html animations/animation-direction-reverse-hardware-opacity.html animations/animation-direction-reverse-timing-functions-hardware.html (In reply to comment #4) > Servicing of animations in the animation frame callback was added in http://trac.webkit.org/changeset/107575, however I don't believe it ever worked as intended. > > In the case of accelerated animations every animation frame callback requested another (see http://trac.webkit.org/browser/trunk/Source/WebCore/page/animation/AnimationController.cpp#L232). > > In the case of software animations calls to updateAnimations(..) are triggered by both the animation timer and the animation frame callback. > > > I believe the failures we're seeing with my patch are because DumpRenderTree (at least for chromium, I will test mac tomorrow) doesn't generate frames unless requested (eg. testRunner.display()) > > By adding the following to the layout tests I can cause them to pass, however I'm not sure whether it's appropriate to do so: > > setInterval(function() { testRunner.display() }, 16); I'm still not sure from this comment what the actual bug is, and how you're trying to fix it. (In reply to comment #6) Hi Simon, Thanks for taking a look. > I'm still not sure from this comment what the actual bug is, The bug is that animation servicing is being triggered by both the animation frame callback and a timer inside AnimationController. http://trac.webkit.org/changeset/107575 (Use requestAnimationFrame callbacks to pump CSS animations) added the use of request animation frame, but didn’t modifiy the existing timing mechanism. This manifests in several different ways: In the case of software animations it results in a style recalc frequently being triggered twice per frame. (on each frame callback in addition to the 40hz animation timer) In the case of an animation with a start delay it results in frames being generated even while we are still waiting for the start delay to elapse. In the case of exclusive accelerated animations it results in constant scheduling of request animation frame while the accelerated animation is running -- results in unnecessary use of the main thread. > and how you're trying to fix it. My attempted fix is to combine the scheduling logic between request animation frame and the animation timer so that: * if servicing of animations is required immediately we use request animation frame (only) * in any other case we use the animation timer Do CSS animation events fire with your patch on backgrounded tabs? (In reply to comment #8) > Do CSS animation events fire with your patch on backgrounded tabs? No, they do not. I can see how in the past the timer would have caused them to fire. I will look into addressing this. However they also do not fire on background tabs in Chrome Canary or WebKit Nightly... any ideas? (In reply to comment #9) > However they also do not fire on background tabs in Chrome Canary or WebKit Nightly... any ideas? I bisected all the way to: Suspend CSS animations for background tabs https://bugs.webkit.org/show_bug.cgi?id=106940 Sounds like that's deliberate behavior, then, so no need to worry about firing the CSS events. Comment on attachment 183931 [details] Patch Attachment 183931 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16033939 New failing tests: compositing/reflections/animation-inside-reflection.html compositing/layer-creation/overlap-animation-clipping.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html animations/animation-add-events-in-handler.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/visible-rect/animated-from-none.html compositing/repaint/become-overlay-composited-layer.html css3/filters/composited-during-animation-layertree.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html compositing/animation/state-at-end-event-transform-layer.html compositing/animation/animation-compositing.html compositing/layer-creation/overlap-animation.html animations/3d/transform-perspective.html animations/animation-direction-normal.html animations/3d/transform-origin-vs-functions.html css3/filters/composited-during-animation.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html compositing/geometry/partial-layout-update.html compositing/visible-rect/animated.html compositing/layer-creation/overlap-animation-container.html compositing/layer-creation/animation-overlap-with-children.html compositing/reflections/nested-reflection-animated.html (In reply to comment #11) > Sounds like that's deliberate behavior, then, so no need to worry about firing the CSS events. Any suggestions on what to do about the issue running under DumpRenderTree? Created attachment 185628 [details]
Patch
(In reply to comment #14) > Created an attachment (id=185628) [details] > Patch Latest patch respects USE(REQUEST_ANIMATION_FRAME_TIMER) which I expect will resolve the mac failures. Also experimenting with not suspending animations on hidden pages and servicing animations at events. Still expect failures on Chromium DRT. Comment on attachment 185628 [details] Patch Attachment 185628 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16260060 New failing tests: compositing/reflections/animation-inside-reflection.html compositing/layer-creation/overlap-animation-clipping.html animations/3d/matrix-transform-type-animation.html animations/animation-direction.html animations/3d/state-at-end-event-transform.html animations/animation-add-events-in-handler.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/repaint/become-overlay-composited-layer.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html compositing/animation/state-at-end-event-transform-layer.html animations/animation-direction-reverse-fill-mode.html compositing/animation/animation-compositing.html compositing/layer-creation/overlap-animation.html animations/3d/change-transform-in-end-event.html animations/animation-direction-reverse-hardware.html animations/animation-drt-api.html animations/animation-direction-reverse-timing-functions.html animations/3d/transform-perspective.html animations/animation-drt-api-multiple-keyframes.html animations/animation-direction-reverse-non-hardware.html animations/animation-direction-normal.html animations/3d/transform-origin-vs-functions.html animations/animation-direction-reverse.html compositing/geometry/partial-layout-update.html animations/animation-direction-reverse-fill-mode-hardware.html compositing/layer-creation/overlap-animation-container.html animations/animation-direction-reverse-hardware-opacity.html animations/animation-direction-reverse-timing-functions-hardware.html Comment on attachment 185628 [details] Patch Attachment 185628 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16253155 New failing tests: fast/loader/image-in-page-cache.html fast/history/timed-refresh-in-cached-frame.html css3/filters/composited-during-animation-layertree.html fast/frames/frame-dead-region.html fast/loader/stateobjects/popstate-fires-with-page-cache.html plugins/frameset-with-plugin-frame.html fast/harness/user-preferred-language.html Comment on attachment 185628 [details] Patch Attachment 185628 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16273018 New failing tests: fast/loader/image-in-page-cache.html fast/history/timed-refresh-in-cached-frame.html css3/filters/composited-during-animation-layertree.html fast/frames/frame-dead-region.html fast/harness/results.html plugins/frameset-with-plugin-frame.html fast/loader/stateobjects/pushstate-clears-forward-history.html fast/harness/user-preferred-language.html fast/dom/Geolocation/not-enough-arguments.html This seems to have landed in Blink via following: https://bugs.chromium.org/p/chromium/issues/detail?id=227161#c4 Is it something needed in Safari, there is no test case to verify on how this improve performance or web-compat but just wanted to share updated information. We rewrote animation code enough that I highly doubt this patch is relevant. |