Bug 193343

Summary: Animation and other code is too aggressive about invalidating layer composition
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: AnimationsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, koivisto, rniwa, sabouhallawa, simon.fraser
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193435
https://bugs.webkit.org/show_bug.cgi?id=193450
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews205 for win-future
none
Patch
graouts: review+
Patch
none
Patch
graouts: review+
Testcase none

Description Simon Fraser (smfr) 2019-01-10 18:55:27 PST 
Animation and other code is too aggressive about invalidating layer composition
Comment 1 Simon Fraser (smfr) 2019-01-10 18:55:48 PST 
Created attachment 358859 [details]
Patch
Comment 2 EWS Watchlist 2019-01-10 18:58:10 PST Comment hidden (obsolete)
Comment 3 EWS Watchlist 2019-01-10 19:57:44 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-01-10 19:57:46 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-01-10 20:07:47 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-01-10 20:07:49 PST Comment hidden (obsolete)
Comment 7 Simon Fraser (smfr) 2019-01-10 20:13:48 PST 
I guess we need some of these.
Comment 8 Simon Fraser (smfr) 2019-01-10 20:28:05 PST 
> New failing tests:
> legacy-animation-engine/compositing/visible-rect/animated-from-none.html
> legacy-animation-engine/compositing/animation/animation-compositing.html
> legacy-animation-engine/compositing/visible-rect/animated.html
> legacy-animation-engine/compositing/layer-creation/translate-animation-
> overlap.html
> legacy-animation-engine/compositing/contents-scale/animating.html
> legacy-animation-engine/compositing/backing/backing-store-attachment-empty-
> keyframe.html
> legacy-animation-engine/compositing/layer-creation/scale-rotation-animation-
> overlap.html
> legacy-animation-engine/compositing/layer-creation/overlap-animation.html
> css3/filters/reference-filter-update-on-attribute-change.html
> css3/filters/effect-reference-delete.html
> legacy-animation-engine/compositing/layer-creation/overlap-animation-
> container.html
> legacy-animation-engine/compositing/layer-creation/overlap-animation-
> clipping.html

Most of these tests seem to dump layers in response to an animationStart event, and that timing changes with this patch. The tests look OK when run manually; I think we'll just create the layers later.
Comment 9 Simon Fraser (smfr) 2019-01-10 20:29:32 PST 
rdar://problem/44366110
Comment 10 EWS Watchlist 2019-01-10 20:40:00 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-01-10 20:40:02 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-01-10 20:44:51 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-01-10 20:44:53 PST 
Created attachment 358870 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2019-01-10 21:49:39 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-01-10 21:49:51 PST Comment hidden (obsolete)
Comment 16 Simon Fraser (smfr) 2019-01-11 11:01:29 PST 
This patch breaks accelerated animations, because we don't do a compositing update after the animation starts (AnimationState New -> StartWaitTimer), so end up running the animation in software.
Comment 17 Simon Fraser (smfr) 2019-01-11 12:31:43 PST 
We need to have already created the composing layer before:

  * frame #0: 0x0000000108bf98d4 WebCore`WebCore::KeyframeAnimation::startAnimation(this=0x0000000122dc95b0, timeOffset=0) at KeyframeAnimation.cpp:288
    frame #1: 0x0000000108baa805 WebCore`WebCore::AnimationBase::updateStateMachine(this=0x0000000122dc95b0, input=StyleAvailable, param=-1) at AnimationBase.cpp:276
    frame #2: 0x0000000108bb3378 WebCore`WebCore::AnimationBase::styleAvailable(this=0x0000000122dc95b0) at AnimationBase.h:207
    frame #3: 0x0000000108bb1c6d WebCore`WebCore::CSSAnimationControllerPrivate::styleAvailable(this=0x0000000122d996e0) at CSSAnimationController.cpp:529
    frame #4: 0x0000000108bb1b7c WebCore`WebCore::CSSAnimationControllerPrivate::endAnimationUpdate(this=0x0000000122d996e0) at CSSAnimationController.cpp:458
    frame #5: 0x0000000108bc26f8 WebCore`WebCore::AnimationPrivateUpdateBlock::~AnimationPrivateUpdateBlock(this=0x00007ffeefbfcbf0) at CSSAnimationController.cpp:65
    frame #6: 0x0000000108bafb35 WebCore`WebCore::AnimationPrivateUpdateBlock::~AnimationPrivateUpdateBlock(this=0x00007ffeefbfcbf0) at CSSAnimationController.cpp:64
    frame #7: 0x0000000108badebd WebCore`WebCore::CSSAnimationControllerPrivate::animationTimerFired(this=0x0000000122d996e0) at CSSAnimationController.cpp:293
Comment 18 Simon Fraser (smfr) 2019-01-11 12:44:34 PST 
This is a delicate dance about doing compositing updates at the right time. We have to do a compositing update when the animation is in StartWaitStyleAvailable, not before!
Comment 19 Simon Fraser (smfr) 2019-01-11 13:18:43 PST 
Created attachment 358933 [details]
Patch
Comment 20 EWS Watchlist 2019-01-11 13:21:41 PST 
Attachment 358933 [details] did not pass style-queue:


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: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Simon Fraser (smfr) 2019-01-11 18:51:27 PST 
Created attachment 358970 [details]
Patch
Comment 22 Simon Fraser (smfr) 2019-01-11 18:56:16 PST 
Created attachment 358972 [details]
Patch
Comment 23 Simon Fraser (smfr) 2019-01-11 18:57:14 PST 
This patch sequence tidies up the legacy animation code path so that only animation changes that start/stop accelerated animations trigger recompositeLayer.

The Web Animations code path is fixed by the first patch.
Comment 24 Simon Fraser (smfr) 2019-01-11 18:57:30 PST 
Created attachment 358973 [details]
Testcase
Comment 25 Antti Koivisto 2019-01-12 00:20:16 PST 
Comment on attachment 358933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358933&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:63
> -        element->invalidateStyleAndLayerComposition();
> +        element->invalidateStyle();

Are there any clients left for invalidateStyleAndLayerComposition after this? Can we remove the whole concept?
Comment 26 Simon Fraser (smfr) 2019-01-12 10:53:32 PST 
Yes, there are clients: things related to composited iframes, plugins etc.
Comment 27 Simon Fraser (smfr) 2019-01-14 22:08:01 PST 
Comment on attachment 358970 [details]
Patch

I moved this patch to bug 193435.
Comment 28 Simon Fraser (smfr) 2019-01-15 08:44:20 PST 
Comment on attachment 358972 [details]
Patch

I moved this patch to bug 193450.
Comment 29 Simon Fraser (smfr) 2019-01-15 17:58:50 PST 
Main patch landed in r239965, other patches landed via other bugs.