Artifacts in transformed linear gradients on Mac

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
x86
macOS
Points:
---
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(3 attachments)

Posted file 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.
Flags: wanted1.9.2+
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.
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?
Indeed, hacking the code to force a fixed number of repetitions makes a given gradient render consistently.
Posted patch CleanupSplinter Review
Clean up a little bit by moving some constant local variables to static const globals.
Attachment #392914 - Flags: review?(jmuizelaar)
Posted patch fixSplinter Review
-- 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
Attachment #392919 - Flags: review?(jmuizelaar)
Whiteboard: [needs review]
Attachment #392914 - Flags: review?(jmuizelaar) → review+
Attachment #392919 - Flags: review?(jmuizelaar) → review+
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 on attachment 392919 [details] [diff] [review]
fix

Makes CSS gradients work better
Attachment #392919 - 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.
I landed those changes as changeset 9f811e15ca51.
Whiteboard: [needs 192 approval]
roc: do you still think we need to take these on 1.9.2?
Already landed on 1.9.2 in commit ebbeedee7a9a
Whiteboard: [needs 192 approval]
You need to log in before you can comment on or make changes to this bug.