We fail test_2d.gradient.radial.cone.top.html on windows (the test isn't run on mac) because skia appears to be handling these slightly differently. From the canvas spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#colors-and-styles "For all values of ω where r(ω) > 0, starting with the value of ω nearest to positive infinity and ending with the value of ω nearest to negative infinity, draw the circumference of the circle with radius r(ω) at position (x(ω), y(ω)), with the color at ω, but only painting on the parts of the canvas that have not yet been painted on by earlier circles in this step for this rendering of the gradient." So for the parts of the gradient where there are two valid solutions, we want the one that is closer to the larger circle. From SkGradientShader.cpp: " If a<0, the start circle is entirely contained in the end circle, and one of the roots will be <0 or >1 (off the line segment). If a>0, the start circle falls at least partially outside the end circle (or vice versa), and the gradient defines a "tube" where a point may be on one circle (on the inside of the tube) or the other (outside of the tube). We choose one arbitrarily." So I believe that we want to modify the behaviour in the a>=0 case to pick the correct root less-arbitrarily. The attached patch fixes the current test failures, without regressing anything, but doesn't seem right. Working on it.

This is using the same solution choices as pixman. I'm not sure what to do when both solutions correspond to negative radii though.

This needs more tidying up and comments. It now passes the full set of radial gradient tests from http://philip.html5.org

Passes 100% of the canvas gradient tests (linear and radial), now with comment changes!

Comment on attachment 589396 [details] [diff] [review] Make skia fully compliant with the canvas spec v2 It actually probably makes more sense for Bas to review this. As he's done radial gradient work before.

Bas: review ping for this. I realize that you don't know this code at all, but I unfortunately nobody else does either. Since we need these changes (or something equivalent) to pass canvas tests with Azure/Skia, I suggest that we take this patch for the time being, submit it upstream and we can back it out later if the skia team disagrees. George: Can you look at getting this upstreamed pretty please :)

(In reply to Matt Woodrow (:mattwoodrow) from comment #2) > It now passes the full set of radial gradient tests from > http://philip.html5.org Note that, in general, those tests are long out of date. Please see <http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/> for the maintained versions.

Rebased the patch against the new skia code. Passes all the gradient.* tests on the link provided by Ms2ger, except for 2d.gradient.object.current.html (which doesn't appear to be related to skia, since we never even get an nsCanvasRenderingContext2DAzure::FillRect call). Bas: Will you be able to review this? Or should I try find someone else.

I will look at this tomorrow.

Yep, 2d.gradient.object.current is bug 629882.

Still haven't fully wrapped my head around this. I'll try very much to have this done before the end of the weekend.

Comment on attachment 628137 [details] [diff] [review] Make skia fully compliant with the canvas spec v3 Review of attachment 628137 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this took so long! I'm a little worried about the large amount of branches involved in each pixel. I wonder if we should have a special-case for |dc| == 0, i.e. where A collapses to -(dr^2). Even more easy to special case (and probably the most common gradient?) would be |dc| == 0 && |r1| == 0 because that collapses A to -(dr^2) B to 0 and C to |dp| where dp is (dx, dy) (i.e. t is trivially calculated from the distance between the pixel and the gradient centers) ::: gfx/skia/src/effects/SkGradientShader.cpp @@ +1793,3 @@ > > + XXXmattwoodrow: I've removed this for now since it breaks > + down when Dr == 0. Is there something else we can do instead? We could special-case Dr == 0. It's quite trivial in the case where |dc| == 0. In the other case it's a little trickier. @@ +1866,5 @@ > int count) { > for (; count > 0; --count) { > + SkFixed t; > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > + sk_bzero(dstC, sizeof(*dstC)); Let's just sk_bzero(dstC++, sizeof(*dstC)) It's probably even better to set 'fully transparent', through an assignment to dstC as sk_bzero seems like it would cause needless overhead here. @@ +1892,5 @@ > int count) { > for (; count > 0; --count) { > + SkFixed t; > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > + sk_bzero(dstC, sizeof(*dstC)); Same comment as above. @@ +1915,5 @@ > int count) { > for (; count > 0; --count) { > + SkFixed t; > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > + sk_bzero(dstC, sizeof(*dstC)); Again @@ +2056,5 @@ > SkScalar b = (SkScalarMul(fDiff.fX, fx) + > SkScalarMul(fDiff.fY, fy) - fStartRadius) * 2; > + SkFixed t; > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > + sk_bzero(dstC, sizeof(*dstC)); Again

(In reply to Bas Schouten (:bas) from comment #11) > Comment on attachment 628137 [details] [diff] [review] > Make skia fully compliant with the canvas spec v3 > > Review of attachment 628137 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry this took so long! > > I'm a little worried about the large amount of branches involved in each > pixel. I wonder if we should have a special-case for |dc| == 0, i.e. where A > collapses to -(dr^2). > > Even more easy to special case (and probably the most common gradient?) > would be |dc| == 0 && |r1| == 0 because that collapses A to -(dr^2) B to 0 > and C to |dp| where dp is (dx, dy) (i.e. t is trivially calculated from the > distance between the pixel and the gradient centers) I've added this by calling into Skia's single point radial gradient code instead. > > ::: gfx/skia/src/effects/SkGradientShader.cpp > @@ +1793,3 @@ > > > > + XXXmattwoodrow: I've removed this for now since it breaks > > + down when Dr == 0. Is there something else we can do instead? > > We could special-case Dr == 0. It's quite trivial in the case where |dc| == > 0. In the other case it's a little trickier. This latter case already exists, and is the reason for stripping the kOpaqueAlpha_Flag in setContext. > > @@ +1866,5 @@ > > int count) { > > for (; count > 0; --count) { > > + SkFixed t; > > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > > + sk_bzero(dstC, sizeof(*dstC)); > > Let's just sk_bzero(dstC++, sizeof(*dstC)) > > It's probably even better to set 'fully transparent', through an assignment > to dstC as sk_bzero seems like it would cause needless overhead here. Skia doesn't have an option for making a gradient as fully transparent. I'd rather not write too much new code as we have to maintain this as a patch. I'd also hope that web authors aren't writing canvas with empty gradients and expecting it to be a nop. Maybe I have too much faith.

(In reply to Matt Woodrow (:mattwoodrow) from comment #12) > Created attachment 633941 [details] [diff] [review] > Make skia fully compliant with the canvas spec v4 > > > > @@ +1866,5 @@ > > > int count) { > > > for (; count > 0; --count) { > > > + SkFixed t; > > > + if (!two_point_radial(b, fx, fy, fSr2D2, foura, fOneOverTwoA, fDiffRadius, fRadius1, t)) { > > > + sk_bzero(dstC, sizeof(*dstC)); > > > > Let's just sk_bzero(dstC++, sizeof(*dstC)) > > > > It's probably even better to set 'fully transparent', through an assignment > > to dstC as sk_bzero seems like it would cause needless overhead here. > > Skia doesn't have an option for making a gradient as fully transparent. I'd > rather not write too much new code as we have to maintain this as a patch. > > I'd also hope that web authors aren't writing canvas with empty gradients > and expecting it to be a nop. Maybe I have too much faith. That's not what I meant, I meant rather than using sk_bzero( etc. using dstC++ = FullyTransParentColor;

Still needs a patch in a patch for Skia rebasing

and out again: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=598b1aa92c6b

Comment on attachment 646457 [details] [diff] [review] Matt's patch + Bas's last comment Review of attachment 646457 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/skia/src/effects/SkGradientShader.cpp @@ +2539,5 @@ > > + if (start == end && startRadius == 0) { > + return CreateRadial(start, endRadius, colors, pos, colorCount, mode, mapper); > + } > + This causes us to fail a whole bunch of the canvas tests. Remove this and they all pass again.

Comment on attachment 646457 [details] [diff] [review] Matt's patch + Bas's last comment reverting to r=bas (undo my r-), on retesting, I we seem to pass all the tests. confusion++

backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cda1c40da986

Unfortunately the bug number in the backout commit message was typoed. There's not really much that can be done about it now, but this means it will prevent m-cMerge (the tool that helps me mark bugs after merges) from doing an automatic match of backouts to landings. There is a semi-manual way to override, but it's a bit of a pain (and if someone other than me merges, they may not have spotted the typo). If it helps for the future, there's an exceptionally useful backout script created by mak, that allows you to do things like: $ backout rev1:rev5 rev8 And it pre-populates the commit message with both changeset and bug number - as well as taking care of backing out consecutive changesets without lots of merges. Many of the sheriffs use it to save time and increase the reliability of backouts - if it sounds of interest to you, just drop the script here into your .profile: https://wiki.mozilla.org/User:Mak77 Alternatively there is a mercurial extension by sfink that does the same thing: https://bitbucket.org/sfink/qbackout

The landing was manually deselected in m-cMerge, but it ended up being marked still (will file a bug on m-cMerge). Reopening since was backed out.

https://hg.mozilla.org/mozilla-central/rev/abf5ac19472b https://hg.mozilla.org/mozilla-central/rev/820d13f4d15f