Closed Bug 508227 Opened 12 years ago Closed 12 years ago

Don't fall back to pixman for repeating radial gradients on Mac


(Core :: Graphics, defect)

Not set



Tracking Status
status1.9.2 --- beta4-fixed


(Reporter: roc, Assigned: roc)



(2 files, 2 obsolete files)

I think we can use CGShading to do repeating radial gradients, at least for cases where the outer circle fully encloses the inner circle. If I understand radial gradients correctly, we can just expand the outer circle parameter, setting it to be one of the repeated outer circles that fully encloses the clipBox, and then set the color function to repeat color stops in the right places (like we already do for linear gradients).

I may give this a go.
Flags: wanted1.9.2?
Attached patch fix (obsolete) — Splinter Review
This does it.

Figuring out where the outer circle should move to is tricky. I hope the algebra in there is understandable.

This is a nice performance improvement, probably because we avoid painting the gradient over the entire clipBox (which is usually the entire surface).

I tried to write reftests that compared a repeating radial gradient to a non-repeating gradient with manually repeated stops, but it didn't work because the rasterization was slightly different --- I'm not sure why.

This patch also forces us to use pixman for all degenerate cases where the circles intersect. This at least makes us consistent across platforms.
Assignee: nobody → roc
Attachment #392864 - Flags: review?(jmuizelaar)
Whiteboard: [needs review]
Attached patch fix v2 (obsolete) — Splinter Review
Better fix ... transform clip into cairo user space before we do all these tests that assume it's in user space.
Attachment #392864 - Attachment is obsolete: true
Attachment #392878 - Flags: review?(jmuizelaar)
Attachment #392864 - Flags: review?(jmuizelaar)
Attached patch fix v3Splinter Review
Oops, there was some stuff missing because I mismanaged my patches.

I'm going to change the way we compute the extents for the radial gradient, in bug 508730. But that builds on this.
Attachment #392878 - Attachment is obsolete: true
Attachment #392913 - Flags: review?(jmuizelaar)
Attachment #392878 - Flags: review?(jmuizelaar)
Comment on attachment 392913 [details] [diff] [review]
fix v3

I know the style in this file is not that consistent but using camel-case stuck out to me as
foreign. I don't feel that strongly about it though.

>+static void
>+UpdateRadialParameterToIncludePoint(double *max_t, CGPoint *center,
>+                                    double dr, double dx, double dy,
>+                                    double x, double y)


>@@ -1142,52 +1283,68 @@ _cairo_quartz_setup_radial_source (cairo
>+    double c1x = _cairo_fixed_to_double (rpat->c1.x);
>+    double c1y = _cairo_fixed_to_double (rpat->c1.y);
>+    double c2x = _cairo_fixed_to_double (rpat->c2.x);
>+    double c2y = _cairo_fixed_to_double (rpat->c2.y);
>+    double r1 = _cairo_fixed_to_double (rpat->r1);
>+    double r2 = _cairo_fixed_to_double (rpat->r2);
>+    double dx = c1x - c2x;
>+    double dy = c1y - c2y;
>+    double centerDistance = sqrt (dx*dx + dy*dy);

you could use hypot(dx, dy) here depending on how you want to trade off accuracy vs. performance.
Attachment #392913 - Flags: review?(jmuizelaar) → review+
The test failures are because we're now falling back to pixman for degenerate radial gradient cases (where neither circle contains the other), and pixman fails these tests --- they were only run on Mac. I've filed bug 512647 for these pixman failures. I think in the meantime it's reasonable to disable them on Mac as well; at least my patch here makes us consistent across platforms.
Attachment #396680 - Flags: review?(jmuizelaar)
Attachment #396680 - Flags: review?(jmuizelaar) → review+
Comment on attachment 392913 [details] [diff] [review]
fix v3

Makes CSS gradients work better
Attachment #392913 - Flags: approval1.9.2?
What prevents this fix from getting lost when we next update cairo?  I don't see any changes to gfx/cairo/README or any tests that would catch such a regression.
Sorry, I have a patch in my queue that adds to gfx/cairo/README but I forgot to land it.
That patch landed as changeset 9f811e15ca51.
Whiteboard: [needs 192 lanid
Whiteboard: [needs 192 lanid → [needs 192 approval]
Do we still want these on 1.9.2?
This already landed on 1.9.2 in commit ebbeedee7a9a.
Flags: wanted1.9.2? → wanted1.9.2+
Whiteboard: [needs 192 approval]
You need to log in before you can comment on or make changes to this bug.