Bug 105929 – feDisplacementMap filter gets color space wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen Chenney]

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.

[Stephen Chenney]

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.

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 182347 [details]
Patch

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

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.

[Stephen Chenney]

2nd on my queue. I should have a revised patch with tests up today or tomorrow.

[WebKit Review Bot]

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

[Stephen Chenney]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Stephen Chenney]

Created attachment 189099 [details]
Patch

Removing old, largely useless test. Adding failing expectations.

[Stephen Chenney]

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.

[Stephen Chenney]

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.

[Stephen Chenney]

Comment on attachment 189198 [details]
Patch

Not fixed. Better fix coming.

[Dirk Schulze]

(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 ;)

[Stephen Chenney]

Created attachment 189377 [details]
Patch

[Stephen Chenney]

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.

[Stephen Chenney]

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

[Stephen Chenney]

Created attachment 189590 [details]
Patch

Wow, this apparently works on all platforms. Who would have thought?

[WebKit Review Bot]

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

[Dirk Schulze]

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

[Stephen Chenney]

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

[Stephen Chenney]

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

[Stephen Chenney]

Created attachment 190139 [details]
Patch

Now everything seems to be better.

[WebKit Review Bot]

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.

[Stephen Chenney]

I'll fix the style issue before landing. It's just an svn prop issue.

[Dirk Schulze]

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.

[Stephen Chenney]

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

[Stephen Chenney]

Committed r144110: <http://trac.webkit.org/changeset/144110>