Bug 102313

Created attachment 174312 [details] Patch

Chromium compositor does not care animation and 3d transform specially also. occlusionTracker.occluded() checks the rect is visible. In the occluded() functions, the rect is mapped to the given transform of the layer, regardless of 2d transform or 3d transform. bool CCLayerTreeHostImpl::calculateRenderPasses(FrameData& frame) { .... if (occlusionTracker.occluded(*it, it->visibleContentRect(), &hasOcclusionFromOutsideTargetSurface)) appendQuadsData.hadOcclusionFromOutsideTargetSurface |= hasOcclusionFromOutsideTargetSurface; else { .... targetRenderPass->appendQuadsForLayer(*it, &occlusionTracker, appendQuadsData); } .... } I think we should remove offscreen tiles also.

Comment on attachment 174312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174312&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:755 > + return tiledBackingStoreVisibleRect().intersects(tiledBackingStoreContentsRect()); Could you feedback here? I think we can unlock animations when this layer is not visible even if we need to sync something. Here is related to this patch because of changing tiledBackingStoreVisibleRect() > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:187 > bool selfOrAncestorHaveNonAffineTransforms(); I think we need to rename from selfOrAncestorHaveNonAffineTransforms to selfOrAncestorHaveTransformAnimationsOrNonAffineTransforms or hasComplexTransforms. Could you suggest?

I tested in http://www.dorothybrowser.com/test/webkitTest/css3/snowstack.html , which is snowstack variant to remove directly composited images. After this patch, Coordinated Graphics has less than 60 images textures, even if we receive a lot of images using AJAX. I have a plan to limit directly composited images also in Bug 101023.

Created attachment 174554 [details] Patch

rebase to upstream

Comment on attachment 174554 [details] Patch Have you considered using GraphicsLayerclient::getCurrentTransform which considers current animation status, rather than running our own GraphicsLayerAnimation?

Comment on attachment 174554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174554&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:663 > + } I am skeptical here, also. When this code exists, about 340 tiles are kept in snowstack. When this code is removed, about 250 tiles are kept in snowstack. Without this code, I think we keep so many tiles. I'll investigate why? And I think we need deletion timer in TiledBackingStore like Bug 102449.

Created attachment 174597 [details] Patch

Comment on attachment 174597 [details] Patch Attachment 174597 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14872031 New failing tests: platform/chromium/virtual/threaded/compositing/webgl/webgl-background-color.html

Created attachment 174634 [details] Patch

(In reply to comment #10) > (From update of attachment 174597 [details]) > Attachment 174597 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14872031 > > New failing tests: > platform/chromium/virtual/threaded/compositing/webgl/webgl-background-color.html I think chromium ews fail does not relate to this bug, so I post again.

Comment on attachment 174634 [details] Patch Who calls setShouldUpdateVisibleRect during the animation?

Created attachment 174890 [details] Patch

(In reply to comment #13) > (From update of attachment 174634 [details]) > Who calls setShouldUpdateVisibleRect during the animation? Nice review! Now, CoordinatedGraphicsLayer::computeTransformedVisibleRect() calls.

Created attachment 174892 [details] Patch

Comment on attachment 174892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174892&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:810 > + setShouldUpdateVisibleRect(); Here is changed. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:818 > + client()->getCurrentTransform(this, currentTransform); Here is changed.

Comment on attachment 174892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174892&action=review >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:810 >> + setShouldUpdateVisibleRect(); > > Here is changed. Instead of calling setShouldUpdateVisibleRect again, we should instead not return early if we have animations, something like if (!m_shouldUpdateVisibleRect && !haveTransformAnimations) return;

(In reply to comment #18) > (From update of attachment 174892 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174892&action=review > > >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:810 > >> + setShouldUpdateVisibleRect(); > > > > Here is changed. > > Instead of calling setShouldUpdateVisibleRect again, we should instead not return early if we have animations, something like > if (!m_shouldUpdateVisibleRect && !haveTransformAnimations) > return; setShouldUpdateVisibleRect() reculsively changes m_shouldUpdateVisibleRect of all children. void CoordinatedGraphicsLayer::setShouldUpdateVisibleRect() { m_shouldUpdateVisibleRect = true; for (size_t i = 0; i < children().size(); ++i) toCoordinatedGraphicsLayer(children()[i])->setShouldUpdateVisibleRect(); if (replicaLayer()) toCoordinatedGraphicsLayer(replicaLayer())->setShouldUpdateVisibleRect(); } So I called setShouldUpdateVisibleRect().

Comment on attachment 174892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174892&action=review >>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:810 >>>> + setShouldUpdateVisibleRect(); >>> >>> Here is changed. >> >> Instead of calling setShouldUpdateVisibleRect again, we should instead not return early if we have animations, something like >> if (!m_shouldUpdateVisibleRect && !haveTransformAnimations) >> return; > > setShouldUpdateVisibleRect() reculsively changes m_shouldUpdateVisibleRect of all children. > void CoordinatedGraphicsLayer::setShouldUpdateVisibleRect() > { > m_shouldUpdateVisibleRect = true; > for (size_t i = 0; i < children().size(); ++i) > toCoordinatedGraphicsLayer(children()[i])->setShouldUpdateVisibleRect(); > if (replicaLayer()) > toCoordinatedGraphicsLayer(replicaLayer())->setShouldUpdateVisibleRect(); > } > > So I called setShouldUpdateVisibleRect(). How about then if selfOrAncestorHasActiveTransformAnimation

Created attachment 175059 [details] Patch

(In reply to comment #20) > (From update of attachment 174892 [details]) > How about then if selfOrAncestorHasActiveTransformAnimation Good idea. I added this method.

Comment on attachment 175059 [details] Patch Clearing flags on attachment: 175059 Committed r135212: <http://trac.webkit.org/changeset/135212>

All reviewed patches have been landed. Closing bug.

fast/multicol/span/positioned-child-not-removed-crash.html started to crash on EFL WK2 Bots both Debug and Release after this patch. Could this be related? crash log for WebProcess (pid <unknown>): STDOUT: <empty> STDERR: 1 0x7f6f139e3ab7 STDERR: 2 0x7f6f161484a0 STDERR: 3 0x7f6f12f3813b WebCore::TiledBackingStore::adjustForContentsRect(WebCore::IntRect&) const STDERR: 4 0x7f6f12f38459 WebCore::TiledBackingStore::computeCoverAndKeepRect(WebCore::IntRect const&, WebCore::IntRect&, WebCore::IntRect&) const STDERR: 5 0x7f6f12f37b30 WebCore::TiledBackingStore::createTiles() STDERR: 6 0x7f6f12f36ac0 WebCore::TiledBackingStore::coverWithTilesIfNeeded(WebCore::FloatPoint const&) STDERR: 7 0x7f6f12f3744c WebCore::TiledBackingStore::commitScaleChange() STDERR: 8 0x7f6f12f373e8 WebCore::TiledBackingStore::setContentsScale(float) STDERR: 9 0x7f6f16def1e1 WebCore::CoordinatedGraphicsLayer::createBackingStore() STDERR: 10 0x7f6f16def7de WebCore::CoordinatedGraphicsLayer::updateContentBuffers() STDERR: 11 0x7f6f16deed08 WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() STDERR: 12 0x7f6f16dee42a WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 13 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 14 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 15 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 16 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 17 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) STDERR: 18 0x7f6f1311e9d8 WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool) STDERR: 19 0x7f6f12e20719 WebCore::FrameView::flushCompositingStateForThisFrame(WebCore::Frame*) STDERR: 20 0x7f6f12e20a53 WebCore::FrameView::flushCompositingStateIncludingSubframes() STDERR: 21 0x7f6f16df5958 WebKit::LayerTreeCoordinator::flushPendingLayerChanges() STDERR: 22 0x7f6f16df5299 WebKit::LayerTreeCoordinator::forceRepaint() STDERR: 23 0x7f6f16db8a02 WebKit::DrawingAreaImpl::forceRepaint() STDERR: 24 0x7f6f16dd8743 WebKit::WebPage::forceRepaintWithoutCallback() STDERR: 25 0x7f6f16d3e483 WKBundlePageForceRepaint STDERR: 26 0x7f6ec1820d8f WTR::InjectedBundlePage::dump() STDERR: 27 0x7f6ec1825799 WTR::InjectedBundlePage::frameDidChangeLocation(OpaqueWKBundleFrame const*, bool) STDERR: 28 0x7f6ec1821295 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundleFrame const*) STDERR: 29 0x7f6ec181f257 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, void const**, void const*) STDERR: 30 0x7f6f16d34167 WebKit::InjectedBundlePageLoaderClient::didFinishLoadForFrame(WebKit::WebPage*, WebKit::WebFrame*, WTF::RefPtr<WebKit::APIObject>&) STDERR: 31 0x7f6f16d9ff54 WebKit::WebFrameLoaderClient::dispatchDidFinishLoad() STDERR: LEAK: 1 WebPage STDERR: LEAK: 1 WebFrame STDERR: LEAK: 18 RenderObject STDERR: LEAK: 1 BidiRun STDERR: LEAK: 1 Page STDERR: LEAK: 1 Frame STDERR: LEAK: 501 CachedResource STDERR: LEAK: 49 WebCoreNode

Yes, looks related. DongSung, could you take a look? (In reply to comment #25) > fast/multicol/span/positioned-child-not-removed-crash.html started to crash on EFL WK2 Bots both Debug and Release after this patch. Could this be related? > > crash log for WebProcess (pid <unknown>): > STDOUT: <empty> > STDERR: 1 0x7f6f139e3ab7 > STDERR: 2 0x7f6f161484a0 > STDERR: 3 0x7f6f12f3813b WebCore::TiledBackingStore::adjustForContentsRect(WebCore::IntRect&) const > STDERR: 4 0x7f6f12f38459 WebCore::TiledBackingStore::computeCoverAndKeepRect(WebCore::IntRect const&, WebCore::IntRect&, WebCore::IntRect&) const > STDERR: 5 0x7f6f12f37b30 WebCore::TiledBackingStore::createTiles() > STDERR: 6 0x7f6f12f36ac0 WebCore::TiledBackingStore::coverWithTilesIfNeeded(WebCore::FloatPoint const&) > STDERR: 7 0x7f6f12f3744c WebCore::TiledBackingStore::commitScaleChange() > STDERR: 8 0x7f6f12f373e8 WebCore::TiledBackingStore::setContentsScale(float) > STDERR: 9 0x7f6f16def1e1 WebCore::CoordinatedGraphicsLayer::createBackingStore() > STDERR: 10 0x7f6f16def7de WebCore::CoordinatedGraphicsLayer::updateContentBuffers() > STDERR: 11 0x7f6f16deed08 WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() > STDERR: 12 0x7f6f16dee42a WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 13 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 14 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 15 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 16 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 17 0x7f6f16dee46b WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&) > STDERR: 18 0x7f6f1311e9d8 WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool) > STDERR: 19 0x7f6f12e20719 WebCore::FrameView::flushCompositingStateForThisFrame(WebCore::Frame*) > STDERR: 20 0x7f6f12e20a53 WebCore::FrameView::flushCompositingStateIncludingSubframes() > STDERR: 21 0x7f6f16df5958 WebKit::LayerTreeCoordinator::flushPendingLayerChanges() > STDERR: 22 0x7f6f16df5299 WebKit::LayerTreeCoordinator::forceRepaint() > STDERR: 23 0x7f6f16db8a02 WebKit::DrawingAreaImpl::forceRepaint() > STDERR: 24 0x7f6f16dd8743 WebKit::WebPage::forceRepaintWithoutCallback() > STDERR: 25 0x7f6f16d3e483 WKBundlePageForceRepaint > STDERR: 26 0x7f6ec1820d8f WTR::InjectedBundlePage::dump() > STDERR: 27 0x7f6ec1825799 WTR::InjectedBundlePage::frameDidChangeLocation(OpaqueWKBundleFrame const*, bool) > STDERR: 28 0x7f6ec1821295 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundleFrame const*) > STDERR: 29 0x7f6ec181f257 WTR::InjectedBundlePage::didFinishLoadForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, void const**, void const*) > STDERR: 30 0x7f6f16d34167 WebKit::InjectedBundlePageLoaderClient::didFinishLoadForFrame(WebKit::WebPage*, WebKit::WebFrame*, WTF::RefPtr<WebKit::APIObject>&) > STDERR: 31 0x7f6f16d9ff54 WebKit::WebFrameLoaderClient::dispatchDidFinishLoad() > STDERR: LEAK: 1 WebPage > STDERR: LEAK: 1 WebFrame > STDERR: LEAK: 18 RenderObject > STDERR: LEAK: 1 BidiRun > STDERR: LEAK: 1 Page > STDERR: LEAK: 1 Frame > STDERR: LEAK: 501 CachedResource > STDERR: LEAK: 49 WebCoreNode

Reopening the bug as the regression needs to be fixed. I have marked the test as crashing on the EFL-WK2 port so it does not show up on the bots as a new failure in <http://trac.webkit.org/changeset/135289> for now. If the issue persists, I will need to roll it out.

(In reply to comment #26) > Yes, looks related. DongSung, could you take a look? > > (In reply to comment #25) > > fast/multicol/span/positioned-child-not-removed-crash.html started to crash on EFL WK2 Bots both Debug and Release after this patch. Could this be related? > > > > crash log for WebProcess (pid <unknown>): > > STDOUT: <empty> > > STDERR: 1 0x7f6f139e3ab7 > > STDERR: 2 0x7f6f161484a0 > > STDERR: 3 0x7f6f12f3813b WebCore::TiledBackingStore::adjustForContentsRect(WebCore::IntRect&) const > > STDERR: 4 0x7f6f12f38459 WebCore::TiledBackingStore::computeCoverAndKeepRect(WebCore::IntRect const&, WebCore::IntRect&, WebCore::IntRect&) const > > STDERR: 5 0x7f6f12f37b30 WebCore::TiledBackingStore::createTiles() > > STDERR: 6 0x7f6f12f36ac0 WebCore::TiledBackingStore::coverWithTilesIfNeeded(WebCore::FloatPoint const&) Yes, of course. I'll take a look. Sorry for this bug.

(In reply to comment #28) > (In reply to comment #26) > > Yes, looks related. DongSung, could you take a look? > Yes, of course. I'll take a look. Sorry for this bug. I posted the patch to fix in Bug 102891