- Revision
- 234073
- Author
- [email protected]
- Date
- 2018-07-20 16:33:39 -0700 (Fri, 20 Jul 2018)
Log Message
Remove completed animations from GraphicsLayer, thus avoiding excessive backing store allocation
https://bugs.webkit.org/show_bug.cgi?id=187844
rdar://problem/40387294
Reviewed by Dean Jackson.
Source/WebCore:
A keyframe animation which animates 3D transforms, and is fill-forwards, currently
leaves the GraphicsLayer in a state where it has a "running" animation. However, the
logic that computes animation extent in RenderLayerBacking::updateGeometry() only does
so for running or paused animations. GraphicsLayer then thinks that it has an active
transform animation with unknown extent, and refuses to detach its backing store.
This triggers excessive layer creation on some sites (e.g. https://www.kqed.org).
Fix by always removing animations from the GraphicsLayer when they finish, whether
or not they fill forwards. This is done by having KeyframeAnimation::onAnimationEnd()
always call endAnimation().
This change only fixes the non-Web Animation code path. webkit.org/b/187845 exists
to fix the other code path.
Also improve some logging that would have revealed this problem sooner.
Test: compositing/backing/backing-store-attachment-fill-forwards-animation.html
* page/animation/AnimationBase.h:
(WebCore::AnimationBase::endAnimation):
* page/animation/ImplicitAnimation.cpp:
(WebCore::ImplicitAnimation::endAnimation):
* page/animation/ImplicitAnimation.h:
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::endAnimation):
(WebCore::KeyframeAnimation::onAnimationEnd):
* page/animation/KeyframeAnimation.h:
* platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::addAnimation):
(WebCore::GraphicsLayerCA::updateCoverage):
LayoutTests:
* compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt: Added.
* compositing/backing/backing-store-attachment-fill-forwards-animation.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (234072 => 234073)
--- trunk/LayoutTests/ChangeLog 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/LayoutTests/ChangeLog 2018-07-20 23:33:39 UTC (rev 234073)
@@ -1,3 +1,14 @@
+2018-07-19 Simon Fraser <[email protected]>
+
+ Remove completed animations from GraphicsLayer, thus avoiding excessive backing store allocation
+ https://bugs.webkit.org/show_bug.cgi?id=187844
+ rdar://problem/40387294
+
+ Reviewed by Dean Jackson.
+
+ * compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt: Added.
+ * compositing/backing/backing-store-attachment-fill-forwards-animation.html: Added.
+
2018-07-20 Ryosuke Niwa <[email protected]>
Picking a color from the color panel for typing attributes needs to inverse transform through color-filter
Added: trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt (0 => 234073)
--- trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt (rev 0)
+++ trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt 2018-07-20 23:33:39 UTC (rev 234073)
@@ -0,0 +1,25 @@
+An element with a finished fill-forwards animation should not trigger backing store outside the viewport.
+
+(GraphicsLayer
+ (anchor 0.00 0.00)
+ (bounds 785.00 1110.00)
+ (backingStoreAttached 1)
+ (children 1
+ (GraphicsLayer
+ (bounds 785.00 1110.00)
+ (contentsOpaque 1)
+ (backingStoreAttached 1)
+ (children 1
+ (GraphicsLayer
+ (position 18.00 1010.00)
+ (bounds 100.00 100.00)
+ (contentsOpaque 1)
+ (drawsContent 1)
+ (backingStoreAttached 0)
+ (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [100.00 0.00 0.00 1.00])
+ )
+ )
+ )
+ )
+)
+Some text here to force backing store.
Added: trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation.html (0 => 234073)
--- trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation.html (rev 0)
+++ trunk/LayoutTests/compositing/backing/backing-store-attachment-fill-forwards-animation.html 2018-07-20 23:33:39 UTC (rev 234073)
@@ -0,0 +1,67 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+ @keyframes move {
+ from { transform: translate3d(0, 0, 0); }
+ to { transform: translate3d(100px, 0, 0); }
+ }
+
+ #box {
+ position: absolute;
+ top: 1000px;
+ width: 100px;
+ height: 100px;
+ background-color: silver;
+ margin: 10px;
+ }
+
+ #box.animating {
+ animation: move linear 0.1s forwards;
+ }
+
+</style>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function dumpLayerTree()
+{
+ if (!window.internals)
+ return;
+
+ var out = document.getElementById('out');
+ out.innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
+}
+
+function dumpLayersSoon()
+{
+ setTimeout(function() {
+ dumpLayerTree();
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 0);
+}
+
+function runTest()
+{
+ let box = document.getElementById('box');
+ box.addEventListener('animationend', dumpLayersSoon, false);
+ box.classList.add('animating');
+}
+
+window.addEventListener('load', runTest, false);
+
+</script>
+</head>
+<body>
+<p>An element with a finished fill-forwards animation should not trigger backing store outside the viewport.</p>
+<pre id="out"></pre>
+<div id="box">
+ Some text here to force backing store.
+</div>
+
+</body>
+</html>
Added: trunk/LayoutTests/platform/ios/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt (0 => 234073)
--- trunk/LayoutTests/platform/ios/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/ios/compositing/backing/backing-store-attachment-fill-forwards-animation-expected.txt 2018-07-20 23:33:39 UTC (rev 234073)
@@ -0,0 +1,25 @@
+An element with a finished fill-forwards animation should not trigger backing store outside the viewport.
+
+(GraphicsLayer
+ (anchor 0.00 0.00)
+ (bounds 800.00 1110.00)
+ (backingStoreAttached 1)
+ (children 1
+ (GraphicsLayer
+ (bounds 800.00 1110.00)
+ (contentsOpaque 1)
+ (backingStoreAttached 1)
+ (children 1
+ (GraphicsLayer
+ (position 18.00 1010.00)
+ (bounds 100.00 100.00)
+ (contentsOpaque 1)
+ (drawsContent 1)
+ (backingStoreAttached 0)
+ (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [100.00 0.00 0.00 1.00])
+ )
+ )
+ )
+ )
+)
+Some text here to force backing store.
Modified: trunk/Source/WebCore/ChangeLog (234072 => 234073)
--- trunk/Source/WebCore/ChangeLog 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/ChangeLog 2018-07-20 23:33:39 UTC (rev 234073)
@@ -1,3 +1,43 @@
+2018-07-19 Simon Fraser <[email protected]>
+
+ Remove completed animations from GraphicsLayer, thus avoiding excessive backing store allocation
+ https://bugs.webkit.org/show_bug.cgi?id=187844
+ rdar://problem/40387294
+
+ Reviewed by Dean Jackson.
+
+ A keyframe animation which animates 3D transforms, and is fill-forwards, currently
+ leaves the GraphicsLayer in a state where it has a "running" animation. However, the
+ logic that computes animation extent in RenderLayerBacking::updateGeometry() only does
+ so for running or paused animations. GraphicsLayer then thinks that it has an active
+ transform animation with unknown extent, and refuses to detach its backing store.
+
+ This triggers excessive layer creation on some sites (e.g. https://www.kqed.org).
+
+ Fix by always removing animations from the GraphicsLayer when they finish, whether
+ or not they fill forwards. This is done by having KeyframeAnimation::onAnimationEnd()
+ always call endAnimation().
+
+ This change only fixes the non-Web Animation code path. webkit.org/b/187845 exists
+ to fix the other code path.
+
+ Also improve some logging that would have revealed this problem sooner.
+
+ Test: compositing/backing/backing-store-attachment-fill-forwards-animation.html
+
+ * page/animation/AnimationBase.h:
+ (WebCore::AnimationBase::endAnimation):
+ * page/animation/ImplicitAnimation.cpp:
+ (WebCore::ImplicitAnimation::endAnimation):
+ * page/animation/ImplicitAnimation.h:
+ * page/animation/KeyframeAnimation.cpp:
+ (WebCore::KeyframeAnimation::endAnimation):
+ (WebCore::KeyframeAnimation::onAnimationEnd):
+ * page/animation/KeyframeAnimation.h:
+ * platform/graphics/ca/GraphicsLayerCA.cpp:
+ (WebCore::GraphicsLayerCA::addAnimation):
+ (WebCore::GraphicsLayerCA::updateCoverage):
+
2018-07-20 Ryosuke Niwa <[email protected]>
Picking a color from the color panel for typing attributes needs to inverse transform through color-filter
Modified: trunk/Source/WebCore/page/animation/AnimationBase.h (234072 => 234073)
--- trunk/Source/WebCore/page/animation/AnimationBase.h 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/page/animation/AnimationBase.h 2018-07-20 23:33:39 UTC (rev 234073)
@@ -223,7 +223,7 @@
virtual bool startAnimation(double /*timeOffset*/) { return false; }
// timeOffset is the time at which the animation is being paused.
virtual void pauseAnimation(double /*timeOffset*/) { }
- virtual void endAnimation() { }
+ virtual void endAnimation(bool /*fillingForwards*/ = false) { }
virtual const RenderStyle& unanimatedStyle() const = 0;
Modified: trunk/Source/WebCore/page/animation/ImplicitAnimation.cpp (234072 => 234073)
--- trunk/Source/WebCore/page/animation/ImplicitAnimation.cpp 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/page/animation/ImplicitAnimation.cpp 2018-07-20 23:33:39 UTC (rev 234073)
@@ -147,7 +147,7 @@
setNeedsStyleRecalc(element());
}
-void ImplicitAnimation::endAnimation()
+void ImplicitAnimation::endAnimation(bool)
{
if (auto* renderer = compositedRenderer())
renderer->transitionFinished(m_animatingProperty);
Modified: trunk/Source/WebCore/page/animation/ImplicitAnimation.h (234072 => 234073)
--- trunk/Source/WebCore/page/animation/ImplicitAnimation.h 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/page/animation/ImplicitAnimation.h 2018-07-20 23:33:39 UTC (rev 234073)
@@ -51,7 +51,7 @@
void onAnimationEnd(double elapsedTime) override;
bool startAnimation(double timeOffset) override;
void pauseAnimation(double timeOffset) override;
- void endAnimation() override;
+ void endAnimation(bool fillingForwards = false) override;
bool animate(CompositeAnimation&, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle);
void getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle) override;
Modified: trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp (234072 => 234073)
--- trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp 2018-07-20 23:33:39 UTC (rev 234073)
@@ -297,7 +297,7 @@
setNeedsStyleRecalc(element());
}
-void KeyframeAnimation::endAnimation()
+void KeyframeAnimation::endAnimation(bool fillingForwards)
{
if (!element())
return;
@@ -306,7 +306,7 @@
renderer->animationFinished(m_keyframes.animationName());
// Restore the original (unanimated) style
- if (!paused())
+ if (!fillingForwards && !paused())
setNeedsStyleRecalc(element());
}
@@ -328,10 +328,7 @@
void KeyframeAnimation::onAnimationEnd(double elapsedTime)
{
sendAnimationEvent(eventNames().animationendEvent, elapsedTime);
- // End the animation if we don't fill forwards. Forward filling
- // animations are ended properly in the class destructor.
- if (!m_animation->fillsForwards())
- endAnimation();
+ endAnimation(m_animation->fillsForwards());
}
bool KeyframeAnimation::sendAnimationEvent(const AtomicString& eventType, double elapsedTime)
Modified: trunk/Source/WebCore/page/animation/KeyframeAnimation.h (234072 => 234073)
--- trunk/Source/WebCore/page/animation/KeyframeAnimation.h 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/page/animation/KeyframeAnimation.h 2018-07-20 23:33:39 UTC (rev 234073)
@@ -69,7 +69,7 @@
void onAnimationEnd(double elapsedTime) override;
bool startAnimation(double timeOffset) override;
void pauseAnimation(double timeOffset) override;
- void endAnimation() override;
+ void endAnimation(bool fillingForwards = false) override;
void overrideAnimations() override;
void resumeOverriddenAnimations() override;
Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (234072 => 234073)
--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2018-07-20 22:55:36 UTC (rev 234072)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2018-07-20 23:33:39 UTC (rev 234073)
@@ -1022,7 +1022,7 @@
bool GraphicsLayerCA::addAnimation(const KeyframeValueList& valueList, const FloatSize& boxSize, const Animation* anim, const String& animationName, double timeOffset)
{
- LOG(Animations, "GraphicsLayerCA %p addAnimation %s (can be accelerated %d)", this, animationName.utf8().data(), animationCanBeAccelerated(valueList, anim));
+ LOG(Animations, "GraphicsLayerCA %p %llu addAnimation %s (can be accelerated %d)", this, primaryLayerID(), animationName.utf8().data(), animationCanBeAccelerated(valueList, anim));
ASSERT(!animationName.isEmpty());
@@ -2351,7 +2351,20 @@
|| commitState.ancestorWithTransformAnimationIntersectsCoverageRect // FIXME: Compute backing exactly for descendants of animating layers.
|| (isRunningTransformAnimation() && !animationExtent()); // Create backing if we don't know the animation extent.
- LOG_WITH_STREAM(Compositing, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " setBackingStoreAttached: " << requiresBacking);
+#if !LOG_DISABLED
+ if (requiresBacking) {
+ auto reasonForBacking = [&]() -> const char* {
+ if (m_intersectsCoverageRect)
+ return "intersectsCoverageRect";
+
+ if (commitState.ancestorWithTransformAnimationIntersectsCoverageRect)
+ return "ancestor with transform";
+
+ return "has transform animation with unknown extent";
+ };
+ LOG_WITH_STREAM(Compositing, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " setBackingStoreAttached: " << requiresBacking << " (" << reasonForBacking() << ")");
+ }
+#endif
m_layer->setBackingStoreAttached(requiresBacking);
if (m_layerClones) {
