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.
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.
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.
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.
2nd on my queue. I should have a revised patch with tests up today or tomorrow.
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
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 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 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
Created attachment 189099 [details] Patch Removing old, largely useless test. Adding failing expectations.
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.
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 on attachment 189198 [details] Patch Not fixed. Better fix coming.
(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 ;)
Created attachment 189377 [details] Patch
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.
(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.
Created attachment 189590 [details] Patch Wow, this apparently works on all platforms. Who would have thought?
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
(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.
(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.
(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.
Created attachment 190139 [details] Patch Now everything seems to be better.
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.
I'll fix the style issue before landing. It's just an svn prop issue.
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.
(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.
Committed r144110: <http://trac.webkit.org/changeset/144110>