The default bug view has changed. See this FAQ.

feDisplacementMap seems to be adding an offset

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: HRJ, Assigned: Robert Longson)

Tracking

Trunk
mozilla2.0b4
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.125 Safari/533.4
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b2) Gecko/20100720 Firefox/4.0b2

I have created this SVG which renders hair on a monster using filters (feTurbulence and feDisplacementMap).

The hair appears to be offset to the bottom, right. To overcome the error I had to add another feOffset filter.

In Opera, there is no such offset required, and hence the above SVG renders wrong in Opera.

Since no offset seems intuitive, I am assuming it's a bug with Firefox rather than Opera. Webkit has only recently added support for feTurbulence and so I couldn't test it.

Reproducible: Always

Steps to Reproduce:
1. Open http://lavadip.com/experiments/bakemono_svg/snap.svg with Firefox 3.5+ or 4.0 b2

Actual Results:  
The hair appears centered, but only because an offset has been subtracted.

Expected Results:  
The hair should appear centered even without the feOffset filter.

For convenience, a simpler SVG is here http://lavadip.com/experiments/bakemono_svg/snap_small.svg
(Reporter)

Comment 1

7 years ago
Just confirmed that Inkscape renders the SVGs exactly like Opera. If they are right, then Firefox might be doing something wrong.
Hmm.  I wonder whether there's a colorspace issue here or something; this looks like the sort of systematic offset that could come up if linearRGB values are interpreted as sRGB or vice versa...
(Reporter)

Comment 3

7 years ago
(In reply to comment #2)
> Hmm.  I wonder whether there's a colorspace issue here or something; this looks
> like the sort of systematic offset that could come up if linearRGB values are
> interpreted as sRGB or vice versa...

Ah, sounds like a valid reason. Thanks for looking into it. 

If true, is it a problem with the browser, or incomplete color space specification in my SVG?

If the former, is there anything I can do to work-around the problem while the browser gets a fix?

Thanks,
HRJ
My current guess is this is an issue on our end, especially since playing around with the colorspace values in the testcase didn't seem to change the behavior... but filters do their own weird colorspace stuff in the code.  :(
Created attachment 462848 [details]
testcase with scale=1 [no difference vs. opera]

Confirming that Opera & Inkscape both disagree with us.

Here's a reduced-ish testcase of bugzilla's own.  The red blob is offset to the lower-right on Firefox, but not on Opera / Inkscape.

It looks like the amount of offset is (in part) controlled by 'scale' on the <feDisplacementMap>.  I can set 'scale' to anything without offsetting the blob in Opera/Inkscape, whereas in Firefox, the lower-right offset increases as 'scale' increases.  At scale=1, we match Opera/Inkscape.
Created attachment 462849 [details]
testcase with scale=50

(In reply to comment #5)
> At scale=1, we match Opera/Inkscape.

d'oh -- of course, to verify that, I set the scale to 1 in my testcase right before clicking 'submit', and ended up submitting a testcase that doesn't show any brokenness. :)

Here's a version with scale=50 that demonstrates the bug.
Attachment #462848 - Attachment description: testcase → testcase with scale=1 [no bug]
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Hmm.  So our relevant code seems to be:

5843   double scaleOver255 = scale / 255.0;
5844   double scaleAdjustment = 0.5 - 0.5 * scale;
...

5850       PRInt32 sourceX = x +
5851         NSToIntFloor(scaleOver255 * displacementData[targIndex + xChannel] +
5852                 scaleAdjustment);

The spec here says:

  P'(x,y) <- P( x + scale * (XC(x,y) - .5), y + scale * (YC(x,y) - .5))

The mapping between the two is:

* targIndex == y*width + 4*x (so the byte offset in the target image where we
  want to write the data).
* displacementData[targIndex + xChannel] == XC(x,y) * 255.0

So our formula for sourceX comes out to (ignoring the rounding):

  x + scale/255.0 * XC(x,y) * 255.0 + (0.5 - 0.5 * scale) =
  x + scale * (XC(x,y) - 0.5) + 0.5

which seems fine (the +0.5 is to make the floor actually round-to-nearest)....
(Assignee)

Comment 8

7 years ago
This seems invalid to me too, so I'm going to mark it as such. It can always be reopened if anything else turns up that contradicts our reasoning.

HRJ feel free to take this up with w3c or Opera and Inkscape. w3c can be contacted via http://lists.w3.org/Archives/Public/www-svg/
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
No, I think the bug is valid.  The feDisplacementMap impl looks ok, so the issue is either in the color spaces (as in, we get them wrong) or in the feTurbulence output.  If the color component mean of the feDisplacementMap is not .5, then that would give the observed effect, for example.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 10

7 years ago
We get this right FWIW: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-turb-01-f.html it's in the linearRGB colour space.
(Assignee)

Comment 11

7 years ago
(In reply to comment #9)

If there is a problem with feTurbulence or colour spaces then surely scale="1" would be affected and according to comment 5 that's OK.

Comment 12

7 years ago
Are you by any chance mixing up premultiplied and non-premultiplied colors? According to the spec the image data should be premultiplied and the displacement data non-premultiplied.
Attachment #462848 - Attachment description: testcase with scale=1 [no bug] → testcase with scale=1 [no difference vs. opera]
Yeah, the scale="1" thing confuses me too...  Especially because when I try feDisplacementMap on feFloods, it seems to work fine at first glance (but could you double-check that?).

Note that from what I just see visually, our feTurbulence output looks visually different from Opera's, but so do our colors in general.  So that might be a red herring.
(Assignee)

Comment 14

7 years ago
(In reply to comment #12)
> Are you by any chance mixing up premultiplied and non-premultiplied colors?
> According to the spec the image data should be premultiplied and the
> displacement data non-premultiplied.

Spot on.
Assignee: nobody → longsonr
(Assignee)

Updated

7 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 15

7 years ago
Created attachment 463835 [details] [diff] [review]
patch with reftest
Attachment #463835 - Flags: review?(roc)
Why do you need the aImage parameter?
(Assignee)

Comment 17

7 years ago
aImage allows us to distinguish between IN1 and result. In this case we happen not to need it as it's IN2 rather than IN1 we're interested in. I can remove it if you like.
You mean we know it's operating on an input if aImage is non-null? That's a bit of a hack :-)

How about making the index a signed int and making -1 mean the output? Give it a proper constant name and it won't even look like a hack :-)
(Assignee)

Comment 19

7 years ago
Created attachment 464001 [details] [diff] [review]
address review comment
Attachment #463835 - Attachment is obsolete: true
Attachment #464001 - Flags: review?(roc)
Attachment #463835 - Flags: review?(roc)
Attachment #464001 - Flags: review?(roc) → review+
(Assignee)

Comment 20

7 years ago
Comment on attachment 464001 [details] [diff] [review]
address review comment

Simple low risk fix that just changes how the feDisplacementMap filter works. Includes a test.
Attachment #464001 - Flags: approval2.0?
Attachment #464001 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [added to landing queue]
(Assignee)

Updated

7 years ago
Whiteboard: [added to landing queue] → [added to landing queue as ride along]

Comment 21

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7b475a00cd7c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [added to landing queue as ride along]
Target Milestone: --- → mozilla2.0b4

Comment 22

7 years ago
backed out:
http://hg.mozilla.org/mozilla-central/rev/538fa866340a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla2.0b4 → ---
(Assignee)

Comment 23

7 years ago
The reftest that is part of the patch was not landed. That's why it failed. It's the only new file in the patch.
http://hg.mozilla.org/mozilla-central/rev/4d25cd8808e5

I made up a commit message; hope it's ok.  (It would have been nice to have one in the patch.)
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
(Assignee)

Comment 25

7 years ago
Thanks David.
(Assignee)

Updated

7 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.