258351 2023-06-21 04:48:59 -0700 [LBSE] Errorous/unnecessary repainting when viewBox is used on <svg> elements 2023-11-22 04:57:57 -0800 1 1 1 Unclassified WebKit SVG WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 90738 1 zimmermann zimmermann rwlbuis sabouhallawa webkit-bug-importer zimmermann oldest_to_newest 1962772 0 466775 zimmermann 2023-06-21 04:48:59 -0700 Created attachment 466775 Slow SVG This used to work in the downstream PoC - but since the implementations are way different nowadays, we need to re-investigate. Currently when viewBox is enabled, the accelerated transform animation performance is artifically lower, since there are unnecessary repaints triggered, but not when viewBoxis omitted -- one sees only compositing steps, no painting (no increase of layer repaint counters, etc) Testcase (fast w/o viewBox, try enabling viewBox). <?xml version="1.0" encoding="utf-8"?> <svg xmlns="http://www.w3.org/2000/svg" fooviewBox="0 0 800 600" width="100%" height="100%"> <style type="text/css"> path#demoScene1 { transform-origin: 50% 50%; animation: rotateAroundCenterAnimationFrames 2s linear 0s infinite; } @keyframes rotateAroundCenterAnimationFrames { 0% { transform: rotate(0deg); } 50% { transform: rotate(180deg); } 100% { transform: rotate(359deg); } } </style> <path id="demoScene1" d="m233.58,47.875c0,7.476-2.158,8.743-8.992,8.743-6.972,0-9.123-1.268-9.123-8.743,0-6.714,2.151-7.723,9.123-7.723,6.834,0.001,8.992,1.01,8.992,7.723zm-1.27,65.47c0,9.976,4.809,15.219,4.809,15.219s-6.383,5.037-11.303,5.037c-6.464,0-9.215-11.614-9.215-16.417v-50.182h15.709v46.343z"/> </svg> 1964003 1 webkit-bug-importer 2023-06-28 04:49:16 -0700 <rdar://problem/111445734> 1978377 2 zimmermann 2023-09-19 12:16:45 -0700 When showing the compositing borders in WebInspector, a spurious layer is visible, that gets frequently repainted according to the increasing repaint count. Breaking on GraphicLayerCA::setNeedsDisplay reveals: * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 7.2 * frame #0: 0x0000000284c8aa8c WebCore`WebCore::GraphicsLayerCA::setNeedsDisplay(this=0x00000001310fc560) at GraphicsLayerCA.cpp:902:10 frame #1: 0x0000000284b8a0f0 WebCore`WebCore::GraphicsLayer::setSize(this=0x00000001310fc560, size=0x000000016bccd2bc) at GraphicsLayer.cpp:550:9 frame #2: 0x0000000284c89810 WebCore`WebCore::GraphicsLayerCA::setSize(this=0x00000001310fc560, size=0x000000016bccd2bc) at GraphicsLayerCA.cpp:618:20 frame #3: 0x00000002852bc378 WebCore`WebCore::RenderLayerBacking::updateGeometry(this=0x00000001310b5ff0, compositedAncestor=0x0000000144003eb0) at RenderLayerBacking.cpp:1435:22 frame #4: 0x00000002852cdd28 WebCore`WebCore::RenderLayerCompositor::updateBackingAndHierarchy(this=0x00000001311079c0, layer=0x00000001440018c0, childLayersOfEnclosingLayer=0x000000016bccd808, traversalState=0x000000016bccd8a0, scrollingTreeState=0x000000016bccd8c0, updateLevel=(m_storage = 2)) at RenderLayerCompositor.cpp:1425:27 frame #5: 0x00000002852ce1c0 WebCore`WebCore::RenderLayerCompositor::updateBackingAndHierarchy(this=0x00000001311079c0, layer=0x0000000144003eb0, childLayersOfEnclosingLayer=0x000000016bccda68, traversalState=0x000000016bccdb00, scrollingTreeState=0x000000016bccdb20, updateLevel=(m_storage = 0)) at RenderLayerCompositor.cpp:1490:13 frame #6: 0x00000002852ce1c0 WebCore`WebCore::RenderLayerCompositor::updateBackingAndHierarchy(this=0x00000001311079c0, layer=0x0000000144002940, childLayersOfEnclosingLayer=0x000000016bccdd38, traversalState=0x000000016bccdd48, scrollingTreeState=0x000000016bccdd90, updateLevel=(m_storage = 0)) at RenderLayerCompositor.cpp:1490:13 frame #7: 0x00000002852cb458 WebCore`WebCore::RenderLayerCompositor::updateCompositingLayers(this=0x00000001311079c0, updateType=AfterStyleChange, updateRoot=0x0000000144002940) at RenderLayerCompositor.cpp:971:9 frame #8: 0x00000002852ca7fc WebCore`WebCore::RenderLayerCompositor::didRecalcStyleWithNoPendingLayout(this=0x00000001311079c0) at RenderLayerCompositor.cpp:607:12 frame #9: 0x00000002846f59e8 WebCore`WebCore::LocalFrameView::updateCompositingLayersAfterStyleChange(this=0x0000000143140600) at LocalFrameView.cpp:811:39 frame #10: 0x00000002837e4450 WebCore`WebCore::Document::resolveStyle(this=0x0000000143142400, type=Normal) at Document.cpp:2205:46 frame #11: 0x00000002837e5034 WebCore`WebCore::Document::updateStyleIfNeeded(this=0x0000000143142400) at Document.cpp:2303:5 frame #12: 0x000000028470b110 WebCore`WebCore::LocalFrameView::updateLayoutAndStyleIfNeededRecursive(this=0x0000000143140600) at LocalFrameView.cpp:4970:44 frame #13: 0x000000028476d238 WebCore`WebCore::Page::layoutIfNeeded(this=0x000000013104b700) at Page.cpp:1678:15 frame #14: 0x000000028476df70 WebCore`WebCore::Page::updateRendering(this=0x000000013104b700) at Page.cpp:1830:5 frame #15: 0x000000011d0d20e4 WebKit`WebKit::WebPage::updateRendering(this=0x000000014c010c08) at WebPage.cpp:4736:13 frame #16: 0x000000011c258010 WebKit`WebKit::TiledCoreAnimationDrawingArea::updateRendering(this=0x0000000131094b00, flushType=Normal) at TiledCoreAnimationDrawingArea.mm:361:18 frame #17: 0x000000011c25c804 WebKit`WebKit::TiledCoreAnimationDrawingArea::renderingUpdateRunLoopCallback(this=0x0000000131094b00) at TiledCoreAnimationDrawingArea.mm:871:5 frame #18: 0x000000011c25e55c WebKit`WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(this=0x00000001310071b8)::$_0::operator()() const at TiledCoreAnimationDrawingArea.mm:90:15 frame #19: 0x000000011c25e508 WebKit`WTF::Detail::CallableWrapper<WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(WebKit::WebPage&, WebKit::WebPageCreationParameters const&)::$_0, void>::call(this=0x00000001310071b0) at Function.h:53:39 frame #20: 0x0000000282ac5314 WebCore`WTF::Function<void ()>::operator(this=0x00000001310f1340)() const at Function.h:82:35 frame #21: 0x0000000284986ca4 WebCore`WebCore::RunLoopObserver::runLoopObserverFired(this=0x00000001310f1340) at RunLoopObserver.cpp:41:5 Summary: Page::updateRendering() updates the document style w/o performing relayout, leading to a LocalFrameView::updateCompositingLayersAfterStyleChange() call. That in turn updates compositing layers (in our case an accelerated transform animation is running which has to be applied). The geometry of the backing store of a certain layer is updated, which leads to a GraphicsLayer::setNeedsDisplay() call. I checked frame #3 0x00000002852bc378 WebCore`WebCore::RenderLayerBacking::updateGeometry(this=0x00000001310b5ff0, compositedAncestor=0x0000000144003eb0) at RenderLayerBacking.cpp:1435:22 the &m_owningLayer=0x1440018c0. Layer tree: layer 0x144002940 scrollableArea 0x13104f480 at (0,0) size 1013x576 (composited [root], bounds=at (0,0) size 1013x576, drawsContent=1, paints into ancestor=0) RenderView 0x144002510 at (0,0) size 1013x576 positive z-order list (1) layer 0x144003eb0 scrollableArea 0x13104fb40 at (0,0) size 1013x576 (composited [transform with composited descendants], bounds=at (0,0) size 1013x576, drawsContent=1, paints into ancestor=0) RenderSVGRoot 0x144003d30 {svg} at (0,0) size 1013x576 positive z-order list (1) *** layer 0x1440018c0 at (0,0) size 1013x576 (composited [transform with composited descendants], bounds=at (716.42,192.27) size 90.34x55.89, drawsContent=1, paints into ancestor=0) RenderSVGViewportContainer 0x1440041e0 at (0,0) size 1013x576 positive z-order list (1) layer 0x1440009a0 at (215.45,40.14) size 23x94 (composited [animation], bounds=at (0,0) size 21.67x93.47, drawsContent=1, paints into ancestor=0) RenderSVGPath 0x1440040b0 {path} at (215.45,40.14) size 21.67x93.47 [fill={[type=SOLID] [color=#000000]}] [data="M 233.58 47.875 C 233.58 55.351 231.422 56.618 224.588 56.618 C 217.616 56.618 215.465 55.35 215.465 47.875 C 215.465 41.161 217.616 40.152 224.588 40.152 C 231.422 40.153 233.58 41.162 233.58 47.875 Z M 232.31 113.345 C 232.31 123.321 237.119 128.564 237.119 128.564 C 237.119 128.564 230.736 133.601 225.816 133.601 C 219.352 133.601 216.601 121.987 216.601 117.184 L 216.601 67.002 L 232.31 67.002 L 232.31 113.345 Z"] id="demoScene1" The layer whose backing store is updated is the one associated with the anonymous RenderSVGViewportContainer (that encloses the whole SVG render tree, and is the only direct child of RenderSVGRoot). 1978383 3 zimmermann 2023-09-19 12:28:46 -0700 GraphicsLayerCA::setNeedsDisplay() is responsible for the repaints: void GraphicsLayerCA::setNeedsDisplay() { if (!drawsContent()) return; m_needsFullRepaint = true; m_dirtyRects.clear(); noteLayerPropertyChanged(DirtyRectsChanged); addRepaintRect(FloatRect(FloatPoint(), m_size)); } But only if drawsContent() = true. So how does a GraphicsLayerCA obtain the 'drawsContent()' flag? <snippet> void RenderLayerBacking::updateDrawsContent(PaintedContentsInfo& contentsInfo) { ... bool hasPaintedContent = containsPaintedContent(contentsInfo); // FIXME: we could refine this to only allocate backing for one of these layers if possible. m_graphicsLayer->setDrawsContent(hasPaintedContent); if (m_foregroundLayer) m_foregroundLayer->setDrawsContent(hasPaintedContent); ... } </snippet> RenderLayerBacking::containsPaintedContent() is relevant: <snippet> bool RenderLayerBacking::containsPaintedContent(PaintedContentsInfo& contentsInfo) const { if (contentsInfo.isSimpleContainer() || paintsIntoWindow() || paintsIntoCompositedAncestor() || m_artificiallyInflatedBounds || m_owningLayer.isReflection()) return false; .. </snippet> So a "simple container" should not even trigger repaints. Does RenderSVGViewportContainer qualify as "simple container" ? <snippet> bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo& contentsInfo) const { if (m_owningLayer.isRenderViewLayer()) return false; if (hasBackingSharingLayers()) return false; if (renderer().isRenderReplaced() && !isCompositedPlugin(renderer())) return false; if (renderer().isTextControl()) return false; if (contentsInfo.paintsBoxDecorations() || contentsInfo.paintsContent()) return false; ... </snippet> "contentsInfo.paintsContent()" returns true in our testcase, causing the observed problem. To shorten the trace a bit: RenderLayerBacking::paintsContent() contains the logic that determines if "contentsInfo.paintsContent()" returns true or false. And here's the issue: <snippet> bool RenderLayerBacking::paintsContent(RenderLayer::PaintedContentRequest& request) const { m_owningLayer.updateDescendantDependentFlags(); bool paintsContent = false; if (m_owningLayer.hasVisibleContent() && m_owningLayer.hasNonEmptyChildRenderers(request)) paintsContent = true; if (request.isSatisfied()) return paintsContent; if (isPaintDestinationForDescendantLayers(request)) paintsContent = true; if (request.isSatisfied()) return paintsContent; #if ENABLE(LAYER_BASED_SVG_ENGINE) if (is<RenderSVGModelObject>(m_owningLayer.renderer())) { // FIXME: [LBSE] Eventually cache if we're part of a RenderSVGHiddenContainer subtree to avoid tree walks. // FIXME: [LBSE] Eventually refine the logic to end up with a narrower set of conditions (webkit.org/b/243417). paintsContent = m_owningLayer.hasVisibleContent() && !lineageOfType<RenderSVGHiddenContainer>(m_owningLayer.renderer()).first(); request.setHasPaintedContent(); } if (request.isSatisfied()) return paintsContent; #endif </snippet> m_owningLayer.hasVisibleContent() returns true for the layer associated with the RenderSVGViewportContainer. The repaint problem observed here, is a regression of "[LBSE] Enable compositing support for SVG elements" (https://commits.webkit.org/253054@main), which added the logic that sets 'request.setHasPaintedContent()' and 'paintsContent = true'. The problem is understood now, checking the landscape of solutions for a proper choice :-) 1979673 4 zimmermann 2023-09-24 14:59:40 -0700 Pull request: https://github.com/WebKit/WebKit/pull/18136 1983430 5 ews-feeder 2023-10-07 00:32:24 -0700 Committed 269034@main (af30a9bb6eef): <https://commits.webkit.org/269034@main> Reviewed commits have been landed. Closing PR #18136 and removing active labels. 466775 2023-06-21 04:48:59 -0700 2023-06-21 04:48:59 -0700 Slow SVG lbse-demo-2023-jun-dbg.svg image/svg+xml 759 zimmermann PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4KPHN2ZyB4bWxucz0iaHR0cDov L3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCA4MDAgNjAwIiB3aWR0aD0iMTAwJSIg aGVpZ2h0PSIxMDAlIj4KPHN0eWxlIHR5cGU9InRleHQvY3NzIj4KcGF0aCNkZW1vU2NlbmUxIHsK ICAgIHRyYW5zZm9ybS1vcmlnaW46IDUwJSA1MCU7CiAgICBhbmltYXRpb246IHJvdGF0ZUFyb3Vu ZENlbnRlckFuaW1hdGlvbkZyYW1lcyAycyBsaW5lYXIgMHMgaW5maW5pdGU7Cn0KCkBrZXlmcmFt ZXMgcm90YXRlQXJvdW5kQ2VudGVyQW5pbWF0aW9uRnJhbWVzIHsKICAgIDAlIHsgdHJhbnNmb3Jt OiByb3RhdGUoMGRlZyk7IH0KICAgNTAlIHsgdHJhbnNmb3JtOiByb3RhdGUoMTgwZGVnKTsgfQog IDEwMCUgeyB0cmFuc2Zvcm06IHJvdGF0ZSgzNTlkZWcpOyB9Cn0KPC9zdHlsZT4KPHBhdGggaWQ9 ImRlbW9TY2VuZTEiIGQ9Im0yMzMuNTgsNDcuODc1YzAsNy40NzYtMi4xNTgsOC43NDMtOC45OTIs OC43NDMtNi45NzIsMC05LjEyMy0xLjI2OC05LjEyMy04Ljc0MywwLTYuNzE0LDIuMTUxLTcuNzIz LDkuMTIzLTcuNzIzLDYuODM0LDAuMDAxLDguOTkyLDEuMDEsOC45OTIsNy43MjN6bS0xLjI3LDY1 LjQ3YzAsOS45NzYsNC44MDksMTUuMjE5LDQuODA5LDE1LjIxOXMtNi4zODMsNS4wMzctMTEuMzAz LDUuMDM3Yy02LjQ2NCwwLTkuMjE1LTExLjYxNC05LjIxNS0xNi40MTd2LTUwLjE4MmgxNS43MDl2 NDYuMzQzeiIvPgo8L3N2Zz4K