skip rounded rect clips whenever possible

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

9 years ago
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.
Assignee

Comment 1

9 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #506839 - Flags: review?(roc)
+  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.
Assignee

Comment 3

9 years ago
Posted patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #506839 - Attachment is obsolete: true
Attachment #506877 - Flags: review?(roc)
Attachment #506839 - Flags: review?(roc)
Assignee

Comment 4

9 years ago
Posted patch interdiff (obsolete) — Splinter Review
Comment on attachment 506877 [details] [diff] [review]
patch v2

Please make sure we have a test for zero-radii.
Attachment #506877 - Flags: review?(roc) → review+
Assignee

Comment 6

9 years ago
Just a test that we execute this code with zero-radii and don't crash?
Yes --- a test that would have crashed with the first version of the patch :-).
Assignee

Comment 8

9 years ago
Posted patch patch v3Splinter Review
Added a test.
Attachment #506877 - Attachment is obsolete: true
Attachment #506878 - Attachment is obsolete: true
Attachment #506889 - Flags: approval2.0?
Comment on attachment 506889 [details] [diff] [review]
patch v3

Let's get this in ASAP!
Attachment #506889 - Flags: approval2.0? → approval2.0+
This might make bug 601512 much less common.
Blocks: 601512
Assignee

Comment 11

9 years ago
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.
Blocks: 629012
Assignee

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8149e1a06476
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 613406
You need to log in before you can comment on or make changes to this bug.