581089 - REFTEST TEST-UNEXPECTED-FAIL | css-gradients/repeating-*.html |

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Quite a bit of difference here. Looks like repeating gradients might be broken.

repeating-radial-1a and repeating-linear-1b also fail.

Comment 1

[Bas Schouten (:bas.schouten)]

I'm not surprised. D2D has no notion of repeating linear gradients. I'm also not sure how that's expected to be bound? By the bounds perpendicular to the gradient line at the first and last stop?

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(Since those other D2D-reftest bugs all had Win7 file URIs in the testnames, I assume the correct platform for these two is also Win7.)

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Fix radial gradient support

This patch fixes radial gradient support. For repeat and reflect it generates a gradient large enough to cover the target destination. EXTEND_NONE and inner radius of != 0 are also fixed with this patch. The one downside is that for large gradients (i.e. size of a window) D2D seems to rasterize to smaller surface and scale up. Causing repeat gradient's hard stops to look slightly fuzzier in a larger window. Since the code will create a larger gradient to ensure it's repeated sufficiently.

Similar work for linear gradients still needs to be done.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460187 [details] [diff] [review]
Part 1: Fix radial gradient support

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>@@ -869,16 +869,189 @@ _cairo_d2d_calculate_visible_rect(cairo_
> 	    *height = 1;
> 	}
>     } else {
> 	*y = 0;
> 	*height = srcSurf->height;
>     }
> }
> 

How about point_dist() instead of point_diff()?

>+static double
>+_cairo_d2d_point_diff(const cairo_point_double_t &p1, const cairo_point_double_t &p2)
>+{
>+    return sqrt(pow(p2.x - p1.x, 2) + pow(p2.y - p1.y, 2));
>+}
>+
>+static RefPtr<ID2D1Brush>
>+_cairo_d2d_create_radial_gradient_brush(cairo_d2d_surface_t *d2dsurf,
>+					cairo_radial_pattern_t *sourcePattern)

I would prefer if cairo typed variables had cairo style names (underscores instead of camelcase)
The backend currently doesn't have the most consistent style what would you think of adopting something like:
- cairo typed variables use cairo style
- D2D typed variables use D2D style


>+{
>+    cairo_matrix_t mat = sourcePattern->base.base.matrix;
>+    cairo_matrix_invert(&mat);
>+    D2D1_BRUSH_PROPERTIES brushProps =
>+	D2D1::BrushProperties(1.0, _cairo_d2d_matrix_from_matrix(&mat));
>+
>+    if ((sourcePattern->c1.x != sourcePattern->c2.x ||
>+	sourcePattern->c1.y != sourcePattern->c2.y) &&
>+	sourcePattern->r1 != 0) {
>+	    /**
>+	     * In this particular case there's no way to deal with this!
>+	     * \todo Create an image surface with the gradient and use that.
>+	     */
>+	    return NULL;
>+    }
>+
>+    D2D_POINT_2F center =
>+	_d2d_point_from_cairo_point(&sourcePattern->c2);
>+    D2D_POINT_2F origin =
>+	_d2d_point_from_cairo_point(&sourcePattern->c1);
>+    origin.x -= center.x;
>+    origin.y -= center.y;
>+
>+    FLOAT outer_radius = _cairo_fixed_to_float(sourcePattern->r2);
>+    FLOAT inner_radius = _cairo_fixed_to_float(sourcePattern->r1);
>+    int num_stops = sourcePattern->base.n_stops;
>+    int repeat_count = 1;
>+    D2D1_GRADIENT_STOP *stops;
>+
>+    if (sourcePattern->base.base.extend == CAIRO_EXTEND_REPEAT || sourcePattern->base.base.extend == CAIRO_EXTEND_REFLECT) {
>+	bool reflected = true;
>+	bool reflect = sourcePattern->base.base.extend == CAIRO_EXTEND_REFLECT;
>+
>+	RefPtr<IDXGISurface> surf;
>+	d2dsurf->surface->QueryInterface(&surf);
>+	DXGI_SURFACE_DESC desc;
>+	surf->GetDesc(&desc);
>+
>+	cairo_matrix_t inv_mat = sourcePattern->base.base.matrix;
>+	if (_cairo_matrix_is_invertible(&inv_mat)) {
>+	    /* If this is not invertible it will be rank zero, and invMat = mat is fine */
>+	    cairo_matrix_invert(&inv_mat);
>+	}
>+
>+	// Calculate the largest distance the origin could be from the edge after inverse
>+	// transforming by the pattern transformation.
>+	cairo_point_double_t top_left, top_right, bottom_left, bottom_right, gradient_center;
>+	top_left.x = bottom_left.x = top_left.y = top_right.y = 0;
>+	top_right.x = bottom_right.x = desc.Width;
>+	bottom_right.y = bottom_left.y = desc.Height;
>+	gradient_center.x = _cairo_fixed_to_float(sourcePattern->c1.x);
>+	gradient_center.y = _cairo_fixed_to_float(sourcePattern->c1.y);
>+	cairo_matrix_transform_point(&inv_mat, &top_left.x, &top_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &top_right.x, &top_right.y);
>+	cairo_matrix_transform_point(&inv_mat, &bottom_left.x, &bottom_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &bottom_right.x, &top_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &gradient_center.x, &gradient_center.y);
>+	double largest = MAX(_cairo_d2d_point_diff(top_left, gradient_center), _cairo_d2d_point_diff(top_right, gradient_center));
>+	largest = MAX(largest, _cairo_d2d_point_diff(bottom_left, gradient_center));
>+	largest = MAX(largest, _cairo_d2d_point_diff(bottom_right, gradient_center));
>+
>+	unsigned int minSize = (unsigned int)ceil(largest);
>+
>+	// Calculate how often we need to repeat on the inside (for filling the inner radius)
>+	// and on the outside (for covering the entire surface) and create the appropriate number
>+	// of stops.
>+	float r_covered = outer_radius - inner_radius;

Any reason you switched from FLOAT to float? personally I prefer 'float' but consistency is important too.
How about 'gradient_length' instead of 'r_covered'?

>+	int inner_repeat = ceil(inner_radius / r_covered);
>+	int outer_repeat = MAX(1, (float)(minSize - inner_radius) / r_covered);
>+	num_stops *= (inner_repeat + outer_repeat);
>+	stops = new D2D1_GRADIENT_STOP[num_stops];
>+	outer_radius = (inner_repeat + outer_repeat) * r_covered;

could use a comment about this outer_radius is different from the earlier outer_radius

>+
>+	FLOAT stop_scale = 1.0f / (inner_repeat + outer_repeat);
>+
>+	float inner_position = (inner_repeat * r_covered) / outer_radius;
>+	for (int i = inner_repeat * sourcePattern->base.n_stops - 1; i >= 0; i--) {
>+	    int repeat = inner_repeat - (i / sourcePattern->base.n_stops);
>+	    int stop = i % sourcePattern->base.n_stops;
>+	    if (reflected) {
>+		stop = sourcePattern->base.n_stops - stop - 1;
>+		stops[i].position = inner_position - (repeat - 1.0f + (float)sourcePattern->base.stops[stop].offset) * stop_scale;
>+	    } else {
>+		stops[i].position = inner_position - (repeat - (float)sourcePattern->base.stops[stop].offset) * stop_scale;
>+	    }
>+	    stops[i].color =
>+		_cairo_d2d_color_from_cairo_color(sourcePattern->base.stops[stop].color);
>+	    if (!(i % sourcePattern->base.n_stops)) {
>+		// Reflect here
>+		reflected = !reflected && reflect;
>+	    }
>+	}
>+	// We reflect at stop 0 - start out reflected.
>+	reflected = true;
>+	for (int i = inner_repeat * sourcePattern->base.n_stops; i < num_stops; i++) {
>+	    if (!(i % sourcePattern->base.n_stops)) {
>+		// Reflect here
>+		reflected = !reflected && reflect;
>+	    }
>+	    int repeat = (i - inner_repeat * sourcePattern->base.n_stops) / sourcePattern->base.n_stops;
>+	    int stop = i % sourcePattern->base.n_stops;
>+	    if (reflected) {
>+		stop = sourcePattern->base.n_stops - stop - 1;
>+		stops[i].position = inner_position + (repeat + 1.0f - (float)sourcePattern->base.stops[stop].offset) * stop_scale;
>+	    } else {
>+		stops[i].position = inner_position + (repeat + (float)sourcePattern->base.stops[stop].offset) * stop_scale;
>+	    }
>+	    stops[i].color =
>+		_cairo_d2d_color_from_cairo_color(sourcePattern->base.stops[stop].color);
>+	}

This loop is pretty complicated. Perhaps it would be simpler to split it out into separate repeat/reflect cases. It might also be worth seeing if using an accumulator value for the position is simpler than the big expression above. More explanation about what it's doing wouldn't hurt either :)

>+    } else if (sourcePattern->base.base.extend == CAIRO_EXTEND_PAD || sourcePattern->base.base.extend == CAIRO_EXTEND_NONE) {

I think this code might be clearer if you split out the EXTEND_PAD and EXTEND_NONE cases instead of interleaving them

>+	float offset_factor = 1.0f;
>+	float global_offset = 0;
>+        
>+	if (inner_radius != 0) {
>+	    offset_factor = (outer_radius - inner_radius) / outer_radius;
>+	    global_offset = inner_radius / outer_radius;
>+	}
>+	if (sourcePattern->base.base.extend == CAIRO_EXTEND_NONE) {
>+	    num_stops += 1;
>+	    if (inner_radius != 0) {
>+		num_stops++;
>+	    }
>+	}
>+
>+
>+	stops = new D2D1_GRADIENT_STOP[num_stops];
>+	D2D1_GRADIENT_STOP *set_stops = stops;
>+        
>+	// If the inner radius is not 0 we need to scale and offset the stops and put a stop at position 0
>+	// of the same color as the position 0 stop.
>+	if (sourcePattern->base.base.extend == CAIRO_EXTEND_NONE && inner_radius != 0) {
>+	    set_stops->position = global_offset;
>+	    set_stops->color = D2D1::ColorF(0, 0);
>+	    set_stops++;
>+	}
>+	for (unsigned int i = 0; i < sourcePattern->base.n_stops; i++) {
>+	    set_stops->position = global_offset + (FLOAT)sourcePattern->base.stops[i].offset * offset_factor;
>+	    set_stops->color = 
>+		_cairo_d2d_color_from_cairo_color(sourcePattern->base.stops[i].color);
>+	    set_stops++;
>+	}
>+	if (sourcePattern->base.base.extend == CAIRO_EXTEND_NONE) {
>+	    set_stops->position = 1.0f;
>+	    set_stops->color = D2D1::ColorF(0, 0);
>+	}
>+    } else {
>+	return NULL;
>+    }

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Fix radial gradient support v2

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> Comment on attachment 460187 [details] [diff] [review]
> I would prefer if cairo typed variables had cairo style names (underscores
> instead of camelcase)
> The backend currently doesn't have the most consistent style what would you
> think of adopting something like:
> - cairo typed variables use cairo style
> - D2D typed variables use D2D style
 
I like it!

Comment 7

[Bas Schouten (:bas.schouten)]

There's a flaw in the patch where inner_repeat should be outer_repeat in a bunch of cases. Will upload a new patch tomorrow.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to comment #7)
> There's a flaw in the patch where inner_repeat should be outer_repeat in a
> bunch of cases. Will upload a new patch tomorrow.

I was mistaking, I believe the patch is correct.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460337 [details] [diff] [review]
Part 1: Fix radial gradient support v2

A couple more comments


>+static double
>+_cairo_d2d_point_dist(const cairo_point_double_t &p1, const cairo_point_double_t &p2)
>+{
>+    return sqrt(pow(p2.x - p1.x, 2) + pow(p2.y - p1.y, 2));
>+}

I lost this comment last review, but using:
return hypot(p2.x-p1.x, p2.y - p1.y)

will be faster and won't suffer from overflow problems like the above.

>+
>+	// Calculate the largest distance the origin could be from the edge after inverse
>+	// transforming by the pattern transformation.
>+	cairo_point_double_t top_left, top_right, bottom_left, bottom_right, gradient_center;
>+	top_left.x = bottom_left.x = top_left.y = top_right.y = 0;
>+	top_right.x = bottom_right.x = desc.Width;
>+	bottom_right.y = bottom_left.y = desc.Height;
>+	gradient_center.x = _cairo_fixed_to_float(source_pattern->c1.x);
>+	gradient_center.y = _cairo_fixed_to_float(source_pattern->c1.y);
>+	cairo_matrix_transform_point(&inv_mat, &top_left.x, &top_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &top_right.x, &top_right.y);
>+	cairo_matrix_transform_point(&inv_mat, &bottom_left.x, &bottom_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &bottom_right.x, &top_left.y);
>+	cairo_matrix_transform_point(&inv_mat, &gradient_center.x, &gradient_center.y);
>+	double largest = MAX(_cairo_d2d_point_dist(top_left, gradient_center), _cairo_d2d_point_dist(top_right, gradient_center));
>+	largest = MAX(largest, _cairo_d2d_point_dist(bottom_left, gradient_center));
>+	largest = MAX(largest, _cairo_d2d_point_dist(bottom_right, gradient_center));

This block of code is quite dense. Can you space it out some.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Fix radial gradient support v3

Updated with comments and also to properly transform everything to gradient space.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460557 [details] [diff] [review]
Part 1: Fix radial gradient support v3


>+	float inner_position = (inner_repeat * gradient_length) / outer_radius;
>+	if (reflect) {
>+	    // We start out reflected.
>+	    reflected = false;
>+	    for (int i = inner_repeat * source_pattern->base.n_stops - 1; i >= 0; i--) {

Can we just loop from i=0 to n_stops? instead of doing it in two parts?

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Fix radial gradient support v4

The loops could indeed be merged relatively easily, done so.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460631 [details] [diff] [review]
Part 1: Fix radial gradient support v4


>+    } else if (source_pattern->base.base.extend == CAIRO_EXTEND_NONE) {
>+	float offset_factor = 1.0f;
>+	float global_offset = 0;
>+        
>+        num_stops++;
>+	if (inner_radius != 0) {
>+	    offset_factor = (outer_radius - inner_radius) / outer_radius;
>+	    global_offset = inner_radius / outer_radius;

Why not just initialize these unconditionally to the formula version instead
of special casing inner_radius == 0?

>+	    num_stops++;
>+	}
>+
>+	stops = new D2D1_GRADIENT_STOP[num_stops];

Looks like we leak the originally allocated stops here. 

>+	D2D1_GRADIENT_STOP *set_stops = stops;

Why can't we just use stops directly?

>+        
>+	// If the inner radius is not 0 we need to scale and offset the stops and put a stop before the inner_radius
>+	// of a transparent color.
>+	if (inner_radius != 0) {
>+	    set_stops->position = global_offset;
>+	    set_stops->color = D2D1::ColorF(0, 0);
>+	    set_stops++;
>+	}
>+	for (unsigned int i = 0; i < source_pattern->base.n_stops; i++) {

Are the loop bounds correct? I'm not convinced that:
(source_pattern->base.n_stops + (inner_radius != 0)) == num_stops

>+	    set_stops->position = global_offset + (FLOAT)source_pattern->base.stops[i].offset * offset_factor;
>+	    set_stops->color = 
>+		_cairo_d2d_color_from_cairo_color(source_pattern->base.stops[i].color);
>+	    set_stops++;
>+	}
>+	set_stops->position = 1.0f;
>+	set_stops->color = D2D1::ColorF(0, 0);
>+    } else {
>+	return NULL;
>+    }

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Fix radial gradient support v5

Updated as per IRC conversation and comments. Mercurial played some tricks on me so I hope nothing got distorted, it looks good.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460737 [details] [diff] [review]
Part 1: Fix radial gradient support v5

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+		    // When reflected take 1 - offset as the offset.
>+		    stops[i].position = (FLOAT)((repeat + 1.0f - source_pattern->base.stops[stop].offset) * stop_scale);

Perhaps factor out source_pattern->base.stops[stop].


>+        num_stops++; // Add a stop on the inner radius.
>+	if (inner_radius != 0) {
>+	    num_stops++; // Add a stop on the outer radius.
>+	}

Seems like the comments have radius names reversed.

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Fix linear gradient support

This patch fixes the issues with linear gradients. However it will not make the reftests pass due to bug 582236. The reftests could be adjusted somewhat so they'd pass, but I'm unsure if that's desirable.

Also some code-share could be made between the stop creation for radial and linear, but since it's not that complicated I chose not to. This is still possible.

Comment 17

[Bas Schouten (:bas.schouten)]

Need blocking to be allowed to push.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

blocks a blocker, go hard.

Comment 19

[Bas Schouten (:bas.schouten)]

Part 1 pushed http://hg.mozilla.org/mozilla-central/rev/cd5ccea5c002.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 460893 [details] [diff] [review]
Part 2: Fix linear gradient support

>+static void
>+_cairo_d2d_normalize_point(cairo_point_double_t *p)
>+{
>+    double length = hypot(p->x, p->y);
>+    p->x /= length;
>+    p->y /= length;
>+}
>+
>+static cairo_point_double_t
>+_cairo_d2d_subtract_point(const cairo_point_double_t &p1, const cairo_point_double_t &p2)
>+{
>+    cairo_point_double_t p = {p1.x - p2.x, p1.y - p2.y};
>+    return p;
>+}
>+
>+static double
>+_cairo_d2d_dot_product(const cairo_point_double_t &p1, const cairo_point_double_t &p2)
>+{
>+    return p1.x * p2.x + p1.y * p2.y;
>+}
>+

These should be generic functions, but we can probably leave them here for now.

Comment 21

[Bas Schouten (:bas.schouten)]

Pushed part 2: http://hg.mozilla.org/mozilla-central/rev/06b578dfadc9. Not resolving this since the reftests do not pass yet, see bug 582236.

Comment 22

[Bas Schouten (:bas.schouten)]

Resolving this wontfix, although the reftests look acceptable now, I don't think we will completely be able to fix them with D2D. I believe the consensus was this is acceptable. The majority of the problem has been resolved.