Bug 102313 – Coordinated Graphics: Remove tiles of a layer when they are off the viewport.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

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

[Dongseong Hwang]

Created attachment 174312 [details]
Patch

[Dongseong Hwang]

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.

[Dongseong Hwang]

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?

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 174554 [details]
Patch

[Dongseong Hwang]

rebase to upstream

[Noam Rosenthal]

Comment on attachment 174554 [details]
Patch

Have you considered using GraphicsLayerclient::getCurrentTransform which considers current animation status, rather than running our own GraphicsLayerAnimation?

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 174597 [details]
Patch

[WebKit Review Bot]

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

[Dongseong Hwang]

Created attachment 174634 [details]
Patch

[Dongseong Hwang]

(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.

[Noam Rosenthal]

Comment on attachment 174634 [details]
Patch

Who calls setShouldUpdateVisibleRect during the animation?

[Dongseong Hwang]

Created attachment 174890 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 174634 [details])
> Who calls setShouldUpdateVisibleRect during the animation?

Nice review! Now, CoordinatedGraphicsLayer::computeTransformedVisibleRect() calls.

[Dongseong Hwang]

Created attachment 174892 [details]
Patch

[Dongseong Hwang]

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.

[Noam Rosenthal]

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;

[Dongseong Hwang]

(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().

[Noam Rosenthal]

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

[Dongseong Hwang]

Created attachment 175059 [details]
Patch

[Dongseong Hwang]

(In reply to comment #20)
> (From update of attachment 174892 [details])
> How about then if selfOrAncestorHasActiveTransformAnimation

Good idea. I added this method.

[WebKit Review Bot]

Comment on attachment 175059 [details]
Patch

Clearing flags on attachment: 175059

Committed r135212: <http://trac.webkit.org/changeset/135212>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

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

[Noam Rosenthal]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Dongseong Hwang]

(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.

[Dongseong Hwang]

(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