Bug 73270 – [Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() can leave layers with stale render surfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Sievers]

[Chromium] Early returns in calculateDrawTransformsAndVisibilityInternal() can leave layers with stale render surfaces

[Daniel Sievers]

Created attachment 116855 [details]
Patch

[Daniel Sievers]

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.

[Adrienne Walker]

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.

[Daniel Sievers]

(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);

[Daniel Sievers]

>     EXPECT_NE(renderSurface1->renderSurface(), surface);

surface being the old value of the initial extra render surface created for the layer before.

[Adrienne Walker]

>     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.

[Daniel Sievers]

Created attachment 117309 [details]
Patch

[Daniel Sievers]

Created attachment 117310 [details]
Patch

[Adrienne Walker]

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.

[WebKit Review Bot]

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

[James Robinson]

Can you rebase this patch to ToT, Daniel?

[Daniel Sievers]

Created attachment 117375 [details]
Patch

[Daniel Sievers]

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?

[WebKit Review Bot]

Comment on attachment 117375 [details]
Patch

Clearing flags on attachment: 117375

Committed r101702: <http://trac.webkit.org/changeset/101702>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.