Bug 89704 – [chromium] Do not accumulate occlusion from 3d layers on the main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

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

[Dana Jansens]

Should 3d layers also not be occluded by other 2d layers?

[Shawn Singh]

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.

[Dana Jansens]

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?

[Shawn Singh]

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.

[Shawn Singh]

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.

[James Robinson]

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!

[Shawn Singh]

Comment on attachment 148912 [details]
Patch

Thanks for the review!

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Shawn Singh]

Comment on attachment 148912 [details]
Patch

Removing cq to finish fixing some other feedback first

[Shawn Singh]

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

[Shawn Singh]

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

[Dana Jansens]

Comment on attachment 148919 [details]
Addressed reviewer comments

Thanks for the test changes!

[WebKit Review Bot]

Comment on attachment 148919 [details]
Addressed reviewer comments

Clearing flags on attachment: 148919

Committed r121060: <http://trac.webkit.org/changeset/121060>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.