In the current state of the code before fix, culling assumes that all layers are correctly sorted. However, for the main thread paint side, layers in a 3d rendering context are not explicitly sorted. This causes some tiles to think they are fully occluded when they are not. Two options to solve this are: (1) do layer sorting on the main thread, or (2) 3d layers should not add occlusion to the occlusion tracker. A patch will be coming soon that does option (2); more discussion at http://code.google.com/p/chromium/issues/detail?id=127683
Should 3d layers also not be occluded by other 2d layers?
Actually this patch seems to be nicely contained. I was just re-naming the bug title to properly reflect that =) But that is a good question, let's discuss it further - the way I see it, 3d layers can still properly receive occlusions because they are sorted correctly relative to any existing occlusion.
So a 3d layer can't be below and above (along the z axis) some 2d layer at different points in its layer? ^ \.. | \.. x-|-> --------2d-layer-------- \.. | \..3d layer z \.. These would be in the right order so 2d didn't occlude 3d? What if 3d was moved to the left and now 2d did occlude, they'd be in the right order still?
The current W3C spec says that sorting only occurs between layers within the same 3d rendering context. So the situation you're describing should actually render without the 3d layer intersecting the non-3d layer. I think css z-index mechanisms would decide the ordering here, which would be already represented by the layer tree structure by the time we reach this point in code.
Created attachment 148912 [details] Patch (1) added the proposed fix (2) added a unit test (3) moved one CCOcclusionTracker unit test from main thread to impl thread, so that 3d layers still cause occlusion for that test. Tested manually, unit, and layout on osx, no obvious regressions.
Comment on attachment 148912 [details] Patch Looks good. I wish we had a good data-driven way to make the decision about whether it's worth paying the cost of full sorting to do occlusion checking on the main thread. Someday!
Comment on attachment 148912 [details] Patch Thanks for the review!
Comment on attachment 148912 [details] Patch LGTM too, there are a lot of other unit tests that use 3d transforms though, I'm surprised they are all passing still. Many of them are marked for main thread only right now even.
Comment on attachment 148912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148912&action=review > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:341 > + if (layerIsIn3dRenderingContextOnMainThread(layer)) post-review bikeshed: I don't really like OnMainThread. What about layerIsInUnsorted3DRenderingContext?
Comment on attachment 148912 [details] Patch Removing cq to finish fixing some other feedback first
(In reply to comment #8) > (From update of attachment 148912 [details]) > LGTM too, there are a lot of other unit tests that use 3d transforms though, I'm surprised they are all passing still. Many of them are marked for main thread only right now even. Dana, do you remember the reason you chose main-thread-only for those tests? is it likely to be equivalent if I migrate an preserves3d==true tests to the impl thread? (In reply to comment #9) > (From update of attachment 148912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148912&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:341 > > + if (layerIsIn3dRenderingContextOnMainThread(layer)) > > post-review bikeshed: I don't really like OnMainThread. What about layerIsInUnsorted3DRenderingContext? sounds good, will do that.
Created attachment 148919 [details] Addressed reviewer comments Fixed other tests too. Looking at them, they all turned out to be expecting no occlusion (or no occlusion in the region of interest), so that's why they passed on the main thread after this patch. But for good measure it was appropraite that we move them to the impl thread, otherwise the tests are no longer testing their intended scenarios. Tested everything again; still no obvious regression
Comment on attachment 148919 [details] Addressed reviewer comments Thanks for the test changes!
Comment on attachment 148919 [details] Addressed reviewer comments Clearing flags on attachment: 148919 Committed r121060: <http://trac.webkit.org/changeset/121060>
All reviewed patches have been landed. Closing bug.