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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

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

Attachment: fix

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.

Comment 2

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

Attachment: fix v2

Better fix ... transform clip into cairo user space before we do all these tests that assume it's in user space.

Comment 3

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

Attachment: fix v3

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.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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)
>+{

[snip]

>@@ -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.

Comment 5

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

Checked in but backed out due to test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1251238468.1251248064.24764.gz

Comment 6

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

Attachment: disable failing gradient tests

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.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/e564f3ab4ea6
http://hg.mozilla.org/mozilla-central/rev/0dc906f51445

Comment 8

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

Comment on attachment 392913 [details] [diff] [review]
fix v3

Makes CSS gradients work better

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

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

Sorry, I have a patch in my queue that adds to gfx/cairo/README but I forgot to land it.

Comment 11

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

That patch landed as changeset 9f811e15ca51.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we still want these on 1.9.2?

Comment 13

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

This already landed on 1.9.2 in commit ebbeedee7a9a.