1399942 - Paul Irish's SVG avatar is missing plaid on his shirt (drawn using feTurbulence filter)

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Visit https://perfmattersconf.com/
 2. Look at the rightmost of the three little avatar people (Paul Irish)

EXPECTED RESULTS:
 His  shirt should be plaid.

ACTUAL RESULTS:
 His shirt is just plain gray.

Chrome 63 dev, inkskape, and Emacs both give "expected results".

Firefox gives "actual results".

Partially reduced testcase attached.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (disregard; testcase showing unrelated issue)

Comment 2

[Daniel Holbert [:dholbert]]

Sorry, I'll spin comment 1's testcase off as a separate bug. It's an unrelated issue that I stumbled into while minimizing the original testcase (we seem to reject filter chains if there's an unrecognized source ID)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (expecting horizontal stripes)

Comment 4

[Daniel Holbert [:dholbert]]

So the testcase uses this turbulence filter:
  <feTurbulence baseFrequency="0 0.19"/>


I think the issue is with the "0" value there. If I replace that with some nonzero value (say, 0.1), then the filter actually shows up (with blobs rather than lines).

Here's the spec text on that attribute:
  https://www.w3.org/TR/SVG11/filters.html#feTurbulenceBaseFrequencyAttribute

0 values are valid, and in this case (with 2 values), it represents the frequency in the "X" direction, and I think it means there's no variability on that axis, hence there being horizontal lines (same color for all x values) in browsers where this works:

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Comment 6

[Daniel Holbert [:dholbert]]

This is a regression, albeit an old-ish one. A 2013-01-01 nightly gives expected results.  Based on blame on the "SVGTurbulenceRenderer" class, I wonder if this regressed from bug 924102?

Comment 7

[Daniel Holbert [:dholbert]]

Yup -- regression range:
Last good revision: 6ecf0c4dfcbe (2013-11-27)
First bad revision: 3c44c1f43a67 (2013-11-28)
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ecf0c4dfcbe&tochange=3c44c1f43a67

Regression from bug 924102 (Firefox 28 era).

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8908339 [details] [diff] [review]
patch

Wow, is it really this simple? :D  Nice!

Could you include a reftest?  Perhaps a "!= foo.svg about:blank" reftest, if it's too hard to make a reference case.  (My attached testcases 2 and 3 render blank in affected Firefox versions, I think, aside from the explicit un-filtered yellow div or <text> -- so !=about:blank should work at catching the bug.)

Comment 10

[Robert Longson [:longsonr]]

Attachment: patch with reftest

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8908344 [details] [diff] [review]
patch with reftest

Review of attachment 8908344 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me

Comment 12

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57088583f7adb81f19240149222369c1e9d67f57

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/830a612d662c
feTurbulence filter should only do nothing if baseFreq is zero in dx and dy r=dholbert

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/830a612d662c

Comment 15

[Markus Stange [:mstange]]

This patch causes us to divide by zero, just after the if {}:

  gfxRect firstPeriodInUserSpace(0, 0, 1 / fX, 1 / fY);
  gfxRect firstPeriodInFilterSpace = aInstance->UserSpaceToFilterSpace(firstPeriodInUserSpace);
  Size frequencyInFilterSpace(1 / firstPeriodInFilterSpace.width,
                              1 / firstPeriodInFilterSpace.height);

Was this intentional? It seems to work out the way we need to, but it would be nice to find a solution that does not rely on dividing by zero and dividing by infinity.

Comment 16

[Markus Stange [:mstange]]

I have a fix in mind and will put it up for review in a new bug.