Last Comment Bug 584322 - feDisplacementMap seems to be adding an offset
: feDisplacementMap seems to be adding an offset
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b4
Assigned To: Robert Longson
:
Mentors:
http://lavadip.com/experiments/bakemo...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-04 03:22 PDT by HRJ
Modified: 2010-08-15 08:52 PDT (History)
6 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase with scale=1 [no difference vs. opera] (584 bytes, image/svg+xml)
2010-08-04 11:26 PDT, Daniel Holbert [:dholbert]
no flags Details
testcase with scale=50 (585 bytes, image/svg+xml)
2010-08-04 11:28 PDT, Daniel Holbert [:dholbert]
no flags Details
patch with reftest (6.98 KB, patch)
2010-08-07 13:17 PDT, Robert Longson
no flags Details | Diff | Review
address review comment (9.15 KB, patch)
2010-08-09 00:02 PDT, Robert Longson
roc: review+
dbaron: approval2.0+
Details | Diff | Review

Description HRJ 2010-08-04 03:22:11 PDT
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 2010-08-04 05:39:22 PDT
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 [:bz] 2010-08-04 09:59:15 PDT
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 2010-08-04 10:37:20 PDT
(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 [:bz] 2010-08-04 10:48:40 PDT
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] 2010-08-04 11:26:20 PDT
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.
Comment 6 Daniel Holbert [:dholbert] 2010-08-04 11:28:34 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2010-08-04 11:42:55 PDT
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 2010-08-04 13:10:38 PDT
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 [:bz] 2010-08-04 17:00:25 PDT
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 2010-08-04 23:26:17 PDT
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 2010-08-05 02:39:26 PDT
(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 2010-08-05 03:59:44 PDT
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 [:bz] 2010-08-05 07:49:42 PDT
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 2010-08-06 12:21:21 PDT
(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 2010-08-07 13:17:22 PDT
Created attachment 463835 [details] [diff] [review]
patch with reftest
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-08 17:16:53 PDT
Why do you need the aImage parameter?
Comment 17 Robert Longson 2010-08-08 22:29:15 PDT
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) (Exited; email my personal email if necessary) 2010-08-08 23:04:11 PDT
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 2010-08-09 00:02:15 PDT
Created attachment 464001 [details] [diff] [review]
address review comment
Comment 20 Robert Longson 2010-08-09 01:14:55 PDT
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 22 timeless 2010-08-14 18:53:31 PDT
backed out:
http://hg.mozilla.org/mozilla-central/rev/538fa866340a
Comment 23 Robert Longson 2010-08-15 03:48:54 PDT
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-7 (review requests must explain patch) 2010-08-15 05:45:08 PDT
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 2010-08-15 08:18:54 PDT
Thanks David.

Note You need to log in before you can comment on or make changes to this bug.