Bug 80743 - [chromium] Use CCOcclusionTracker for draw culling
Summary: [chromium] Use CCOcclusionTracker for draw culling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 80741 81083
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-09 18:18 PST by Dana Jansens
Modified: 2012-03-13 23:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (74.74 KB, patch)
2012-03-09 20:52 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (1.23 MB, application/zip)
2012-03-09 21:29 PST, WebKit Review Bot
no flags Details
Patch (548.59 KB, patch)
2012-03-12 16:04 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (549.65 KB, patch)
2012-03-13 09:44 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-03-09 18:18:59 PST
[chromium] Use CCOcclusionTracker for draw culling
Comment 1 Dana Jansens 2012-03-09 20:52:11 PST
Created attachment 131150 [details]
Patch
Comment 2 Dana Jansens 2012-03-09 20:57:38 PST
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?
Comment 3 Dana Jansens 2012-03-09 20:58:13 PST
(Oh a couple other tests including compositing/geometry/layer-due-to-layer-children.html fail until bug #80741 lands.)
Comment 4 WebKit Review Bot 2012-03-09 21:29:39 PST
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
Comment 5 WebKit Review Bot 2012-03-09 21:29:44 PST
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
Comment 6 Dana Jansens 2012-03-12 16:04:19 PDT
Created attachment 131434 [details]
Patch
Comment 7 Dana Jansens 2012-03-12 16:06:25 PDT
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.
Comment 8 Adrienne Walker 2012-03-12 20:18:58 PDT
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.
Comment 9 Dana Jansens 2012-03-13 08:49:03 PDT
(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.
Comment 10 Dana Jansens 2012-03-13 09:44:38 PDT
Created attachment 131640 [details]
Patch
Comment 11 WebKit Review Bot 2012-03-13 10:16:38 PDT
Comment on attachment 131640 [details]
Patch

Attachment 131640 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11945554
Comment 12 Dana Jansens 2012-03-13 10:31:04 PDT
sick bot? :(
Comment 13 Adrienne Walker 2012-03-13 11:15:33 PDT
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?
Comment 14 WebKit Review Bot 2012-03-13 13:15:02 PDT
Comment on attachment 131640 [details]
Patch

Clearing flags on attachment: 131640

Committed r110596: <http://trac.webkit.org/changeset/110596>
Comment 15 WebKit Review Bot 2012-03-13 13:15:19 PDT
All reviewed patches have been landed.  Closing bug.