584322 - feDisplacementMap seems to be adding an offset

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[HRJ]

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

Comment 1

[HRJ]

Just confirmed that Inkscape renders the SVGs exactly like Opera. If they are right, then Firefox might be doing something wrong.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[HRJ]

(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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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)....

Comment 8

[Robert Longson [:longsonr]]

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/

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Robert Longson [:longsonr]]

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.

Comment 11

[Robert Longson [:longsonr]]

(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

[Jasper]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Robert Longson [:longsonr]]

(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.

Comment 15

[Robert Longson [:longsonr]]

Attachment: patch with reftest

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do you need the aImage parameter?

Comment 17

[Robert Longson [:longsonr]]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 19

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 20

[Robert Longson [:longsonr]]

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.

Comment 21

[timeless]

http://hg.mozilla.org/mozilla-central/rev/7b475a00cd7c

Comment 22

[timeless]

backed out:
http://hg.mozilla.org/mozilla-central/rev/538fa866340a

Comment 23

[Robert Longson [:longsonr]]

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.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 25

[Robert Longson [:longsonr]]

Thanks David.