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

RESOLVED FIXED in Firefox 57

Status

()

Core
SVG
P3
normal
RESOLVED FIXED
a month ago
3 days ago

People

(Reporter: dholbert, Assigned: longsonr)

Tracking

({regression, testcase})

Trunk
mozilla57
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

a month ago
Created attachment 8908219 [details]
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.
(Reporter)

Comment 1

a month ago
Created attachment 8908223 [details]
(disregard; testcase showing unrelated issue)
(Reporter)

Updated

a month ago
Attachment #8908219 - Attachment description: test.svg → testcase 1
(Reporter)

Updated

a month ago
Attachment #8908223 - Attachment description: testcase 2 → testcase 2 (expecting a gray blur overlay)
(Reporter)

Comment 2

a month ago
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)
(Reporter)

Comment 3

a month ago
Created attachment 8908236 [details]
testcase 2 (expecting horizontal stripes)
Attachment #8908223 - Attachment is obsolete: true
(Reporter)

Updated

a month ago
Attachment #8908223 - Attachment description: testcase 2 (expecting a gray blur overlay) → (disregard; testcase showing unrelated issue)
(Reporter)

Updated

a month ago
Priority: -- → P3
(Reporter)

Updated

a month ago
Summary: Paul Irish's SVG avatar is missing plaid on his shirt (drawn using filters) → Paul Irish's SVG avatar is missing plaid on his shirt (drawn using feTurbulence filter)
(Reporter)

Comment 4

a month ago
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:
(Reporter)

Comment 5

a month ago
Created attachment 8908280 [details]
testcase 3
(Reporter)

Comment 6

a month ago
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?
Keywords: regression
(Reporter)

Comment 7

a month ago
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).
Blocks: 924102
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → affected
(Reporter)

Updated

a month ago
Keywords: testcase
(Assignee)

Comment 8

a month ago
Created attachment 8908339 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #8908339 - Flags: review?(dholbert)
(Reporter)

Comment 9

a month ago
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.)
(Assignee)

Comment 10

a month ago
Created attachment 8908344 [details] [diff] [review]
patch with reftest
Attachment #8908339 - Attachment is obsolete: true
Attachment #8908339 - Flags: review?(dholbert)
Attachment #8908344 - Flags: review?(dholbert)
(Reporter)

Comment 11

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

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

Thanks! r=me
Attachment #8908344 - Flags: review?(dholbert) → review+
status-firefox55: affected → wontfix
status-firefox56: affected → fix-optional
status-firefox-esr52: affected → fix-optional
(Assignee)

Comment 12

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

Comment 13

a month ago
Pushed by longsonr@gmail.com:
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
(Assignee)

Updated

a month ago
Flags: in-testsuite+

Comment 14

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/830a612d662c
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox56: fix-optional → wontfix
status-firefox-esr52: fix-optional → wontfix
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.
Flags: needinfo?(longsonr)
I have a fix in mind and will put it up for review in a new bug.
Flags: needinfo?(longsonr)
Depends on: 1409132
You need to log in before you can comment on or make changes to this bug.