Closed
Bug 718849
Opened 13 years ago
Closed 12 years ago
[Skia] Handle cone radial gradients as per the canvas spec
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(2 files, 6 obsolete files)
15.91 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
17.21 KB,
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #589318 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 1•13 years ago
|
||
This is using the same solution choices as pixman. I'm not sure what to do when both solutions correspond to negative radii though.
Attachment #589318 -
Attachment is obsolete: true
Attachment #589318 -
Flags: review?(jmuizelaar)
Attachment #589338 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•13 years ago
|
||
This needs more tidying up and comments. It now passes the full set of radial gradient tests from http://philip.html5.org
Attachment #589338 -
Attachment is obsolete: true
Attachment #589338 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•13 years ago
|
||
Passes 100% of the canvas gradient tests (linear and radial), now with comment changes!
Attachment #589381 -
Attachment is obsolete: true
Attachment #589396 -
Flags: review?(jmuizelaar)
Comment 4•13 years ago
|
||
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.
Attachment #589396 -
Flags: review?(jmuizelaar) → review?(bas.schouten)
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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.
Attachment #589396 -
Attachment is obsolete: true
Attachment #589396 -
Flags: review?(bas.schouten)
Attachment #628137 -
Flags: review?(bas.schouten)
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Attachment #628137 -
Attachment is obsolete: true
Attachment #628137 -
Flags: review?(bas.schouten)
Attachment #633941 -
Flags: review?(bas.schouten)
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
Still needs a patch in a patch for Skia rebasing
Attachment #633941 -
Attachment is obsolete: true
Attachment #633941 -
Flags: review?(bas.schouten)
Attachment #646457 -
Flags: review?(bas.schouten)
Updated•12 years ago
|
Attachment #646457 -
Flags: review?(bas.schouten) → review+
Updated•12 years ago
|
Attachment #647020 -
Flags: review?(gwright) → review+
Comment 17•12 years ago
|
||
and out again: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=598b1aa92c6b
Comment 18•12 years ago
|
||
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.
Attachment #646457 -
Flags: review+ → review-
Updated•12 years ago
|
Attachment #647020 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #647020 -
Attachment is obsolete: false
Comment 19•12 years ago
|
||
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++
Attachment #646457 -
Flags: review- → review+
Comment 21•12 years ago
|
||
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cda1c40da986
Comment 22•12 years ago
|
||
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
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 23•12 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/abf5ac19472b https://hg.mozilla.org/mozilla-central/rev/820d13f4d15f
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•