Bug 127014 - AnimationController: replace animation timer with compositor-driven mechanism
Summary: AnimationController: replace animation timer with compositor-driven mechanism
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-14 15:27 PST by Ralph T
Modified: 2016-12-21 18:35 PST (History)
15 users (show)

See Also:


Attachments
Patch (19.16 KB, patch)
2014-01-14 15:30 PST, Ralph T
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (518.52 KB, application/zip)
2014-01-14 21:50 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (513.51 KB, application/zip)
2014-01-15 02:00 PST, Build Bot
no flags Details
Patch (19.81 KB, patch)
2014-01-16 13:46 PST, Ralph T
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2014-01-16 13:57 PST, Ralph T
gtk-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph T 2014-01-14 15:27:13 PST
AnimationController: replace animation timer with compositor-driven mechanism
Comment 1 Ralph T 2014-01-14 15:30:40 PST
Created attachment 221210 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-14 15:33:17 PST
Attachment 221210 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/AnimationTimer.cpp', u'Source/WebCore/page/animation/AnimationTimer.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:38:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:44:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:54:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:55:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:59:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:76:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:52:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:53:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:103:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameView.cpp:2608:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 15 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ralph T 2014-01-14 15:59:33 PST
I switched the timer in AnimationController out for a new "AnimationTimer". AnimationTimer uses Document::requestAnimationFrame to schedule callbacks when there's no interval and when repeating -- otherwise it falls back to a traditional timer and then uses requestAnimationFrame. This requires that REQUEST_ANIMATION_FRAME is enabled.

The goal of this is to have unaccelerated CSS animations run synced with the compositor. On ports where the compositor calls serviceScriptedAnimations directly prior to flushing layers for a new frame (I do this and CoordinatedGraphics does this when R_A_F_TIMER and R_A_F_DISPLAY_MONITOR are disabled) this change results in smooth 60Hz animations.

On mac, the animation is still chunky which must be due to the rAF callbacks being driven by the DisplayMonitor instead of from the compositor when it begins compositing.

There was previous work done to achieve the same goal, here: https://bugs.webkit.org/show_bug.cgi?id=64591

I disabled this change in my patch because it has some negative consequences:

 1. When an accelerated CSS animation is running or scheduled, AnimationController will invalidate style every time some other unrelated paint happens.

 2. I see some duplicated work -- on some frames the animation code runs twice, so there are two style invals and two layouts per painted and composited frame. I think this is because the old animation timer is still running even though the rAF timer is more frequent and should be doing all the work. Visually this causes jitter as some frames get calculated late if the WebProcess was busy handling the animation timer when it should have been calculating for the compositor/rAF tiemr.

I didn't remove any of the code in AnimationController for serviceAnimations, but it's not called any more.

I'd love to get some feedback on this approach, so my questions are:
 1. Is it appropriate to use the rAF timer mechanism for the AnimationController?

 2. Do you know why the rAF callbacks on mac seem uneven? This was briefly discussed in bug 64591. It's almost like the compositor and rAF timer aren't in sync.
Comment 4 EFL EWS Bot 2014-01-14 16:58:29 PST
Comment on attachment 221210 [details]
Patch

Attachment 221210 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5917566883069952
Comment 5 EFL EWS Bot 2014-01-14 17:07:36 PST
Comment on attachment 221210 [details]
Patch

Attachment 221210 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4850917794906112
Comment 6 kov's GTK+ EWS bot 2014-01-14 21:31:32 PST
Comment on attachment 221210 [details]
Patch

Attachment 221210 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5196396911132672
Comment 7 Build Bot 2014-01-14 21:50:44 PST
Comment on attachment 221210 [details]
Patch

Attachment 221210 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5882426131742720

New failing tests:
animations/animation-welcome-safari.html
Comment 8 Build Bot 2014-01-14 21:50:47 PST
Created attachment 221229 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Build Bot 2014-01-15 02:00:05 PST
Comment on attachment 221210 [details]
Patch

Attachment 221210 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6734238942494720

New failing tests:
animations/animation-welcome-safari.html
Comment 10 Build Bot 2014-01-15 02:00:08 PST
Created attachment 221245 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Ralph T 2014-01-16 13:46:01 PST
Created attachment 221409 [details]
Patch
Comment 12 WebKit Commit Bot 2014-01-16 13:47:26 PST
Attachment 221409 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/AnimationTimer.cpp', u'Source/WebCore/page/animation/AnimationTimer.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:38:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:44:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:54:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:55:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:59:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:76:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:52:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:53:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:78:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:108:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameView.cpp:2608:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 16 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 EFL EWS Bot 2014-01-16 13:50:53 PST
Comment on attachment 221409 [details]
Patch

Attachment 221409 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5462993718476800
Comment 14 Ralph T 2014-01-16 13:53:28 PST
Comment on attachment 221409 [details]
Patch

Again, just for discussion/feedback. This should correct the LayoutTest regression the previous version had.

I'm more sure that the chunkiness on mac is from using the CA timer and the DisplayMonitor mechanism. I imagine that as mac starts using the remote layer tree that you'll want to stop using the DisplayMonitor in the WebProcess completely.

I might try a change on mac to service rAFs from the LayerFlushScheduler and see if mac is smoother then, though I'm not very familiar with the mac port, so perhaps I'm completely misunderstanding what I'm seeing.
Comment 15 Ralph T 2014-01-16 13:57:14 PST
Created attachment 221412 [details]
Patch
Comment 16 Ralph T 2014-01-16 13:57:59 PST
Comment on attachment 221412 [details]
Patch

s/OVERRIDE/override/
Comment 17 WebKit Commit Bot 2014-01-16 13:59:13 PST
Attachment 221412 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/AnimationTimer.cpp', u'Source/WebCore/page/animation/AnimationTimer.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:38:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:44:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:54:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:55:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:59:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.cpp:76:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:52:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:53:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:78:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/WebCore/page/animation/AnimationTimer.h:108:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameView.cpp:2608:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 16 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 kov's GTK+ EWS bot 2014-01-16 20:23:17 PST
Comment on attachment 221412 [details]
Patch

Attachment 221412 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/6374895806054400