Bug 81227

Summary: [chromium] Cull occluded surface quads
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, dglazkov, enne, jamesr, piman, reveman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74147, 81296, 82270, 83287, 83494    
Bug Blocks: 82262    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cq-03
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch none

Dana Jansens
Reported 2012-03-15 09:27:53 PDT
[chromium] Cull occluded surface quads
Attachments
Patch (41.88 KB, patch)
2012-03-26 17:42 PDT, Dana Jansens
no flags
Archive of layout-test-results from ec2-cr-linux-01 (9.60 MB, application/zip)
2012-03-26 21:10 PDT, WebKit Review Bot
no flags
Patch (46.07 KB, patch)
2012-04-02 10:47 PDT, Dana Jansens
no flags
Patch (58.98 KB, patch)
2012-04-05 11:17 PDT, Dana Jansens
no flags
Patch (47.72 KB, patch)
2012-04-06 17:48 PDT, Dana Jansens
no flags
Archive of layout-test-results from ec2-cq-03 (6.34 MB, application/zip)
2012-04-09 03:02 PDT, WebKit Review Bot
no flags
Archive of layout-test-results from ec2-cr-linux-04 (6.25 MB, application/zip)
2012-04-09 04:28 PDT, WebKit Review Bot
no flags
Patch (47.74 KB, patch)
2012-04-09 12:22 PDT, Dana Jansens
no flags
Dana Jansens
Comment 1 2012-03-26 17:42:38 PDT
Dana Jansens
Comment 2 2012-03-26 17:43:05 PDT
Note: this fails compositing/backface-visibility-hierarchical-transform.html until bug #82270 fixes the surface clipRect.
WebKit Review Bot
Comment 3 2012-03-26 21:10:18 PDT
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
Comment 4 2012-03-26 21:10:25 PDT
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 5 2012-03-30 15:14:11 PDT
Comment on attachment 133942 [details] Patch Will be fixing this based on shawn's work.
Dana Jansens
Comment 6 2012-04-02 10:47:08 PDT
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 7 2012-04-02 18:40:51 PDT
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
Comment 8 2012-04-05 08:33:05 PDT
(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
Comment 9 2012-04-05 11:01:20 PDT
(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
Comment 10 2012-04-05 11:17:37 PDT
Created attachment 135862 [details] Patch Not marking for review and EWS until replica quads bug is resolved.
Adrienne Walker
Comment 11 2012-04-06 17:25:06 PDT
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 12 2012-04-06 17:38:37 PDT
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
Comment 13 2012-04-06 17:48:03 PDT
Created attachment 136107 [details] Patch Use const reference, and rebased.
WebKit Review Bot
Comment 14 2012-04-09 03:02:22 PDT
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
Comment 15 2012-04-09 03:02:29 PDT
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 16 2012-04-09 04:28:01 PDT
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
Comment 17 2012-04-09 04:28:07 PDT
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
Comment 18 2012-04-09 12:22:45 PDT
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 19 2012-04-09 18:23:39 PDT
Comment on attachment 136283 [details] Patch Clearing flags on attachment: 136283 Committed r113652: <http://trac.webkit.org/changeset/113652>
WebKit Review Bot
Comment 20 2012-04-09 18:24:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.