Bug 105929

Summary: feDisplacementMap filter gets color space wrong
Product: WebKit Reporter: Stephen Chenney <schenney>
Component: SVGAssignee: Stephen Chenney <schenney>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, dglazkov, dino, d-r, eric, esprehn+autocc, fmalita, frankhome61, krit, ojan.autocc, pdr, rniwa, sugoi, thorton, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=181267
https://bugs.webkit.org/show_bug.cgi?id=136418
Bug Depends on: 109985    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
Color-interp test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch krit: review+

Description Stephen Chenney 2013-01-02 10:31:01 PST
Our test result for feDisplacementMap does not match the results on Firefox or Opera. There is also an obvious bug demonstrated by this URL: http://jsfiddle.net/jsfmullany/VkyAa/1/

Simplifications reveal that we seem to be not offsetting the input channels by 0.5, but maybe it's something else.

Better tests coming soon.
Comment 1 Stephen Chenney 2013-01-02 15:56:09 PST
Created attachment 181092 [details]
Testcase

This test should render 4 squares each completely filed with mid-grey. Succeeds in Firefox and Opera. Fails in WebKit.
Comment 2 Stephen Chenney 2013-01-02 15:59:00 PST
Created attachment 181094 [details]
Color-interp test case

This test covers color-interpolation functionality for feDisplacementMap. This feature currently works correctly, but I'll add it to ensure future test coverage.
Comment 3 Stephen Chenney 2013-01-11 08:29:00 PST
Created attachment 182347 [details]
Patch
Comment 4 sugoi 2013-01-11 10:21:06 PST
Comment on attachment 182347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182347&action=review

> Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:118
> +    float scaledOffsetY = scaleY * 0.5f;

By removing the 0.5f from the original scaleAjustment[XY] variables, doing the static_cast<int>(...) at lines 124-125 will now do a "floor" instead of a "round". You probably want to have either "0.5f * scale[XY] - 0.5f" here or "0.5f - 0.5f * scale[XY]" if you replace your - (minus) by a + (plus) in lines 124-125 just before scaledOffset[XY].
Comment 5 sugoi 2013-02-14 06:59:09 PST
Hi all,

  If any form of activity could resume on this cl, it would help me as this fix is a prerequisite for other work I'll have to do shortly.

Thanks.
Comment 6 Stephen Chenney 2013-02-14 07:05:48 PST
2nd on my queue. I should have a revised patch with tests up today or tomorrow.
Comment 7 WebKit Review Bot 2013-02-14 08:40:18 PST
Comment on attachment 182347 [details]
Patch

Attachment 182347 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16570400

New failing tests:
svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-in2-prop.html
svg/filters/feDisplacementMap-filterUnits.svg
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-xChannelSelector-attr.html
svg/filters/feDisplacementMap.svg
svg/W3C-SVG-1.1/filters-displace-01-f.svg
svg/filters/feDisplacementMap-color-interpolation-filters.svg
svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-yChannelSelector-prop.html
svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-in-prop.html
svg/custom/feDisplacementMap-01.svg
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-scale-attr.html
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in-attr.html
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in2-attr.html
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-yChannelSelector-attr.html
svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-scale-prop.html
svg/filters/filterRes.svg
svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-xChannelSelector-prop.html
Comment 8 Stephen Chenney 2013-02-18 15:21:06 PST
Created attachment 188950 [details]
Patch

Actually, the filter is not wrong. But it is woefully poorly tested, and just a little inefficient. New tests match Opera to within one pixel, and are correct in their own right \(even without considering Opera\).
Comment 9 WebKit Review Bot 2013-02-18 17:11:18 PST
Comment on attachment 188950 [details]
Patch

Attachment 188950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16613396

New failing tests:
svg/filters/filterRes.svg
svg/filters/feDisplacementMap.svg
Comment 10 Build Bot 2013-02-19 04:00:14 PST
Comment on attachment 188950 [details]
Patch

Attachment 188950 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16615670

New failing tests:
svg/filters/feDisplacementMap-color-interpolation-filters.svg
Comment 11 Stephen Chenney 2013-02-19 09:00:50 PST
Created attachment 189099 [details]
Patch

Removing old, largely useless test. Adding failing expectations.
Comment 12 Stephen Chenney 2013-02-19 11:53:20 PST
The mac result for the new color-interpolation-filters test is wrong, because the spec is not being respected:

"The ‘color-interpolation-filters’ property only applies to the ‘in2’ source image and does not apply to the ‘in’ source image. The ‘in’ source image must remain in its current color space."

Which means that the result should not be converted to linearRGB from sRGB.
Comment 13 Stephen Chenney 2013-02-19 16:20:59 PST
Created attachment 189198 [details]
Patch

This is not quite ready for commit, but feel free to review. The color-interpolation-filters test needs some more work, to test that we correctly handle non-flood inputs.
Comment 14 Stephen Chenney 2013-02-20 11:50:30 PST
Comment on attachment 189198 [details]
Patch

Not fixed. Better fix coming.
Comment 15 Dirk Schulze 2013-02-20 11:55:10 PST
(In reply to comment #14)
> (From update of attachment 189198 [details])
> Not fixed. Better fix coming.

I do not think this is a big deal. But you are not supposed to set r- or r+ even to your own patches if you are not a reviewer. Just wait 2 more days before doing that ;)
Comment 16 Stephen Chenney 2013-02-20 14:22:08 PST
Created attachment 189377 [details]
Patch
Comment 17 Stephen Chenney 2013-02-20 16:22:36 PST
The latest patch still fails on mac, due to CGs handling of color space transforms. Evidence suggests that CQ refuses to allow inputs with different color spaces to be used in generating a filter result, but rather converts everything to the result space.

So we can either fail on the displacement tests or fail on the color-interpolation-filters test. I think it is best to fail on the color interpolation test because that is less visually problematic than failing on the displacement map.

I repeat, I do not think it is possible to get the correct behaviour with CG. Someone from Apple should decide how they wish to fail.
Comment 18 Stephen Chenney 2013-02-20 16:23:39 PST
(In reply to comment #17)
> The latest patch still fails on mac, due to CGs handling of color space transforms. Evidence suggests that CG refuses to allow inputs with different color spaces to be used in generating a filter result, but rather converts everything to the result space.
> 
> So we can either fail on the displacement tests or fail on the color-interpolation-filters test. I think it is best to fail on the color interpolation test because that is less visually problematic than failing on the displacement map.
> 
> I repeat, I do not think it is possible to get the correct behaviour with CG. Someone from Apple should decide how they wish to fail.
Comment 19 Stephen Chenney 2013-02-21 13:26:56 PST
Created attachment 189590 [details]
Patch

Wow, this apparently works on all platforms. Who would have thought?
Comment 20 WebKit Review Bot 2013-02-23 10:38:46 PST
Comment on attachment 189590 [details]
Patch

Attachment 189590 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16721735

New failing tests:
svg/W3C-SVG-1.1/filters-displace-01-f.svg
svg/dynamic-updates/SVGFECompositeElement-svgdom-k2-prop.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-bias-attr.html
svg/dynamic-updates/SVGFECompositeElement-svgdom-k1-prop.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-divisor-attr.html
svg/dynamic-updates/SVGFEColorMatrixElement-dom-values-attr.html
svg/dynamic-updates/SVGFECompositeElement-dom-k3-attr.html
inspector/audits/audits-panel-functional.html
fast/loader/text-document-wrapping.html
svg/dynamic-updates/SVGFECompositeElement-svgdom-operator-prop.html
svg/dynamic-updates/SVGFECompositeElement-dom-operator-attr.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-preserveAlpha-attr.html
svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-kernelUnitLength-attr.html
svg/dynamic-updates/SVGFECompositeElement-dom-k1-attr.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-edgeMode-attr.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-targetX-attr.html
svg/dynamic-updates/SVGFEColorMatrixElement-dom-type-attr.html
svg/dynamic-updates/SVGFECompositeElement-dom-k2-attr.html
svg/dynamic-updates/SVGFEColorMatrixElement-svgdom-values-prop.html
svg/dynamic-updates/SVGFEColorMatrixElement-svgdom-type-prop.html
css3/filters/effect-reference.html
svg/dynamic-updates/SVGFECompositeElement-svgdom-k3-prop.html
fast/loader/javascript-url-in-object.html
svg/dynamic-updates/SVGFECompositeElement-dom-k4-attr.html
svg/dynamic-updates/SVGFECompositeElement-svgdom-k4-prop.html
svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html
Comment 21 Dirk Schulze 2013-02-24 19:34:36 PST
(In reply to comment #19)
> Created an attachment (id=189590) [details]
> Patch
> 
> Wow, this apparently works on all platforms. Who would have thought?

Does it still work on all platforms including Mac OS? I saw the failing tests on chromium EWS. Doe these tests need a rebaseline, or are they really broken now?

To Mac. Mac uses CG + Software rendering. Hopefully we can use the Core Image code for Mac that was removed a couple of years ago. Tim, when you have time, can you backport this code to trunk? :)

In general feDisplacementMap is important, but sadly rarely used. I would still prefer a working feDisplacementMap. But is color-interpolation-filters just a rare case that fails, or could it be a real problem? I guess most people use the default linearRGB anyway.
Comment 22 Stephen Chenney 2013-02-25 07:55:08 PST
(In reply to comment #21)
> (In reply to comment #19)
> > Created an attachment (id=189590) [details] [details]
> > Patch
> > 
> > Wow, this apparently works on all platforms. Who would have thought?
> 
> Does it still work on all platforms including Mac OS? I saw the failing tests on chromium EWS. Doe these tests need a rebaseline, or are they really broken now?
> 
> To Mac. Mac uses CG + Software rendering. Hopefully we can use the Core Image code for Mac that was removed a couple of years ago. Tim, when you have time, can you backport this code to trunk? :)
> 
> In general feDisplacementMap is important, but sadly rarely used. I would still prefer a working feDisplacementMap. But is color-interpolation-filters just a rare case that fails, or could it be a real problem? I guess most people use the default linearRGB anyway.

I'll look at the failures. Most of them I rebaselined with the feFlood color space fix, so maybe that fix was in fact bad. or maybe this one's bad. I'll get it sorted.

I think getting it right is important, particularly since it seems pretty horrible to fail on a simple flood-filled displacement. Note comment #5, indicating a need for a fix. Someone else within the Chromium org also asked me to get on it.
Comment 23 Stephen Chenney 2013-02-25 09:14:22 PST
(In reply to comment #22)
> I'll look at the failures. Most of them I rebaselined with the feFlood color space fix, so maybe that fix was in fact bad. or maybe this one's bad. I'll get it sorted.

Something is going wrong with blend/composite style filters. Now to figure out why.
Comment 24 Stephen Chenney 2013-02-25 15:25:06 PST
Created attachment 190139 [details]
Patch

Now everything seems to be better.
Comment 25 WebKit Review Bot 2013-02-26 02:56:31 PST
Attachment 190139 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/W3C-SVG-1.1/filters-displace-01-f-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in2-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-scale-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-xChannelSelector-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-yChannelSelector-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-in-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-in2-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-scale-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-xChannelSelector-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-svgdom-yChannelSelector-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/filters/feDisplacementMap-expected.png', u'LayoutTests/platform/chromium-linux/svg/filters/filterRes-expected.png', u'LayoutTests/svg/filters/feDisplacementMap-color-interpolation-filters-expected.svg', u'LayoutTests/svg/filters/feDisplacementMap-color-interpolation-filters.svg', u'LayoutTests/svg/filters/feDisplacementMap-filterUnits-expected.svg', u'LayoutTests/svg/filters/feDisplacementMap-filterUnits.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp', u'Source/WebCore/platform/graphics/filters/FEDisplacementMap.h', u'Source/WebCore/platform/graphics/filters/FEFlood.cpp', u'Source/WebCore/platform/graphics/filters/FEFlood.h', u'Source/WebCore/platform/graphics/filters/FilterEffect.cpp', u'Source/WebCore/platform/graphics/filters/FilterEffect.h', u'Source/WebCore/platform/graphics/filters/SourceGraphic.h', u'Source/WebCore/rendering/FilterEffectRenderer.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in-attr-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Stephen Chenney 2013-02-26 04:57:21 PST
I'll fix the style issue before landing. It's just an svn prop issue.
Comment 27 Dirk Schulze 2013-02-26 09:07:23 PST
Comment on attachment 190139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190139&action=review

LGTM. Some snippets below. r=me.

> Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:102
> +    // Do not transform the 0th input, as per the spec.

s/0th input/first primitive input/ sounds better.

> Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:134
> +    float scaledOffsetX = 0.5 - scaleX * 0.5f;
> +    float scaledOffsetY = 0.5 - scaleY * 0.5f;

I think you can omit the .f here.

> Source/WebCore/platform/graphics/filters/FEFlood.h:44
> +    // feFlood does not perform color interpolation of any kind, so the result is always in ColorSpaceDeviceRGB
> +    // regardless of the value of color-interpolation-filters.

s/ColorSpaceDeviceRGB/current color space/ There is the property color-interpolation that can change the current color space. We just do not support that yet.
Comment 28 Stephen Chenney 2013-02-26 14:40:46 PST
(In reply to comment #27)
> (From update of attachment 190139 [details])
> 
> > Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:134
> > +    float scaledOffsetX = 0.5 - scaleX * 0.5f;
> > +    float scaledOffsetY = 0.5 - scaleY * 0.5f;
> 
> I think you can omit the .f here.

I'll get rid of the "f" to match the other code. In my old game development job, one had to always use the "f" on floats to prevent the compiler from assuming it was double (as the spec says it should) and hence doing type conversions to doubles for the math then back to floats for the assign. Maybe there's a performance win here somewhere.
Comment 29 Stephen Chenney 2013-02-26 14:41:16 PST
Committed r144110: <http://trac.webkit.org/changeset/144110>