[Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() can leave layers with stale render surfaces
Created attachment 116855 [details] Patch
Note that the first EXPECT_EQ() actually currently passes. EXPECT_EQ(renderSurfaceLayerList.size(), 0U); ASSERT_FALSE(renderSurface1->renderSurface()); However, the layer still has a stale render surface even if it might end up being drawn onto another render surface now (i.e. root layer). The texture residency logic seems to not really like this. We might assert that the texture is currently not protected early on (even if there is nothing to draw), because we expect any referenced render surfaces to have been processed before as part of the same frame.
Isn't the problem that renderSurface1 appears in a layer list but doesn't appear in the render surface list? That seems to me like the invariant that this test should be testing. I don't know that we're always going to clear render surfaces from every layer. For instance, we skip entire layer subtrees if we know that they're not going to get drawn.
(In reply to comment #3) > Isn't the problem that renderSurface1 appears in a layer list but doesn't appear in the render surface list? That seems to me like the invariant that this test should be testing. > Are you talking about the test in general or just the concluding checks? I guess the test shows one case (for the !drawOpacity early-return) that is currently broken. When making the layer transparent the scenario you describe is the case... > I don't know that we're always going to clear render surfaces from every layer. For instance, we skip entire layer subtrees if we know that they're not going to get drawn. ...so should I change it to something like: // After the second pass and having made the layer fully transparent, we expect the // intermediate render surface to have been removed. The layer should now be drawn onto // the root render surface. It's important that a given render surface's layer list does // not reference other render surfaces which are not part of renderSurfaceLayerList for // the current frame (as the content texture residency logic relies on it). EXPECT_EQ(parent->renderSurface()->layerList().size(), 1U); EXPECT_EQ(parent->renderSurface()->layerList()[0].get(), renderSurface1.get()); EXPECT_EQ(renderSurfaceLayerList.size(), 0U); EXPECT_NE(renderSurface1->renderSurface(), surface);
> EXPECT_NE(renderSurface1->renderSurface(), surface); surface being the old value of the initial extra render surface created for the layer before.
> EXPECT_EQ(parent->renderSurface()->layerList().size(), 1U); > EXPECT_EQ(parent->renderSurface()->layerList()[0].get(), renderSurface1.get()); > EXPECT_EQ(renderSurfaceLayerList.size(), 0U); > EXPECT_NE(renderSurface1->renderSurface(), surface); What you wrote above is what I would expect from the current code. > EXPECT_EQ(parent->renderSurface()->layerList().size(), 0); > EXPECT_EQ(renderSurfaceLayerList.size(), 0); This is what I would expect if the code were correct.
Created attachment 117309 [details] Patch
Created attachment 117310 [details] Patch
Comment on attachment 117309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117309&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:368 > + if (drawsContent) { This is great. It looks like this also fixes a bug where layers that don't get drawn were still influencing the drawable content rect size.
Comment on attachment 117310 [details] Patch Rejecting attachment 117310 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ng file Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp Hunk #4 FAILED at 389. Hunk #5 succeeded at 454 (offset -2 lines). 1 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp.rej patching file Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp Hunk #1 succeeded at 481 with fuzz 1 (offset 29 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10687517
Can you rebase this patch to ToT, Daniel?
Created attachment 117375 [details] Patch
Done. I've also added this check to the unit test to make sure we don't expand the parent's drawable content rect from the children that are not drawing: EXPECT_EQ(parent->drawableContentRect(), IntRect()); (In reply to comment #11) > Can you rebase this patch to ToT, Daniel?
Comment on attachment 117375 [details] Patch Clearing flags on attachment: 117375 Committed r101702: <http://trac.webkit.org/changeset/101702>
All reviewed patches have been landed. Closing bug.