Bug 74147 – [chromium] layer->clipRect() is not initialized for layers that create a renderSurface

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

In calculateDrawTransformsAndVisibility, the clipRect is assumed to be in the space of the target render surface.  However, the clipRect is not being transformed when it propagates to the descendant target surface.

It is possible this is the root cause of some of our reasonable assumptions being violated, for example https://bugs.webkit.org/show_bug.cgi?id=74037

The first thing to do is demonstrate a reduced test case that proves this is actually a problem.
If so, then we can resume with our original assumptions after fixing this bug.

[Shawn Singh]

Changed the bug title to reflect the problem, now that the problem is figured out exactly.

[Shawn Singh]

Created attachment 130217 [details]
Patch

[Shawn Singh]

The patch just uploaded does the following:

(1a) Properly initializes layer->clipRect() when the layer has a renderSurface.

(1b) Moves the masksToBounds code so that it applies to all layers, rather than just layers without renderSurfaces.

(2) Renames "layerOriginTransform" to "surfaceOriginTransform", though both are technically correct, the comment added for (1a) makes more sense when transforming between renderSurfaces.

(3) Extends a unit test to cover this change.

As per Enne's preference, this patch does not remove our fix for https://bugs.webkit.org/show_bug.cgi?id=74037 (refer to comment #11).

[Shawn Singh]

Enne, when you have a chance, could you please review this?  Thanks in advance!

[Adrienne Walker]

Wait, doesn't http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp#L445 set the clip rect for all non-root render surface-creating layers?

[Shawn Singh]

(In reply to comment #5)
> Wait, doesn't http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp#L445 set the clip rect for all non-root render surface-creating layers?

No it doesnt - this sets the clipRect after recursion bubbles back up.  By that time, things were already incorrectly added to layerLists when the clipRect was still un-initialized.   So that's why I put a fixme and a bug url on this patch, related to that exact line of code.

The actual bug being fixed here is that clipRect is un-initialized before/during recursion, for layers that own a renderSurface.


If it helps, here's a reminder of the entire situation:
1. Uninitialized clipRect caused the crasher bug https://bugs.webkit.org/show_bug.cgi?id=74037
2. At that time, we opted for the safer quick fix, post-removing surfaces that we knew shouldn't exist.
3. But really, the correct fix is to initialize clipRect correctly, so they are not added in the first place.  That's what this patch does.
4. We opted to keep the "safe fix" anyway. I adjusted it so that the assert triggers for us, but for user-facing code, the safe fix remains there.

[Shawn Singh]

btw, it may help to refer to comments #10 and #11 on that old crasher bug https://bugs.webkit.org/show_bug.cgi?id=74037   =)

[Adrienne Walker]

Comment on attachment 130217 [details]
Patch

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

Ah, thanks for the context.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:324
> +        TransformationMatrix inverseOriginTransform = surfaceOriginTransform.inverse();
> +        layer->setClipRect(enclosingIntRect(inverseOriginTransform.mapRect(FloatRect(renderSurface->clipRect()))));

How does setUsesLayerClipping interact here? You set the clip rect in this patch, but don't call setUsesLayerClipping.  Previously, the layer wasn't setting its own clip rect until later, but did call setUsesLayerClipping(false) on line 271.  It seems like ew check this value prior to using clipRect everywhere in this function, so I'm curious where clipRect is getting misused without a check to whether a layer uses layer clipping or not?

Forgive my skepticism, but I do not think this code will work in all cases.  If the matrix does not have an inverse, then the clip rect will be empty.  It's also possible that due to floating point or enclosingIntRect issues, we could have a child layer that doesn't get clipped on the way down, but does get clipped on the way back up when we recalculate the clip rect, causing the same issue, just in fewer cases.

I think I'd prefer a simpler solution: leave the while loop as-is, skip an unnecessary matrix inverse, and only set the clip rect when we know what its value will be.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:456
> +        // FIXME: this initialization of clipRect is redundant, and should be
> +        //        double-checked while cleaning up drawableContentRect logic.
> +        //        https://bugs.webkit.org/show_bug.cgi?id=80331
>          layer->setClipRect(layer->drawableContentRect());

This is only redundant if the layer masks to bounds.  If not, this is a more accurate clip rect, since it will be exactly the size of the render surface, and not just the inverse projection of some potentially more loose parent clip.

[Adrienne Walker]

Comment on attachment 130217 [details]
Patch

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

Replying to some offline discussion from Shawn.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:324
>> +        layer->setClipRect(enclosingIntRect(inverseOriginTransform.mapRect(FloatRect(renderSurface->clipRect()))));
> 
> How does setUsesLayerClipping interact here? You set the clip rect in this patch, but don't call setUsesLayerClipping.  Previously, the layer wasn't setting its own clip rect until later, but did call setUsesLayerClipping(false) on line 271.  It seems like ew check this value prior to using clipRect everywhere in this function, so I'm curious where clipRect is getting misused without a check to whether a layer uses layer clipping or not?
> 
> Forgive my skepticism, but I do not think this code will work in all cases.  If the matrix does not have an inverse, then the clip rect will be empty.  It's also possible that due to floating point or enclosingIntRect issues, we could have a child layer that doesn't get clipped on the way down, but does get clipped on the way back up when we recalculate the clip rect, causing the same issue, just in fewer cases.
> 
> I think I'd prefer a simpler solution: leave the while loop as-is, skip an unnecessary matrix inverse, and only set the clip rect when we know what its value will be.

Ok.  I looked at this more, and by setting this here, this will end up making a more accurate content rect on the layer for a surface, which will end up resulting in a tighter visible content rect for a surface, because drawable content rects for child layers will be smaller or empty because they'll inherit this clip.  I still think this shouldn't be necessary for correctness, but maybe this will end up having a performance benefit by making clipped surfaces smaller.

It also looks like we we make a lot of assumptions elsewhere about transforms being invertible (see: RenderLayer::paintLayer, LayerRendererChromium::drawTileQuad, CCLayerTreeHostCommon::calculateVisibleRect), where we just don't draw anything if the transform doesn't have an inverse.  If the transform has no inverse, then the clip rect will end up empty, and that's fine.  So, maybe that's ok.

Although, if now every layer has a clip rect, then maybe we don't need this usesLayerClipping flag anymore...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:456
>>          layer->setClipRect(layer->drawableContentRect());
> 
> This is only redundant if the layer masks to bounds.  If not, this is a more accurate clip rect, since it will be exactly the size of the render surface, and not just the inverse projection of some potentially more loose parent clip.

On second thought, maybe this should be the intersection of the drawable content rect and the previously calculated clip rect?

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:574
> +    // Case 2: Contaminate the grandChild and greatGrandChild's clipRect to reproduce the

Can you put separate tests in separate TEST blocks? When things fail, it makes it a lot more clear where the problem from the logs is if they're broken up.

Can you also write a test case that more thoroughly tests that the new clip rect you've calculated is correct, more than just verifying that the assert no longer fires?

[Shawn Singh]

Thanks for the additional feedback!

(In reply to comment #9)
> 
> Ok.  I looked at this more, and by setting this here, this will end up making a more accurate content rect on the layer for a surface, which will end up resulting in a tighter visible content rect for a surface, because drawable content rects for child layers will be smaller or empty because they'll inherit this clip.  I still think this shouldn't be necessary for correctness, but maybe this will end up having a performance benefit by making clipped surfaces smaller.
> 

Small bikeshed: I insist it is a correctness issue; even though the end-to-end rendering results might remain correct. The code is adding renderSurfaces to the renderSurfaceLayerList that should not be there, which is contrary to the semantics we intend for calcDrawTransforms.

> 
> Although, if now every layer has a clip rect, then maybe we don't need this usesLayerClipping flag anymore...
> 

If we do that we have to be very careful about the use of clipRect.isEmpty(), which is currently being used to detect clipping in at least one other place (the culling code).  So we should do this in a separate bug.
For now, we do need to properly inherit usesLayerClipping - without this, my first patch here is wrong.

> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:574
> > +    // Case 2: Contaminate the grandChild and greatGrandChild's clipRect to reproduce the
> 
> Can you put separate tests in separate TEST blocks? When things fail, it makes it a lot more clear where the problem from the logs is if they're broken up.
> 
> Can you also write a test case that more thoroughly tests that the new clip rect you've calculated is correct, more than just verifying that the assert no longer fires?

OK, I'll separate the tests =)

Can you please elaborate what more needs to be tested?  The test case that I added in this patch does not trigger any assertion, it really shows that surfaces still get incorrectly added to the renderSurfaceLayerList despite our earlier fix.

[Adrienne Walker]

(In reply to comment #10)

> Small bikeshed: I insist it is a correctness issue; even though the end-to-end rendering results might remain correct. The code is adding renderSurfaces to the renderSurfaceLayerList that should not be there, which is contrary to the semantics we intend for calcDrawTransforms.

What is correctness other than end-to-end rendering results? ;)

I don't find it terrible to add surfaces to a list that we remove later once we have more information.  We still have to process all those child layers, so it's not like we save any recursion.  It's just an intermediate state that nothing else but this function sees.

Besides, with your patch, we're *still* adding layers to the list that ultimately don't end up needing to be there--the only difference is that they get removed one at a time instead of as a group.

/me paints the bikeshed a nice periwinkle color.

> Can you please elaborate what more needs to be tested?  The test case that I added in this patch does not trigger any assertion, it really shows that surfaces still get incorrectly added to the renderSurfaceLayerList despite our earlier fix.

Your test is loose--your code adds a new clip rect calculation, but that clip rect could be incorrect and the test could still pass.  I want to better check that the clip rect is being set to the right value.

I'll suggest the following tests:

(1) Have a transformed layer that creates a surface but doesn't mask to bounds.  Make sure that the clip rect that it ends up with is correct by testing its value explicitly.

(2) Same, but with a layer that does mask to bounds.

(3) Compare a transformed layer that creates a surface and is partially clipped to an otherwise identical layer that doesn't create a surface.  Verify that their clip rects are identical.  You could do this with a filter to force the creation of a surface or something similar.

[Shawn Singh]

> > Can you please elaborate what more needs to be tested?  The test case that I added in this patch does not trigger any assertion, it really shows that surfaces still get incorrectly added to the renderSurfaceLayerList despite our earlier fix.
> 
> Your test is loose--your code adds a new clip rect calculation, but that clip rect could be incorrect and the test could still pass.  I want to better check that the clip rect is being set to the right value.
> 

Very good point, I should have seen that =)  I'll come back with a new patch soon.

[Shawn Singh]

I have had the patch ready to submit, but its failing one test case, and I believe it is related to the problem of trying to un-project onto a surface that is behind the projection camera.

So, unfortunately, this has to be blocked until that issue is solved.

[Hajime Morrita]

Comment on attachment 130217 [details]
Patch

Clearing r? to shorten the pending review queue.

[Dana Jansens]

Is the failing test compositing/reflections/nested-reflection-on-overflow.html? 

That's what fails for me with this CL. But with 2 minor adjustments it passes.

1) Use projectRect instead of mapRect like we discussed
2) Intersect with the layer's original clipRect instead of throwing it away.


However, then one of my occlusion tracker unit tests crashes, but that's probably my fault. Looking into that.

[Dana Jansens]

Argh, so intersecting with the layer's original clipRect is actually just setting the layer clipRect to IntRect(). It's not the answer but maybe it'll help you debug..

[Dana Jansens]

Wait(In reply to comment #13)
> I have had the patch ready to submit, but its failing one test case, and I 

Wait if the test case is compositing/reflections/nested-reflection-on-overflow.html then I think we are now passing the test. The "expected" image for chromium has no text, but the mac expectation does have text. I think your CL as posted (with s/mapRect/projectQuad/) does what we want.

[Shawn Singh]

*** Bug 82270 has been marked as a duplicate of this bug. ***

[Shawn Singh]

We should push this for landing because of http://code.google.com/p/chromium/issues/detail?id=119121 (see comment #14)

I have addressed Enne's comments, and for now I added an if-statement (with a FIXME and bug url) that does not try to use the inverse transform if there is a perspective transform.  In that case it uses the entire layer as the clipRect.

Patch is coming in a moment.  =)

[Shawn Singh]

Created attachment 133974 [details]
Patch

passes unit tests and layout tests on mac with no obvious regressions.

[WebKit Review Bot]

Attachment 133974 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:4367:  Path does not exist. fast/eventsource/eventsource-url-attribute.html  [test/expectations] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Comment on attachment 133974 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
> +            layer->setClipRect(transformedLayerRect);

This rect is in the surface's space but it should be in the target space? transformedLayerRect is a bit misleading here! It is the rect after drawTransform when the layer doesn't own a transform. Should we apply surfaceOriginTransform to the transformedLayerRect when a layer owns a surface? This is impacting the code below also.. or argue for why it's correct as is?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:434
> +        IntRect clipRect = transformedLayerRect;

This is just the bounds of the rendersurface when the layer owns a surface, is that right?

[Dana Jansens]

Comment on attachment 133974 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
>> +            layer->setClipRect(transformedLayerRect);
> 
> This rect is in the surface's space but it should be in the target space? transformedLayerRect is a bit misleading here! It is the rect after drawTransform when the layer doesn't own a transform. Should we apply surfaceOriginTransform to the transformedLayerRect when a layer owns a surface? This is impacting the code below also.. or argue for why it's correct as is?

Ah ok, sorry for the confusion and smoke. I see what this is meaning to do. But it is assuming that the surface bounds are the same as the layer bounds which isn't really right. Maybe this is better than having it set to IntRect(0, 0, 0, 0), but this doesn't fix the origin of the clipRect when perspective is in play? Is this just not something we can do atm?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:434
>> +        IntRect clipRect = transformedLayerRect;
> 
> This is just the bounds of the rendersurface when the layer owns a surface, is that right?

Take this comment back, it lgtm.

[Shawn Singh]

Created attachment 134115 [details]
Same patch updated to tip of tree

I think the style issue is a false alarm. Updated to tip of tree; also retested, still no obvious regressions.  Fun fact - this bug number is a palindrome.

[Shawn Singh]

Thanks for looking at this.  =)

(In reply to comment #23)
> (From update of attachment 133974 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133974&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
> >> +            layer->setClipRect(transformedLayerRect);
> > 
> > This rect is in the surface's space but it should be in the target space? transformedLayerRect is a bit misleading here! It is the rect after drawTransform when the layer doesn't own a transform. Should we apply surfaceOriginTransform to the transformedLayerRect when a layer owns a surface? This is impacting the code below also.. or argue for why it's correct as is?
> 
> Ah ok, sorry for the confusion and smoke. I see what this is meaning to do. But it is assuming that the surface bounds are the same as the layer bounds which isn't really right. Maybe this is better than having it set to IntRect(0, 0, 0, 0), but this doesn't fix the origin of the clipRect when perspective is in play? Is this just not something we can do atm?

The clipRect of both the layer and surface get further adjusted below.  I'm pretty sure, however, that the origin of the surface and origin of the layer that owns that surface will always be the same point, so setting the clipRect this way should be fully correct even though its not "optimal".

> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:434
> >> +        IntRect clipRect = transformedLayerRect;
> > 
> > This is just the bounds of the rendersurface when the layer owns a surface, is that right?
> 
> Take this comment back, it lgtm.

But your comments do point to the fact that "transformedLayerRect" should probably be re-named to "layerRectInTargetSpace" or something like that.  Let me know if you would like me to do that on this patch =)

[Dana Jansens]

Comment on attachment 133974 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
>>>> +            layer->setClipRect(transformedLayerRect);
>>> 
>>> This rect is in the surface's space but it should be in the target space? transformedLayerRect is a bit misleading here! It is the rect after drawTransform when the layer doesn't own a transform. Should we apply surfaceOriginTransform to the transformedLayerRect when a layer owns a surface? This is impacting the code below also.. or argue for why it's correct as is?
>> 
>> Ah ok, sorry for the confusion and smoke. I see what this is meaning to do. But it is assuming that the surface bounds are the same as the layer bounds which isn't really right. Maybe this is better than having it set to IntRect(0, 0, 0, 0), but this doesn't fix the origin of the clipRect when perspective is in play? Is this just not something we can do atm?
> 
> The clipRect of both the layer and surface get further adjusted below.  I'm pretty sure, however, that the origin of the surface and origin of the layer that owns that surface will always be the same point, so setting the clipRect this way should be fully correct even though its not "optimal".

It's actually not in compositing/reflections/nested-reflection-on-overflow.html. The origin is negative in both axes.

But perhaps adding a width/height here is strictly better than not having one at all?

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:434
>>>> +        IntRect clipRect = transformedLayerRect;
>>> 
>>> This is just the bounds of the rendersurface when the layer owns a surface, is that right?
>> 
>> Take this comment back, it lgtm.
> 
> But your comments do point to the fact that "transformedLayerRect" should probably be re-named to "layerRectInTargetSpace" or something like that.  Let me know if you would like me to do that on this patch =)

i'd say followup :)

[Adrienne Walker]

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

Sorry for all the comments, but changing calcDrawEtc and transforms always seems to create a lot of bugs.  I need to feel more convinced that these changes are correct.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
> +        if (surfaceOriginTransform.hasPerspective())
> +            layer->setClipRect(transformedLayerRect);

This doesn't seem correct to me.  If the parent uses layer clipping, then transformedLayerRect could be more restrictive than the projection of the clip rect onto this surface.  This could cause children of this layer to inherit this clip rect and possibly clip more than they otherwise would, since this is essentially saying "this layer masks to bounds".  It seems better to set usesLayerClipping to false here and (optionally) initialize the clip rect to empty.

I'd feel better with a test that involved a surface with a perspective transform.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:407
> +        //
> +        // FIXME: it is possible the usesLayerClipping logic can eventually be removed
> +        //        entirely: https://bugs.webkit.org/show_bug.cgi?id=80622

(Maybe this is just personal bias, but I think you've gone overboard with FIXMEs in this patch.  I think a FIXME is more for "this code doesn't handle some particular case" either for performance or for correctness, and not just "I'd like to make this better in the future.")

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:543
> +        // FIXME: the final clipRect could probably be more optimal as the intersection of
> +        //        the existing clipRect and drawableContentRect. https://bugs.webkit.org/show_bug.cgi?id=80331
>          layer->setClipRect(layer->drawableContentRect());

Can you just do that here? That seems like a one line change.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-561
> +            // In theory it should be a valid assumption that the current layer is the
> +            // last one on the list, and the while-loop should be unnecessary. However,
> +            // in https://bugs.webkit.org/show_bug.cgi?id=74037 we opted to keep the
> +            // while-loop to avoid user-facing bugs, while the ASSERT will still trigger
> +            // in debug mode.
> +            ASSERT(renderSurfaceLayerList.last() == layer);
>              while (renderSurfaceLayerList.last() != layer) {
>                  renderSurfaceLayerList.last()->clearRenderSurface();
>                  renderSurfaceLayerList.removeLast();
>              }
> -            ASSERT(renderSurfaceLayerList.last() == layer);

I think this assert is wrong, given that perspective transforms are being ignored on surfaces with respect to clip rects? Couldn't some particular combination of transforms still trigger this?

I continue to feel similarly to what I said in https://bugs.webkit.org/show_bug.cgi?id=74037#c11.  I think just removing the last layer was always too clever of a solution and looping through all the layers is not problematic.  I'd prefer to remove both the assert and giant comment.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:929
> +    IntRect clipRect1 = grandChild1->clipRect();
> +    IntRect clipRect2 = grandChild2->clipRect();
> +    IntRect clipRect3 = grandChild3->clipRect();
> +    IntRect clipRect4 = grandChild4->clipRect();

Is there a need for all these intermediate variables?

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1017
> +    // because grandChild4 is entirely clipped, it is not expected to have a renderSurface.

EXPECT_FALSE?

[Shawn Singh]

Thanks for all the comments.  Here's my rebuttal =)

(1) Talked with Dana offline.  The comment about negative origin was a misunderstanding and does not apply here.  Her comment about changing from empty clipRect to full clipRect was actually a concern because some code might be using if (clipRect().isEmpty()) in the code, and those would then become wrong.   I agree with that, and I think that only happens in one place in the code, and I'll submit a new patch to fix that.

(2) my response to Enne's comments are inlined below.

(In reply to comment #27)
> View in context: https://bugs.webkit.org/attachment.cgi?id=133974&action=review
> 
> Sorry for all the comments, but changing calcDrawEtc and transforms always seems to create a lot of bugs.  I need to feel more convinced that these changes are correct.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:398
> > +        if (surfaceOriginTransform.hasPerspective())
> > +            layer->setClipRect(transformedLayerRect);
> 
> This doesn't seem correct to me.  If the parent uses layer clipping, then transformedLayerRect could be more restrictive than the projection of the clip rect onto this surface.  This could cause children of this layer to inherit this clip rect and possibly clip more than they otherwise would, since this is essentially saying "this layer masks to bounds".  It seems better to set usesLayerClipping to false here and (optionally) initialize the clip rect to empty.

Yeah I agree.  Good catch about usesLayerclipping().

For now I can set the clipRect to empty, too, just to hold us over.  If that's true, then there is one other piece of logic to fix; where masksToBounds is true, we should not intersect the parent's clipRect unless usesLayerClipping is true.  I'll go ahead and make that change, too.

> 
> I'd feel better with a test that involved a surface with a perspective transform.
> 

OK, I'll do that.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:407
> > +        //
> > +        // FIXME: it is possible the usesLayerClipping logic can eventually be removed
> > +        //        entirely: https://bugs.webkit.org/show_bug.cgi?id=80622
> 
> (Maybe this is just personal bias, but I think you've gone overboard with FIXMEs in this patch.  I think a FIXME is more for "this code doesn't handle some particular case" either for performance or for correctness, and not just "I'd like to make this better in the future.")

I just didn't want you to think I ignored your previous feedback, even though those changes should not happen in this patch. =)


> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:543
> > +        // FIXME: the final clipRect could probably be more optimal as the intersection of
> > +        //        the existing clipRect and drawableContentRect. https://bugs.webkit.org/show_bug.cgi?id=80331
> >          layer->setClipRect(layer->drawableContentRect());
> 
> Can you just do that here? That seems like a one line change.

I feel uncomfortable changing this in this patch.  It is technically not part of the bugfix, but it could have unintended consequences that we couldn't foresee because of the tangle between clipRect and drawableContentRect... I would prefer to do that as part of my incremental refactoring strategy.  This patch will need to be merged back to m19, and I feel this drive-by cleanup is too risky for that. 

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-561
> > +            // In theory it should be a valid assumption that the current layer is the
> > +            // last one on the list, and the while-loop should be unnecessary. However,
> > +            // in https://bugs.webkit.org/show_bug.cgi?id=74037 we opted to keep the
> > +            // while-loop to avoid user-facing bugs, while the ASSERT will still trigger
> > +            // in debug mode.
> > +            ASSERT(renderSurfaceLayerList.last() == layer);
> >              while (renderSurfaceLayerList.last() != layer) {
> >                  renderSurfaceLayerList.last()->clearRenderSurface();
> >                  renderSurfaceLayerList.removeLast();
> >              }
> > -            ASSERT(renderSurfaceLayerList.last() == layer);
> 
> I think this assert is wrong, given that perspective transforms are being ignored on surfaces with respect to clip rects? Couldn't some particular combination of transforms still trigger this?
> 
> I continue to feel similarly to what I said in https://bugs.webkit.org/show_bug.cgi?id=74037#c11.  I think just removing the last layer was always too clever of a solution and looping through all the layers is not problematic.  I'd prefer to remove both the assert and giant comment.
> 

tl;dr As a compromise I'll remove the change in this patch and leave the code unchanged for now.  This change isn't really part of the bugfix either.

Personally I dont think this code should get so cozy and comfortable here.  I would like to see it eventually removed.  It should be easily possible to remove this code by restructuring things so that surfaces are added to the renderSurfaceLayerList only after we know that they draw content.

To me, removing layers like this as a post-adjustment is a hack that adds to logic complexity and the cognitive load of doing anything in this function.  It requires us to make assumptions about how we arranged the schedule of renderSurfaces with respect to the structure of the tree, these are not reasonable assumptions (for example, when we fix layer sorting across multiple renderSurfaces that have preserves3D, this assumption might break).  At the time, we added this post-fix because we were not willing to restructure the logic in that way.  But I really think we should put a strong effort to removing this logic, and I'm not aware of any significant reason that we can't accomplish that.

(As a side note, in comment 11 you mentioned that we remove layers later on, too, but I don't see where in the code do we remove more surfaces/layers from the list ?)

> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:929
> > +    IntRect clipRect1 = grandChild1->clipRect();
> > +    IntRect clipRect2 = grandChild2->clipRect();
> > +    IntRect clipRect3 = grandChild3->clipRect();
> > +    IntRect clipRect4 = grandChild4->clipRect();
> 
> Is there a need for all these intermediate variables?
> 

probably not =) will remove them.

> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1017
> > +    // because grandChild4 is entirely clipped, it is not expected to have a renderSurface.
> 
> EXPECT_FALSE?

OK sure.

[Adrienne Walker]

(In reply to comment #28)

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:543
> > > +        // FIXME: the final clipRect could probably be more optimal as the intersection of
> > > +        //        the existing clipRect and drawableContentRect. https://bugs.webkit.org/show_bug.cgi?id=80331
> > >          layer->setClipRect(layer->drawableContentRect());
> > 
> > Can you just do that here? That seems like a one line change.
> 
> I feel uncomfortable changing this in this patch.  It is technically not part of the bugfix, but it could have unintended consequences that we couldn't foresee because of the tangle between clipRect and drawableContentRect... I would prefer to do that as part of my incremental refactoring strategy.  This patch will need to be merged back to m19, and I feel this drive-by cleanup is too risky for that.

The smallest, least risky patch would be to just disable the clip rect for all layers that create surfaces on the way down.

My worry here is that you are setting the clip rect to one thing on the way down and then something else entirely after you've come back up.  If there's an error in the new clip rect you are calculating, layers will get dropped--in my opinion, that's the risky part of this patch.  It seems really tricky to try to solve that sort of bug post-hoc if clip rects get a final value that is potentially more loose than the clip rect that we used when determining which layers were visible.

[Shawn Singh]

> The smallest, least risky patch would be to just disable the clip rect for all layers that create surfaces on the way down.

Yeah I was just coming across that solution as I was addressing your feedback.  Next patch should reflect that =)

> 
> My worry here is that you are setting the clip rect to one thing on the way down and then something else entirely after you've come back up.  If there's an error in the new clip rect you are calculating, layers will get dropped--in my opinion, that's the risky part of this patch.  It seems really tricky to try to solve that sort of bug post-hoc if clip rects get a final value that is potentially more loose than the clip rect that we used when determining which layers were visible.


I'm not changing anything about the behavior of how clipRect is set on the way back up...  And yes I agree that is questionable stuff.  https://bugs.webkit.org/show_bug.cgi?id=80331  ... sorry I didn't cc you on that earlier =)


Anyway, the next patch should be far more comfortable.  Thanks!

[Shawn Singh]

Created attachment 134158 [details]
Much simpler fix, addressed reviewer feedback

[Dana Jansens]

Comment on attachment 134158 [details]
Much simpler fix, addressed reviewer feedback

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:371
>          renderSurface->setClipRect(layer->parent() ? layer->parent()->clipRect() : layer->clipRect());

Sorry for the possible backtracking, but I think that this still going to be empty for parents with usesLayerClipping() == false?

I see the test verifyClipRectIsPropagatedCorrectlyToSurfaces checks explicitly for when the parent->usesLayerClipping() == true. Should code always guard using a surface clipRect on this and otherwise use its parents entire bounds? Or can this be set for surfaces with parents without usesLayerClipping() later, when the clipRect for the owning layer is set? Wondering what your thinking is in this regard..

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:415
> +    if (layer->masksToBounds()) {

random question: why is this only conditional on masksToBounds() but later we clip the drawableContentRect conditional on masksToBounds() || maskLayer()?

[Shawn Singh]

(In reply to comment #32)
> (From update of attachment 134158 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134158&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:371
> >          renderSurface->setClipRect(layer->parent() ? layer->parent()->clipRect() : layer->clipRect());
> 
> Sorry for the possible backtracking, but I think that this still going to be empty for parents with usesLayerClipping() == false?


OK... I have combed through all instances of clipRect in platform/graphics/chromium and platform/graphics/chromium/cc again, just to make everything clear:

(1) for Quads: sharedQuadState clipRect is empty by convention when it should not be used.  With this convention it looks like all the drawing code works as desired.

(2) for Layers: with this patch, there is only one place in the code that the layer's clipRect is being used without checking usesLayerClipping, and that's int the culling code.  I forgot to change that and need to submit a new patch for this bug.

before this patch, layers were accidentally using clipRect without checking usesLayerClipping; it was very subtle, but it was right under our noses in the masksToBounds condition.

(3) for surfaces: in calcDrawTransformsAndVisibility, when there is a replica, the surface clipRect is being used... if it is NOT empty.  so we're OK in that case.   in the case where the surface clipRect is used to compute the scissor, it is protected by the ancestor layer's usesLayerClipping(), which is also correct.

So yeah, that was a really good question and I'm glad to have combed through everything again, but I do feel it is correct.   I also agree that this confusion is just another strong indicator that we need to clean it up, and the appropriate bugs already exist to do that soon.

> 
> I see the test verifyClipRectIsPropagatedCorrectlyToSurfaces checks explicitly for when the parent->usesLayerClipping() == true. Should code always guard using a surface clipRect on this and otherwise use its parents entire bounds? Or can this be set for surfaces with parents without usesLayerClipping() later, when the clipRect for the owning layer is set? Wondering what your thinking is in this regard..
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:415
> > +    if (layer->masksToBounds()) {
> 
> random question: why is this only conditional on masksToBounds() but later we clip the drawableContentRect conditional on masksToBounds() || maskLayer()?

I think its precarious (as usual) but correct:

drawableContentRect can be affected by layer->maskLayer, if we assume that everything outside the bounds of the mask are masked out anyway, then we can shrink the drawable area to just the mask.

I don't think we can do the same for clipRect, because it is inherited by other layers that may be in some completely different position outside of the layer's bounds.

[Shawn Singh]

Created attachment 134198 [details]
Same patch but including the occlusion tracker fix

[Dana Jansens]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 134158 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134158&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:371
> > >          renderSurface->setClipRect(layer->parent() ? layer->parent()->clipRect() : layer->clipRect());
> > 
> > Sorry for the possible backtracking, but I think that this still going to be empty for parents with usesLayerClipping() == false?
> 
> 
> OK... I have combed through all instances of clipRect in platform/graphics/chromium and platform/graphics/chromium/cc again, just to make everything clear:
> 
> (1) for Quads: sharedQuadState clipRect is empty by convention when it should not be used.  With this convention it looks like all the drawing code works as desired.

k. So quad->clipRect() use should be guarded by checking isEmpty().

> (2) for Layers: with this patch, there is only one place in the code that the layer's clipRect is being used without checking usesLayerClipping, and that's int the culling code.  I forgot to change that and need to submit a new patch for this bug.

Ok, so all layer->clipRect() use should be guarded by layer->usesLayerClipping().

> before this patch, layers were accidentally using clipRect without checking usesLayerClipping; it was very subtle, but it was right under our noses in the masksToBounds condition.
> 
> (3) for surfaces: in calcDrawTransformsAndVisibility, when there is a replica, the surface clipRect is being used... if it is NOT empty.  so we're OK in that case.   in the case where the surface clipRect is used to compute the scissor, it is protected by the ancestor layer's usesLayerClipping(), which is also correct.

ok, so all surface->clipRect() use should be guarded by surface->owningLayer->parent->usesLayerClipping().

> So yeah, that was a really good question and I'm glad to have combed through everything again, but I do feel it is correct.   I also agree that this confusion is just another strong indicator that we need to clean it up, and the appropriate bugs already exist to do that soon.

Thanks for doing that. Can we stick a comment on each of the clipRect() functions stating what check must be done to guard its use?

> > I see the test verifyClipRectIsPropagatedCorrectlyToSurfaces checks explicitly for when the parent->usesLayerClipping() == true. Should code always guard using a surface clipRect on this and otherwise use its parents entire bounds? Or can this be set for surfaces with parents without usesLayerClipping() later, when the clipRect for the owning layer is set? Wondering what your thinking is in this regard..
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:415
> > > +    if (layer->masksToBounds()) {
> > 
> > random question: why is this only conditional on masksToBounds() but later we clip the drawableContentRect conditional on masksToBounds() || maskLayer()?
> 
> I think its precarious (as usual) but correct:
> 
> drawableContentRect can be affected by layer->maskLayer, if we assume that everything outside the bounds of the mask are masked out anyway, then we can shrink the drawable area to just the mask.
> 
> I don't think we can do the same for clipRect, because it is inherited by other layers that may be in some completely different position outside of the layer's bounds.

I don't think that I understand the distinction here. AIUI the mask should affect its subtree, so being in a different position should cause them to be clipped.

http://www.webkit.org/blog/181/css-masks/ is pretty explicit: "The mask will therefore overlay the child and all of its descendants, and at a minimum will knock out everything outside the border box of the object."

Also svg masks seem similar. More links:

http://developer.apple.com/library/safari/#documentation/InternetWeb/Conceptual/SafariVisualEffectsProgGuide/Masks/Masks.html
http://www.w3.org/TR/2000/03/WD-SVG-20000303/masking.html#Masking

[Shawn Singh]

Created attachment 134339 [details]
added comments about clipRect usage

Dana and I discussed offline, it felt acceptable to put these comments in .h files, rather than instrumenting every new location where clipRect is used

[Shawn Singh]

Created attachment 134342 [details]
added comments about clipRect usage

Dana and I discussed offline, it felt acceptable to put these comments in .h files, rather than instrumenting every new location where clipRect is used

[Dana Jansens]

Comment on attachment 134342 [details]
added comments about clipRect usage

Thanks for the comments :) I'm happy with this.

[Adrienne Walker]

Comment on attachment 134342 [details]
added comments about clipRect usage

Can you rebaseline this patch so EWS can take a look at it?

[Shawn Singh]

Created attachment 134362 [details]
updated to tip of tree

[Shawn Singh]

re-ran unit and layout tests once more, just for good measure.  still no obvious regressions =)

[Adrienne Walker]

Comment on attachment 134362 [details]
updated to tip of tree

Ok, sure.  Please watch this carefully on landing.  :)

[Shawn Singh]

Committed r112436: <http://trac.webkit.org/changeset/112436>