Bug 80743 – [chromium] Use CCOcclusionTracker for draw culling

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

[Dana Jansens]

[chromium] Use CCOcclusionTracker for draw culling

[Dana Jansens]

Created attachment 131150 [details]
Patch

[Dana Jansens]

Two layout tests are failing locally with this CL:
compositing/direct-image-compositing.html
compositing/geometry/vertical-scroll-composited.html

They both have translucent directly composited images, and pixels change by 1 value in each color channel. It hints at the off-by-one blending errors again, but I am completely at a loss how this can happen.

The only functional change here is that we are able to cull rotated things against already-known occlusion, and culling across surfaces.

But rotated things don't add themselves to occlusion, so I don't think anything would be culled under the images (also they are not opaque). And there is only one surface in these tests.

So I don't really get it. Any ideas?

[Dana Jansens]

(Oh a couple other tests including compositing/geometry/layer-due-to-layer-children.html fail until bug #80741 lands.)

[WebKit Review Bot]

Comment on attachment 131150 [details]
Patch

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

New failing tests:
compositing/geometry/abs-position-inside-opacity.html
compositing/reflections/become-simple-composited-reflection.html
compositing/geometry/vertical-scroll-composited.html
compositing/geometry/layer-due-to-layer-children.html
compositing/reflections/masked-reflection-on-composited.html
compositing/direct-image-compositing.html
compositing/reflections/nested-reflection-size-change.html
compositing/geometry/layer-due-to-layer-children-deep.html
compositing/geometry/fixed-position.html
accessibility/aria-disabled.html
compositing/masks/multiple-masks.html
compositing/reflections/deeply-nested-reflections.html
compositing/reflections/compositing-change-inside-reflection.html
compositing/reflections/nested-reflection-transformed2.html
compositing/masks/simple-composited-mask.html
compositing/culling/filter-occlusion-alpha-large.html
compositing/reflections/nested-reflection-on-overflow.html
compositing/geometry/video-opacity-overlay.html
compositing/flat-with-transformed-child.html
compositing/reflections/backface-hidden-reflection.html
compositing/reflections/nested-reflection-opacity.html
compositing/culling/filter-occlusion-blur.html
compositing/reflections/nested-reflection-mask-change.html
compositing/reflections/nested-reflection-transformed.html
compositing/culling/filter-occlusion-alpha.html
compositing/masks/masked-ancestor.html
compositing/masks/direct-image-mask.html
compositing/culling/filter-occlusion-blur-large.html
compositing/backface-visibility-hierarchical-transform.html
compositing/reflections/nested-reflection-animated.html

[WebKit Review Bot]

Created attachment 131151 [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 131434 [details]
Patch

[Dana Jansens]

3 layout tests rebaselined for linux that all are rotated layers, that have one of their edges culled and so AA disabled for the edge, which causes slight rendering differences.

I didn't mark it R? to avoid EWS blowing up since this needs 80741 to pass layout tests with 3d transforms. Locally passing everything with drt however.

[Adrienne Walker]

Comment on attachment 131434 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:416
> +    for (size_t i = quadList.size(); i > 0; --i)
> +        drawQuad(quadList[i-1].get(), renderPass->surfaceDamageRect());

style nit: s/i-1/i - 1/

If you're going to remove the explicit reversing of the quad list, can you document in CCRenderPass that the quad list is in reverse order? Or, alternatively encapsulate that in CCRenderPass with quad list accessors that do the reversing internally, so that it doesn't trip anybody else up?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:258
> +        surfacePassMap.add(renderSurface, pass.get());

Is this setting up for a use-after-free? I think the OwnPtr might be destroyed after this loop exits.

[Dana Jansens]

(In reply to comment #8)
> (From update of attachment 131434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131434&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:416
> > +    for (size_t i = quadList.size(); i > 0; --i)
> > +        drawQuad(quadList[i-1].get(), renderPass->surfaceDamageRect());
> 
> style nit: s/i-1/i - 1/

done.

> If you're going to remove the explicit reversing of the quad list, can you document in CCRenderPass that the quad list is in reverse order? Or, alternatively encapsulate that in CCRenderPass with quad list accessors that do the reversing internally, so that it doesn't trip anybody else up?

done. wrapped the vector in a subclass that renames reverse_iterator to front_to_back_iterator.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:258
> > +        surfacePassMap.add(renderSurface, pass.get());
> 
> Is this setting up for a use-after-free? I think the OwnPtr might be destroyed after this loop exits.

Should be okay. The RenderPass* is released to the CCRenderPassList (which is a Vector<OwnPtr<>>) on the next line. And that list is owned by the calling function.

[Dana Jansens]

Created attachment 131640 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131640 [details]
Patch

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

[Dana Jansens]

sick bot? :(

[Adrienne Walker]

Comment on attachment 131640 [details]
Patch

Quite right about the use-after-free.  I just misread the diff.  Looks good to me, and the cr-linux failure looks like build machine weirdness, unrelated to this.  Can you keep an eye on this after it lands to make sure?

[WebKit Review Bot]

Comment on attachment 131640 [details]
Patch

Clearing flags on attachment: 131640

Committed r110596: <http://trac.webkit.org/changeset/110596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.