Last Comment Bug 718849 - [Skia] Handle cone radial gradients as per the canvas spec
: [Skia] Handle cone radial gradients as per the canvas spec
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on: 777614
Blocks: 622842 687187 734668 746883
  Show dependency treegraph
 
Reported: 2012-01-17 15:10 PST by Matt Woodrow (:mattwoodrow)
Modified: 2012-12-05 14:26 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change the root choice (1.14 KB, patch)
2012-01-17 15:10 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Use pixmans solution choice (6.53 KB, patch)
2012-01-17 16:19 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make skia fully compliant with the canvas spec (10.80 KB, patch)
2012-01-17 18:49 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make skia fully compliant with the canvas spec v2 (13.54 KB, patch)
2012-01-17 19:47 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make skia fully compliant with the canvas spec v3 (15.14 KB, patch)
2012-05-29 16:27 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make skia fully compliant with the canvas spec v4 (16.01 KB, patch)
2012-06-17 20:12 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Matt's patch + Bas's last comment (15.91 KB, patch)
2012-07-26 20:18 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review
for keeping Skia up to date (17.21 KB, patch)
2012-07-29 14:48 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2012-01-17 15:10:40 PST
Created attachment 589318 [details] [diff] [review]
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) 2012-01-17 16:19:48 PST
Created attachment 589338 [details] [diff] [review]
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) 2012-01-17 18:49:37 PST
Created attachment 589381 [details] [diff] [review]
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) 2012-01-17 19:47:18 PST
Created attachment 589396 [details] [diff] [review]
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] 2012-01-19 10:49:47 PST
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) 2012-04-16 00:31:52 PDT
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 (⌚ UTC+1/+2) 2012-04-19 01:32:53 PDT
(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) 2012-05-29 16:27:38 PDT
Created attachment 628137 [details] [diff] [review]
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) 2012-05-29 17:50:51 PDT
I will look at this tomorrow.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-05-29 23:50:50 PDT
Yep, 2d.gradient.object.current is bug 629882.
Comment 10 Bas Schouten (:bas.schouten) 2012-06-03 00:06:31 PDT
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) 2012-06-04 02:35:26 PDT
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) 2012-06-17 20:12:07 PDT
Created attachment 633941 [details] [diff] [review]
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) 2012-06-29 05:52:49 PDT
(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] 2012-07-26 20:18:41 PDT
Created attachment 646457 [details] [diff] [review]
Matt's patch + Bas's last comment

Still needs a patch in a patch for Skia rebasing
Comment 15 Nick Cameron [:nrc] 2012-07-29 14:48:50 PDT
Created attachment 647020 [details] [diff] [review]
for keeping Skia up to date
Comment 17 Nick Cameron [:nrc] 2012-07-29 16:42:10 PDT
and out again: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=598b1aa92c6b
Comment 18 Nick Cameron [:nrc] 2012-07-29 16:43:20 PDT
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] 2012-07-29 22:17:06 PDT
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 21 Nick Cameron [:nrc] 2012-07-30 23:23:39 PDT
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cda1c40da986
Comment 22 Ed Morley [:emorley] 2012-07-31 01:14:07 PDT
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] 2012-07-31 06:24:10 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.