Bug 109179 – [CoordGfx] Regression from r135212: big layers with transform animations sometime fail to render tiles.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Created attachment 187072 [details]
Test case - looks green in chrome, red in CoordGfx-based MiniBrowser

In https://bugs.webkit.org/show_bug.cgi?id=102313, we added an optimization that allows releasing tiles of transform-animated layers when they are outside the viewport.
This optimization works in some cases; But in slower animations when the layer has more than one tile and nothing else happens during the animation, the new tiles would not be recreated until something triggers a layout/render. This is because the animation works completely in the UI process, and nobody triggers CoordinatedLayerTreeHost to flush layer changes or check for new tiles.

A possible solution would be to periodically compute the visible rect when a transform animation is ongoing.

[Kenneth Rohde Christiansen]

I get red in chrome...

[Noam Rosenthal]

Created attachment 187075 [details]
Test case

Oops, uploaded wrong attachment.

[Allan Sandfeld Jensen]

It turns out we do regularly check for new tiles to create and flush layer changes. Unfortunately this only leads to tiles being dropped and in the end the last tile being created and dropped in an infinite loop. 

Besides the infinite loop thing, the problem seems to be that the visible rect is not updated on the WebProcess side. Or rather it is constantly updated using outdated values.

[Allan Sandfeld Jensen]

Created attachment 187326 [details]
Patch

[Noam Rosenthal]

Comment on attachment 187326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187326&action=review

Great! But needs an autotest. (pixel-test?)

> Source/WebCore/ChangeLog:11
> +        Force updates of the visible rect while it is animating, and until we have done one last update after
> +        it stops animating.
> +
> +        * platform/graphics/TiledBackingStore.cpp:

Needs an autotest.

[Allan Sandfeld Jensen]

Created attachment 187332 [details]
Patch

[Build Bot]

Comment on attachment 187332 [details]
Patch

Attachment 187332 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16435702

New failing tests:
compositing/transitions/transform-on-large-layer.html

[WebKit Review Bot]

Comment on attachment 187332 [details]
Patch

Attachment 187332 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16471442

New failing tests:
compositing/transitions/transform-on-large-layer.html
platform/chromium/virtual/softwarecompositing/transitions/transform-on-large-layer.html

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (From update of attachment 187332 [details])
> Attachment 187332 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/16435702
> 
> New failing tests:
> compositing/transitions/transform-on-large-layer.html

I guessed it would fail on Chromium since it has a similar issue, but I am surprised it fails on Safari as well. Could just be a hard case to solve, or maybe the baseline I commited is not as generic as I thought.

[Allan Sandfeld Jensen]

Created attachment 187605 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187605 [details]
Patch

Attachment 187605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16501286

New failing tests:
compositing/transitions/transform-on-large-layer.html
platform/chromium/virtual/softwarecompositing/transitions/transform-on-large-layer.html

[Dongseong Hwang]

Comment on attachment 187605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187605&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:968
> +    if (!m_shouldUpdateVisibleRect && !m_movingVisibleRect)

Thank you for fixing this bug that I created.

Could you explain why we need new member: m_movingVisibleRect?
currently, flushCompositingStateForThisLayerOnly() calls computeTransformedVisibleRect() every frame. and computeTransformedVisibleRect() is called only from flushCompositingStateForThisLayerOnly().
I don't fully understand how this patch changes behavior. I mean the behavior looks like the same to before to me..

[Build Bot]

Comment on attachment 187605 [details]
Patch

Attachment 187605 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16488733

New failing tests:
compositing/transitions/transform-on-large-layer.html

[Allan Sandfeld Jensen]

Created attachment 187801 [details]
Patch

Also resize expectations

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 187605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187605&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:968
> > +    if (!m_shouldUpdateVisibleRect && !m_movingVisibleRect)
> 
> Thank you for fixing this bug that I created.
> 
> Could you explain why we need new member: m_movingVisibleRect?
> currently, flushCompositingStateForThisLayerOnly() calls computeTransformedVisibleRect() every frame. and computeTransformedVisibleRect() is called only from flushCompositingStateForThisLayerOnly().
> I don't fully understand how this patch changes behavior. I mean the behavior looks like the same to before to me..

The difference is that computeTransformedVisibleRect() is also called one last time after the animation stops. Otherwise something that jumps to the end in a single frame would fail to update the visible rect.

[Build Bot]

Comment on attachment 187801 [details]
Patch

Attachment 187801 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16522409

New failing tests:
compositing/transitions/transform-on-large-layer.html

[Build Bot]

Comment on attachment 187801 [details]
Patch

Attachment 187801 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16510644

New failing tests:
compositing/transitions/transform-on-large-layer.html

[Allan Sandfeld Jensen]

Created attachment 188042 [details]
Patch

[Noam Rosenthal]

Comment on attachment 188042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188042&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:427
> +            keepRect.unite(coverRect);

Is this the same bug?

[Allan Sandfeld Jensen]

(In reply to comment #19)
> (From update of attachment 188042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188042&action=review
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:427
> > +            keepRect.unite(coverRect);
> 
> Is this the same bug?

It is too different changes but they are both needed for animating large layers without dropping tiles.

[Dongseong Hwang]

Comment on attachment 188042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188042&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:671
> +    if (!hasActiveTransformAnimation)

(In reply to comment #15)
> The difference is that computeTransformedVisibleRect() is also called one last time after the animation stops. Otherwise something that jumps to the end in a single frame would fail to update the visible rect.

Thank you for explanation. I find '!' in if statement now.

Your approach is quite good, but how about get together all code into computeTransformedVisibleRect()? please see computeTransformedVisibleRect.
If so, other developers don't need to see both flushCompositingStateForThisLayerOnly() and computeTransformedVisibleRect() to understand how this patch fixed the bug.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:967
>  {

how about fix this bug as changing only this method as follows,

void CoordinatedGraphicsLayer::computeTransformedVisibleRect()
{
    bool hasActiveTransformAnimation = selfOrAncestorHasActiveTransformAnimation();
    bool shouldUpdateDueToAnimation = hasActiveTransformAnimation | m_hasActiveTransformAnimationAtPreviousFrame;
    if (!m_shouldUpdateVisibleRect && !shouldUpdateDueToAnimation) {
        m_hasActiveTransformAnimationAtPreviousFrame = hasActiveTransformAnimation;
        return;
    }
    ...
    m_hasActiveTransformAnimationAtPreviousFrame = hasActiveTransformAnimation;
}

[Allan Sandfeld Jensen]

Created attachment 188087 [details]
Patch

Fix the adjustForContentRect logic for large AC layers

[Noam Rosenthal]

Comment on attachment 188087 [details]
Patch

I'm ok with the AC parts; Can Kenneth or Jocelyn review the TBS parts?

[Dongseong Hwang]

Comment on attachment 188087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188087&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:377
>          rect.inflateX(((pixelsCovered / rect.height()) - rect.width()) / 2);

In TBS part, LGTM.

But I have one question.
Although this inflate code has been in long time, the inflate code is still needed?
If we remove the code, adjustForContentsRect() would be more succinct.

[Allan Sandfeld Jensen]

(In reply to comment #24)
> (From update of attachment 188087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188087&action=review
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:377
> >          rect.inflateX(((pixelsCovered / rect.height()) - rect.width()) / 2);
> 
> In TBS part, LGTM.
> 
> But I have one question.
> Although this inflate code has been in long time, the inflate code is still needed?
> If we remove the code, adjustForContentsRect() would be more succinct.

We could do that. I only kept it because it was possible, and I don't want to change unrelated behavior. Keeping it though means that we can better do memory pressure response in the future.

[Jocelyn Turcotte]

Comment on attachment 188087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188087&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:417
> +    // Adjust the content rect.

This should probably say "Adjust the cover rect." or be removed altogether. The code pretty much says the same.

[Allan Sandfeld Jensen]

Committed r142979: <http://trac.webkit.org/changeset/142979>

[Ryosuke Niwa]

The test added by this patch is failing on all non-GTK+ ports:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=compositing%2Ftransitions%2Ftransform-on-large-layer.html

Should this test be moved into platform/gtk?

[Allan Sandfeld Jensen]

(In reply to comment #28)
> The test added by this patch is failing on all non-GTK+ ports:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=compositing%2Ftransitions%2Ftransform-on-large-layer.html
> 
> Should this test be moved into platform/gtk?

No, what would it do there??

It may need a larger timeout or a faster animation though.

[Allan Sandfeld Jensen]

(In reply to comment #28)
> The test added by this patch is failing on all non-GTK+ ports:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=compositing%2Ftransitions%2Ftransform-on-large-layer.html
> 

Only looks flaky on Mountain Lion WK2, so you may need to mark it flaky for that one, or skip the test completely on Mac.

I also noticed you don't have a Qt WK2 bot in there, which is what I developed the test-case against.