[chromium] Cull occluded surface quads
Created attachment 133942 [details] Patch
Note: this fails compositing/backface-visibility-hierarchical-transform.html until bug #82270 fixes the surface clipRect.
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
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
Comment on attachment 133942 [details] Patch Will be fixing this based on shawn's work.
Created attachment 135131 [details] Patch Correctly guard use of the render surface clipRect() based on shawn's work in determining clipRect() correctness.
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?
(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 :)
(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.
Created attachment 135862 [details] Patch Not marking for review and EWS until replica quads bug is resolved.
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?
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.
Created attachment 136107 [details] Patch Use const reference, and rebased.
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
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
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
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
Created attachment 136283 [details] Patch No change, the uninitialized variable in CCMathUtil::mapQuad was causing the test failures. This is fixed in blocker 83494.
Comment on attachment 136283 [details] Patch Clearing flags on attachment: 136283 Committed r113652: <http://trac.webkit.org/changeset/113652>
All reviewed patches have been landed. Closing bug.