This is actually part 1 of https://bugs.webkit.org/show_bug.cgi?id=94541, I realized it would be a good idea to split it into a separate patch so that any problems down the road may be easier to diagnose. This patch removes the boolean flags that controlled wether clipRects are actually "active" for a layer or not. Instead, now the clipRect is always active. There was one kink where, now that clipRects are passed across renderSurfaces, they may not be valid on the main thread if the transform was animating. I believe it's correct and equivalent to the existing code to prevent the clipRect from affecting drawableContentRects and surface clipRects when !transformToScreenIsKnown(), so I opted for that.
Created attachment 159841 [details] Patch tested on osx unit and layout tests, no obvious regressions. Also tested on the apple iphone page which has been a good practical example of multiple renderSurfaces with clipping and animations all combined; the page works fine with this patch.
Comment on attachment 159841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159841&action=review As long as things aren't removed from the RSLL when they wouldn't have been before, I think you're good. The values within layers are supposed to represent a given point in time in the animation, not the full span of possibilities. So I dont think you don't need to loosen drawableContentRect or anything, if that's what you mean. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:685 > + if (!layer->replicaLayer() && transformToScreenIsKnown(layer) && !clippedContentRect.isEmpty()) { Why transformToScreen? It's only using drawTransform() inside.
(In reply to comment #2) > (From update of attachment 159841 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159841&action=review > > As long as things aren't removed from the RSLL when they wouldn't have been before, I think you're good. The values within layers are supposed to represent a given point in time in the animation, not the full span of possibilities. So I dont think you don't need to loosen drawableContentRect or anything, if that's what you mean. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:685 > > + if (!layer->replicaLayer() && transformToScreenIsKnown(layer) && !clippedContentRect.isEmpty()) { > > Why transformToScreen? It's only using drawTransform() inside. Because the clipRect is now passed down across renderSurfaces, and we shouldn't be using it when any of the hierarchy of transforms is unknown. Without this, it actually does fail your own test case, "verifyClipRectCullsSurfaceWithoutVisibleContent". This is what I wanted to ask you about offline, but after thinking over it more, I think this patch is doing the right thing. Note, there were 2 places where we have to replace subtreeShouldBeSkipped with transformToScreenIsKnown(...) I'm pretty sure those have to be screen space checks for the same reason.
Comment on attachment 159841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159841&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:579 > + // so that it is described with repsect to the current target surface. Note that this transformation may cause typo > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:581 > + clipRectForSubtree = enclosingIntRect(CCMathUtil::projectClippedRect(renderSurface->drawTransform().inverse(), FloatRect(clipRectFromAncestor))); I have a lot of mixed feelings about this patch and this line is the source of most of them. If you project clip rects down, then it will cut down on the ability to reuse render surfaces. If they are ever-so-slightly off-screen then they will change size and need to be entirely redrawn. I also don't see how this change allows you to handle visibleContentRect calculations any more easily. Previously with a boolean, you could either use the clip rect or just set the visible state to the entire content bounds. Now without a boolean, you can just use the clip rect. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:380 > - parentSublayerMatrix.scale3d(10, 10, 3.3); > + parentSublayerMatrix.scale3d(0.3, 0.3, 1); 0.3f
Comment on attachment 159841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159841&action=review >> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:380 >> + parentSublayerMatrix.scale3d(0.3, 0.3, 1); > > 0.3f Er, scratch that, this takes a double. Too many literla float conversion warnings on Windows has clouded my brain.
(In reply to comment #4) > (From update of attachment 159841 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159841&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:579 > > + // so that it is described with repsect to the current target surface. Note that this transformation may cause > > typo > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:581 > > + clipRectForSubtree = enclosingIntRect(CCMathUtil::projectClippedRect(renderSurface->drawTransform().inverse(), FloatRect(clipRectFromAncestor))); > > I have a lot of mixed feelings about this patch and this line is the source of most of them. If you project clip rects down, then it will cut down on the ability to reuse render surfaces. If they are ever-so-slightly off-screen then they will change size and need to be entirely redrawn. > > I also don't see how this change allows you to handle visibleContentRect calculations any more easily. Previously with a boolean, you could either use the clip rect or just set the visible state to the entire content bounds. Now without a boolean, you can just use the clip rect. > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:380 > > - parentSublayerMatrix.scale3d(10, 10, 3.3); > > + parentSublayerMatrix.scale3d(0.3, 0.3, 1); > > 0.3f (1) You're right, we should not affect surface clipRects. It's also worth noting that in one of my previous refactors, I forced the root layer to clip to viewport. This means that renderSurfaces that draw to the root will also be uncachable when they animate and get clipped. I'll fix it very soon. (2) However, we still do need to inverse-project clipRects. I think I figured out how to do this cleanly without forcing us to propagate clipRects down. The downside is that it further relies on surface->clipRect().isEmpty() - but it's definitely less evil than what I was trying to do in this patch =) So this patch is probably WONTFIX, I'll mark it that way when I'm certain. Thanks for the voice of reason Enne =)
I have a working patch now for merging visibleLayerRect that doesn't require this factor.
Comment on attachment 159841 [details] Patch Cleared review? from attachment 159841 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).