Bug 80372 - RenderLayerCompositor doesn't properly clip graphics layer sizes
Summary: RenderLayerCompositor doesn't properly clip graphics layer sizes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-05 20:20 PST by Adrienne Walker
Modified: 2012-03-20 16:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.55 KB, patch)
2012-03-08 13:42 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Remove recursion (6.54 KB, patch)
2012-03-08 15:00 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Use cached clip rects (14.28 KB, patch)
2012-03-15 18:08 PDT, Adrienne Walker
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-03-05 20:20:49 PST
Copying from previous discussion from https://bugs.webkit.org/show_bug.cgi?id=50192.

On http://goo.gl/eYGqX with Chrome, using accelerated canvas as a compositing trigger, some layer sizes end up being too large.  See https://bugs.webkit.org/attachment.cgi?id=130232.

jamesr says in https://bugs.webkit.org/show_bug.cgi?id=50192#c29:

The <div> being composited has children that extend down that far, but they're clipped by a (software-rendered) ancestor.  When I load the page this part of the DOM looks like:
<div id="views-control" style="z-index: 0"> // this is composited
  <div>
    <div>
      <div class="mv-scroller" style="overflow-y: hidden; height: 26px"> // this clips
        <div>Traffic</div>
        <div>Transit</div>
        <div>Photos</div>
...


Only the Traffic div is visible because of mv-scroller, but the other <div>s still occupy layout space and grow this layer's bounds.
Comment 1 Adrienne Walker 2012-03-08 13:42:57 PST
Created attachment 130895 [details]
Patch
Comment 2 Adrienne Walker 2012-03-08 13:49:29 PST
(In reply to comment #1)
> Created an attachment (id=130895) [details]
> Patch

I'm not super happy with this patch because of the extra work walking up layer trees, but it's a first cut at getting the job done.

The other approach I considered was to switch calculateCompositedBounds to iterate through RenderLayer children directly rather than through the z-order and normal flow lists.  There's some hairiness with which layers get included in those lists and which don't, but it might end up being cleaner.

smfr: What do you think?
Comment 3 Simon Fraser (smfr) 2012-03-08 13:53:09 PST
I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.

I don't like how clippingLayerClosestToAncestor() uses recursion when it could iterate.
Comment 4 Adrienne Walker 2012-03-08 15:00:42 PST
Created attachment 130911 [details]
Remove recursion
Comment 5 Adrienne Walker 2012-03-08 15:04:19 PST
(In reply to comment #3)
> I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.

I may just be misunderstanding clip rects, but the cached clip rects root didn't seem to necessarily be the composited layer passed originally to calculateCompositedBounds.

> I don't like how clippingLayerClosestToAncestor() uses recursion when it could iterate.

Fixed.
Comment 6 Adrienne Walker 2012-03-13 15:22:41 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.
> 
> I may just be misunderstanding clip rects, but the cached clip rects root didn't seem to necessarily be the composited layer passed originally to calculateCompositedBounds.

smfr: If you want to use cached clip rects, do you have any suggestions on how best to go about that?

Is it a possibility to collapse clip rects down the layer tree hierarchy and store directly on child render layers when we build the z-order and normal flow lists? Or, do those lists not get dirtied at the right time?
Comment 7 Simon Fraser (smfr) 2012-03-13 15:33:00 PDT
I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.
Comment 8 Adrienne Walker 2012-03-15 13:54:02 PDT
(In reply to comment #7)
> I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.

As far as I can tell, sometimes layer->clippingRoot() can be an ancestor of that layer's ancestor compositing layer.

So, I can use the cached clip rects, but they are not in the right space.  Transforming them from a container layer to a local layer would require using localToAbsolute and absoluteToLocal (to respect transforms), and therefore a double walk of the render object hierarchy.

That seems more expensive than my original solution of walking up fewer render layers.
Comment 9 Adrienne Walker 2012-03-15 14:05:44 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.
> 
> As far as I can tell, sometimes layer->clippingRoot() can be an ancestor of that layer's ancestor compositing layer.

Er, ignore this.  I misread some code.  Let me take another look and see if I can get clip rects to work.
Comment 10 Adrienne Walker 2012-03-15 18:08:34 PDT
Created attachment 132168 [details]
Use cached clip rects
Comment 11 Adrienne Walker 2012-03-20 14:42:57 PDT
(In reply to comment #10)
> Created an attachment (id=132168) [details]
> Use cached clip rects

smfr: Do you have any time to look at this?
Comment 12 Simon Fraser (smfr) 2012-03-20 14:54:36 PDT
Comment on attachment 132168 [details]
Use cached clip rects

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:511
> -
>  // The bounds of the GraphicsLayer created for a compositing layer is the union of the bounds of all the descendant

I want that blank line :)
Comment 13 Adrienne Walker 2012-03-20 14:58:04 PDT
(In reply to comment #12)
> (From update of attachment 132168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132168&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:511
> > -
> >  // The bounds of the GraphicsLayer created for a compositing layer is the union of the bounds of all the descendant
> 
> I want that blank line :)

No problem, you can have it.  I've got plenty of extras.
Comment 14 Adrienne Walker 2012-03-20 16:00:57 PDT
Committed r111456: <http://trac.webkit.org/changeset/111456>