Bug 73270 - [Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() can leave layers with stale render surfaces
Summary: [Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() ca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Sievers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-28 17:16 PST by Daniel Sievers
Modified: 2011-12-01 13:45 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.54 KB, patch)
2011-11-28 17:16 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2011-11-30 18:50 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (8.41 KB, patch)
2011-11-30 18:52 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2011-12-01 01:03 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Sievers 2011-11-28 17:16:33 PST
[Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() can leave layers with stale render surfaces
Comment 1 Daniel Sievers 2011-11-28 17:16:59 PST
Created attachment 116855 [details]
Patch
Comment 2 Daniel Sievers 2011-11-28 17:37:05 PST
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.
Comment 3 Adrienne Walker 2011-11-29 09:16:37 PST
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.
Comment 4 Daniel Sievers 2011-11-29 14:49:18 PST
(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);
Comment 5 Daniel Sievers 2011-11-29 14:50:14 PST
>     EXPECT_NE(renderSurface1->renderSurface(), surface);

surface being the old value of the initial extra render surface created for the layer before.
Comment 6 Adrienne Walker 2011-11-29 16:38:16 PST
>     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.
Comment 7 Daniel Sievers 2011-11-30 18:50:18 PST
Created attachment 117309 [details]
Patch
Comment 8 Daniel Sievers 2011-11-30 18:52:21 PST
Created attachment 117310 [details]
Patch
Comment 9 Adrienne Walker 2011-11-30 19:03:11 PST
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 10 WebKit Review Bot 2011-11-30 23:03:20 PST
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
Comment 11 James Robinson 2011-11-30 23:07:19 PST
Can you rebase this patch to ToT, Daniel?
Comment 12 Daniel Sievers 2011-12-01 01:03:41 PST
Created attachment 117375 [details]
Patch
Comment 13 Daniel Sievers 2011-12-01 01:05:23 PST
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 14 WebKit Review Bot 2011-12-01 13:45:10 PST
Comment on attachment 117375 [details]
Patch

Clearing flags on attachment: 117375

Committed r101702: <http://trac.webkit.org/changeset/101702>
Comment 15 WebKit Review Bot 2011-12-01 13:45:15 PST
All reviewed patches have been landed.  Closing bug.