Bug 81227 – [chromium] Cull occluded surface quads

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

[Dana Jansens]

[chromium] Cull occluded surface quads

[Dana Jansens]

Created attachment 133942 [details]
Patch

[Dana Jansens]

Note: this fails compositing/backface-visibility-hierarchical-transform.html until bug #82270 fixes the surface clipRect.

[WebKit Review Bot]

Comment on attachment 133942 [details]
Patch

Attachment 133942 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12148230

New failing tests:
compositing/backface-visibility-hierarchical-transform.html

[WebKit Review Bot]

Created attachment 133969 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Comment on attachment 133942 [details]
Patch

Will be fixing this based on shawn's work.

[Dana Jansens]

Created attachment 135131 [details]
Patch

Correctly guard use of the render surface clipRect() based on shawn's work in determining clipRect() correctness.

[Adrienne Walker]

Comment on attachment 135131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135131&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:359
> +        const RenderSurfaceType* targetSurface = m_stack.last().surface;

Maybe I am misunderstanding, but should this be secondLast.surface?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:393
> +    return unionRect(unoccludedSurfaceRect, unoccludedReplicaRect);

I see what you're doing here, but this is super weird.  It's weird to need to draw part of either the surface or the replica because the other is unoccluded and it's weird to have tests that this union is the correct result.

Could you maybe do one of:

(a) shave this yak and separate replicas into separate layers or even just separate quads (in some separate CL)

(b) put the replica unoccluded rect somewhere separate so that when we do complete (a) your tests don't have to be rewritten?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:54
> +    if (drawQuad->material() == CCDrawQuad::RenderSurface)

Ew.  If we created different materials for render surfaces, I wouldn't want occlusion to start failing.  The material property is how it should be drawn, not what it is.

Can the caller pass this information in explicitly, e.g. refactor into appendSurface/append/appendInternal?

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:1041
>          occlusion.leaveToTargetRenderSurface(parent->renderSurface());
> +
> +        // There is nothing above child2's surface in the z-order.
> +        EXPECT_EQ_RECT(IntRect(-10, 420, 70, 80), occlusion.unoccludedContributingSurfaceContentRect(child2, IntRect(-10, 420, 70, 80)));

Should this expect call occur before the leaveToTargetRenderSurface? Why does this not trigger the asserts in this function?

[Dana Jansens]

(In reply to comment #7)
> (From update of attachment 135131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135131&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:359
> > +        const RenderSurfaceType* targetSurface = m_stack.last().surface;
> 
> Maybe I am misunderstanding, but should this be secondLast.surface?

oh thanks!

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:393
> > +    return unionRect(unoccludedSurfaceRect, unoccludedReplicaRect);
> 
> I see what you're doing here, but this is super weird.  It's weird to need to draw part of either the surface or the replica because the other is unoccluded and it's weird to have tests that this union is the correct result.
> 
> Could you maybe do one of:
> 
> (a) shave this yak and separate replicas into separate layers or even just separate quads (in some separate CL)

ok i will look into making them separate quads for now..

> (b) put the replica unoccluded rect somewhere separate so that when we do complete (a) your tests don't have to be rewritten?

..because i have no idea where else i'd put this rect :)

[Dana Jansens]

(In reply to comment #7)
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:54
> > +    if (drawQuad->material() == CCDrawQuad::RenderSurface)
> 
> Ew.  If we created different materials for render surfaces, I wouldn't want occlusion to start failing.  The material property is how it should be drawn, not what it is.
> 
> Can the caller pass this information in explicitly, e.g. refactor into appendSurface/append/appendInternal?

Ok added appendSurface and appendReplica. I didn't want to add a replica version too, but its needed for the debug quads, as the debug quad has no idea if this is for a surface or a replica unless it is in the shared quad state. And it doesn't really make sense there unless we make a SharedSurfaceQuadState..

> > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:1041
> >          occlusion.leaveToTargetRenderSurface(parent->renderSurface());
> > +
> > +        // There is nothing above child2's surface in the z-order.
> > +        EXPECT_EQ_RECT(IntRect(-10, 420, 70, 80), occlusion.unoccludedContributingSurfaceContentRect(child2, IntRect(-10, 420, 70, 80)));
> 
> Should this expect call occur before the leaveToTargetRenderSurface? Why does this not trigger the asserts in this function?

Yes you're right. At this point there is only 1 surface on the stack in the occlusion tracker so it skips by the assert. Will move the assert.

[Dana Jansens]

Created attachment 135862 [details]
Patch

Not marking for review and EWS until replica quads bug is resolved.

[Adrienne Walker]

Comment on attachment 135862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135862&action=review

Thanks for doing that refactoring first.  Also, does this need to be rebased?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:363
> +    TransformationMatrix transformToScreen = forReplica ? surface->replicaScreenSpaceTransform() : surface->screenSpaceTransform();
> +    TransformationMatrix transformToTarget = forReplica ? surface->replicaOriginTransform() : surface->originTransform();

Make these const references?

[Dana Jansens]

Comment on attachment 135862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135862&action=review

I see what I did here, I included both this and the previous CL in one. Will rebase :)

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:363
>> +    TransformationMatrix transformToTarget = forReplica ? surface->replicaOriginTransform() : surface->originTransform();
> 
> Make these const references?

Good call.

[Dana Jansens]

Created attachment 136107 [details]
Patch

Use const reference, and rebased.

[WebKit Review Bot]

Comment on attachment 136107 [details]
Patch

Rejecting attachment 136107 [details] from commit-queue.

New failing tests:
CCOcclusionTrackerTestSurfaceAndReplicaOccludedDifferentlyImplThreadOpaqueLayers.runTest
CCOcclusionTrackerTestSurfaceWithReplicaUnoccludedImplThreadOpaqueLayers.runTest
CCOcclusionTrackerTestReplicaOccludedImplThreadOpaqueLayers.runTest
Full output: http://queues.webkit.org/results/12370133

[WebKit Review Bot]

Created attachment 136205 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 136107 [details]
Patch

Attachment 136107 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12365981

New failing tests:
CCOcclusionTrackerTestSurfaceAndReplicaOccludedDifferentlyImplThreadOpaqueLayers.runTest
CCOcclusionTrackerTestSurfaceWithReplicaUnoccludedImplThreadOpaqueLayers.runTest
CCOcclusionTrackerTestReplicaOccludedImplThreadOpaqueLayers.runTest

[WebKit Review Bot]

Created attachment 136212 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Created attachment 136283 [details]
Patch

No change, the uninitialized variable in CCMathUtil::mapQuad was causing the test failures. This is fixed in blocker 83494.

[WebKit Review Bot]

Comment on attachment 136283 [details]
Patch

Clearing flags on attachment: 136283

Committed r113652: <http://trac.webkit.org/changeset/113652>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.