718849 - [Skia] Handle cone radial gradients as per the canvas spec

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Change the root choice

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.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Use pixmans solution choice

This is using the same solution choices as pixman.

I'm not sure what to do when both solutions correspond to negative radii though.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Make skia fully compliant with the canvas spec

This needs more tidying up and comments.

It now passes the full set of radial gradient tests from http://philip.html5.org

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Make skia fully compliant with the canvas spec v2

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

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 5

[Matt Woodrow (:mattwoodrow)]

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 :)

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Make skia fully compliant with the canvas spec v3

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.

Comment 8

[Bas Schouten (:bas.schouten)]

I will look at this tomorrow.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 10

[Bas Schouten (:bas.schouten)]

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 11

[Bas Schouten (:bas.schouten)]

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

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Make skia fully compliant with the canvas spec v4

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

Comment 13

[Bas Schouten (:bas.schouten)]

(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;

Comment 14

[Nick Cameron [:nrc]]

Attachment: Matt's patch + Bas's last comment

Still needs a patch in a patch for Skia rebasing

Comment 15

[Nick Cameron [:nrc]]

Attachment: for keeping Skia up to date

Comment 16

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c457efd86223

Comment 17

[Nick Cameron [:nrc]]

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

Comment 18

[Nick Cameron [:nrc]]

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 19

[Nick Cameron [:nrc]]

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

Comment 20

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e4baff472b7f

Comment 21

[Nick Cameron [:nrc]]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Ed Morley [:emorley]]

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.

Comment 24

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7198b3ea1763

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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