Bug 127014 – AnimationController: replace animation timer with compositor-driven mechanism

• Component: New Bugs
• Status: NEW

[Ralph T]

AnimationController: replace animation timer with compositor-driven mechanism

[Ralph T]

Created attachment 221210 [details]
Patch

[WebKit Commit Bot]

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.

[Ralph T]

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.

[EFL EWS Bot]

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

[EFL EWS Bot]

Comment on attachment 221210 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 221210 [details]
Patch

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Ralph T]

Created attachment 221409 [details]
Patch

[WebKit Commit Bot]

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.

[EFL EWS Bot]

Comment on attachment 221409 [details]
Patch

Attachment 221409 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5462993718476800

[Ralph T]

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.

[Ralph T]

Created attachment 221412 [details]
Patch

[Ralph T]

Comment on attachment 221412 [details]
Patch

s/OVERRIDE/override/

[WebKit Commit Bot]

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.

[kov's GTK+ EWS bot]

Comment on attachment 221412 [details]
Patch

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