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.
I get red in chrome...
Created attachment 187075 [details] Test case Oops, uploaded wrong attachment.
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.
Created attachment 187326 [details] Patch
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.
Created attachment 187332 [details] Patch
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
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
(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.
Created attachment 187605 [details] Patch
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
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..
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
Created attachment 187801 [details] Patch Also resize expectations
(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.
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
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
Created attachment 188042 [details] Patch
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?
(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.
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; }
Created attachment 188087 [details] Patch Fix the adjustForContentRect logic for large AC layers
Comment on attachment 188087 [details] Patch I'm ok with the AC parts; Can Kenneth or Jocelyn review the TBS parts?
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.
(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.
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.
Committed r142979: <http://trac.webkit.org/changeset/142979>
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?
(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.
(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.