628745 - skip rounded rect clips whenever possible

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Rounded rect clips are slow, and a good portion of the time they are useless as their rounded corners don't clip anything. We should replace them with their non-rounded rect cousins whenever possible.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (!mHaveClipRect)
+    return;
+
+  mClipRect = NonRoundedIntersection();
+  mRoundedClipRects.Clear();

The early returns for !mHaveClipRect seem unnecessary. How about
  if (mRoundedClipRects.IsEmpty())
    return;
  mClipRect = NonRoundedIntersection();

and have NonRoundedIntersection skip using mClipRect if !mHaveClipRect? Assert in NonRoundedIntersection that !mRoundedClipRects.IsEmpty() instead of checking mHaveClipRect. IsRectClippedByRoundedCorner can exit early if !mRoundedClipRects.IsEmpty() too.

I think you need to do something for zero radii here. Right now it looks like we can die in IsInsideEllipse for zero radii. I think changing
+    if (rect.x <= rr.mRect.x + rr.mRadii[NS_CORNER_TOP_LEFT_X] &&
+        rect.y <= rr.mRect.y + rr.mRadii[NS_CORNER_TOP_LEFT_Y]) {
to use < (and similar changes too) would avoid that, and optimize a little bit more too.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

(In reply to comment #2)
> and have NonRoundedIntersection skip using mClipRect if !mHaveClipRect?

The way it is coded right now if mHaveClipRect is true then both mClipRect and mRoundedClipRects are valid (http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1782), so if mRoundedClipRects is not empty then mClipRect must be present and valid.

Otherwise made the other changes.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: interdiff

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 506877 [details] [diff] [review]
patch v2

Please make sure we have a test for zero-radii.

Comment 6

[Timothy Nikkel (:tnikkel)]

Just a test that we execute this code with zero-radii and don't crash?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes --- a test that would have crashed with the first version of the patch :-).

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

Added a test.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 506889 [details] [diff] [review]
patch v3

Let's get this in ASAP!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This might make bug 601512 much less common.

Comment 11

[Timothy Nikkel (:tnikkel)]

Try server of an intermediate version of this patch was green. Try server run of final patch is running now. I'll probably land tomorrow unless someone is landing late tonight and wants to land this.

Comment 12

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/8149e1a06476