Bug 102313

Summary: Coordinated Graphics: Remove tiles of a layer when they are off the viewport.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, laszlo.gombos, noam, ostap73, rakuco, tmpsantos, webkit.review.bot, yael, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102309, 102891    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dongseong Hwang 2012-11-14 18:19:08 PST
We need to remove tiles of the layer with the special properties: a transform animation and non affine transform. If a page has a lot of layers with a transform animation, we can encounter OOM. 
For example, we create tiles endlessly in http://www.satine.org/research/webkit/snowleopard/snowstack.html
Comment 1 Dongseong Hwang 2012-11-14 18:25:08 PST
Created attachment 174312 [details]
Patch
Comment 2 Dongseong Hwang 2012-11-14 18:29:14 PST
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 3 Dongseong Hwang 2012-11-14 18:34:15 PST
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?
Comment 4 Dongseong Hwang 2012-11-14 18:47:15 PST
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.
Comment 5 Dongseong Hwang 2012-11-15 16:31:58 PST
Created attachment 174554 [details]
Patch
Comment 6 Dongseong Hwang 2012-11-15 16:32:32 PST
rebase to upstream
Comment 7 Noam Rosenthal 2012-11-15 16:35:54 PST
Comment on attachment 174554 [details]
Patch

Have you considered using GraphicsLayerclient::getCurrentTransform which considers current animation status, rather than running our own GraphicsLayerAnimation?
Comment 8 Dongseong Hwang 2012-11-15 18:05:32 PST
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.
Comment 9 Dongseong Hwang 2012-11-15 20:16:56 PST
Created attachment 174597 [details]
Patch
Comment 10 WebKit Review Bot 2012-11-15 21:18:21 PST
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
Comment 11 Dongseong Hwang 2012-11-16 02:13:58 PST
Created attachment 174634 [details]
Patch
Comment 12 Dongseong Hwang 2012-11-16 02:14:51 PST
(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 13 Noam Rosenthal 2012-11-16 07:03:57 PST
Comment on attachment 174634 [details]
Patch

Who calls setShouldUpdateVisibleRect during the animation?
Comment 14 Dongseong Hwang 2012-11-18 22:14:38 PST
Created attachment 174890 [details]
Patch
Comment 15 Dongseong Hwang 2012-11-18 22:15:35 PST
(In reply to comment #13)
> (From update of attachment 174634 [details])
> Who calls setShouldUpdateVisibleRect during the animation?

Nice review! Now, CoordinatedGraphicsLayer::computeTransformedVisibleRect() calls.
Comment 16 Dongseong Hwang 2012-11-18 22:19:22 PST
Created attachment 174892 [details]
Patch
Comment 17 Dongseong Hwang 2012-11-18 22:20:10 PST
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 18 Noam Rosenthal 2012-11-18 22:21:38 PST
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;
Comment 19 Dongseong Hwang 2012-11-19 00:15:53 PST
(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 20 Noam Rosenthal 2012-11-19 07:27:32 PST
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
Comment 21 Dongseong Hwang 2012-11-19 15:42:43 PST
Created attachment 175059 [details]
Patch
Comment 22 Dongseong Hwang 2012-11-19 15:43:26 PST
(In reply to comment #20)
> (From update of attachment 174892 [details])
> How about then if selfOrAncestorHasActiveTransformAnimation

Good idea. I added this method.
Comment 23 WebKit Review Bot 2012-11-19 16:38:40 PST
Comment on attachment 175059 [details]
Patch

Clearing flags on attachment: 175059

Committed r135212: <http://trac.webkit.org/changeset/135212>
Comment 24 WebKit Review Bot 2012-11-19 16:38:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Thiago Marcos P. Santos 2012-11-20 05:52:49 PST
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
Comment 26 Noam Rosenthal 2012-11-20 07:36:51 PST
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
Comment 27 Raphael Kubo da Costa (:rakuco) 2012-11-20 09:26:20 PST
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.
Comment 28 Dongseong Hwang 2012-11-20 15:35:51 PST
(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.
Comment 29 Dongseong Hwang 2012-11-21 00:32:00 PST
(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