508730 - Artifacts in transformed linear gradients on Mac

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: testcase

In the attached testcase, if you scroll up and down a bit you can see that successive repaints of the linear gradient in the top DIV are giving inconsistent results, creating visual artifacts. You can also see that even in the stationary case the stops look somewhat uneven.

Also, you can see that the degenerate case of a zero-length vector is handled by Quartz by painting nothing, whereas on X we fill with the color of the first stop.

Comment 1

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

I thought I had a fix for this, but it turns out I don't. I don't know what's causing it. Maybe Quartz is just incorrigible.

Comment 2

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

OK, I think I understand this better now.

It seems Quartz precomputes the values of the color function for some fixed number of input values chosen from the input range domain. So changing the values of the start/end points and input range changes the rasterization of the entire gradient, and choosing very far-apart start/end points makes the gradient lose internal resolution. Since the current code depends on the target surface extents, and the surface extents depend on the dirty rect, we get different input ranges and different gradient rasterizations.

The same problem applies to radial gradients, although it's harder to spot the inconsistencies in curved edges.

I think the right fix here for _cairo_quartz_surface_fill, _cairo_quartz_surface_stroke and _cairo_quartz_surface_show_glyphs is to base the computed input range and values of the start/end points on the extents of the object we're filling/stroking. For _cairo_quartz_surface_paint I'm not sure what we should do... perhaps use the maximum of the surface extents and some predefined "large" extents (e.g 1024x1024), so that we get consistent results for all surfaces below that size?

Comment 3

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

Indeed, hacking the code to force a fixed number of repetitions makes a given gradient render consistently.

Comment 4

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

Attachment: Cleanup

Clean up a little bit by moving some constant local variables to static const globals.

Comment 5

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

Attachment: fix

-- Add a parameter to _cairo_quartz_setup_source so we can pass down the extents of the object we're drawing
-- Compute fill/stroke/glyph extents and pass them down in the cases we need to (repeating/reflecting gradients)
-- Pass those extents on down to where we set up the gradients
-- Make _cairo_quartz_setup_linear_source fall back to pixman for the degenerate case where the linear gradient vector has no length
-- In CreateRepeatingRadialGradientFunction and CreateRepeatingLinearGradientFunction, use the object extents (or surface extents, for the paint() case) instead of the clip box to calculate the parameters for the gradient
-- I've changed the way CreateRepeatingLinearGradientFunction calculates the repetition count. The new approach gives much more precise bounds on the number of repetitions needed (and is very similar to what we do for radial gradients). This is important because if we specify a much larger input range than we really need for our gradient color function, Quartz samples it too coarsely over the range we actually care about, and the gradients look bad.

For example, suppose start = (5,0), end = (6,10), the CTM is identity and the bounds we want to cover is (0,0)-(10,10). I think the current approach sets up the gradient to be repeated 10 times. In fact only 3 repetitions are needed.

Also, using 'width' here didn't look right:
-       y_rep_end = (int) ceil((surface->extents.width - MAX(mstart.y, mend.y)) / dy

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 392919 [details] [diff] [review]
fix

It might be good to have a comment here explaining why these sources need
extents.

>+    return (source->type == CAIRO_PATTERN_TYPE_LINEAR ||
>+            source->type == CAIRO_PATTERN_TYPE_RADIAL) &&
>+           (source->extend == CAIRO_EXTEND_REPEAT ||
>+            source->extend == CAIRO_EXTEND_REFLECT);
>+}
>+

[snip]

>     CGContextBeginPath (surface->cgContext);
> 
>     stroke.cgContext = surface->cgContext;
>     stroke.ctm_inverse = NULL;
>     rv = _cairo_quartz_cairo_path_to_quartz_context (path, &stroke);
>     if (rv)
>         goto BAIL;
>@@ -2059,17 +2114,24 @@ _cairo_quartz_surface_stroke (void *abst
> 
> 	CGContextSetLineDash (surface->cgContext, style->dash_offset, fdash, max_dashes);
> 	if (fdash != sdash)
> 	    free (fdash);
>     }
> 
>     CGContextSetCompositeOperation (surface->cgContext, _cairo_quartz_cairo_operator_to_quartz (op));
> 
>-    action = _cairo_quartz_setup_source (surface, source);
>+    if (_cairo_quartz_source_needs_extents (source))
>+    {

Perhaps add a comment explaining why the approximate extents are sufficient.

>+        cairo_rectangle_int_t path_extents;
>+        _cairo_path_fixed_approximate_stroke_extents (path, style, ctm, &path_extents);
>+        action = _cairo_quartz_setup_source (surface, source, &path_extents);
>+    } else {
>+        action = _cairo_quartz_setup_source (surface, source, NULL);
>+    }

It might also be useful to add a brief summary of the work we have to do to map cairo gradients to quartz gradients near the beginning of the CGShading creation.

Comment 7

[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 8

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

http://hg.mozilla.org/mozilla-central/rev/963b9451ad30
http://hg.mozilla.org/mozilla-central/rev/0bac4c903d2b

Comment 9

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

Comment on attachment 392919 [details] [diff] [review]
fix

Makes CSS gradients work better

Comment 10

[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 11

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

I landed those changes as changeset 9f811e15ca51.

Comment 12

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

roc: do you still think we need to take these on 1.9.2?

Comment 13

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

Already landed on 1.9.2 in commit ebbeedee7a9a