Bug 107848 – [CSS Filters] Using negative drop-shadow radius values has slow performance

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Using a negative radius for the drop-shadow filters has slow repaint performance.

Also added a bug on the spec to check what's the right behavior for negative radius lengths:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20762

Example:
-webkit-filter: drop-shadow(10px 10px -1px red)

[Radar WebKit Bug Importer]

<rdar://problem/13118861>

[Dean Jackson]

Alex, do you think you'll address this soon? It seems like an easy fix.

[Alexandru Chiculita]

I will take a look at it.

[Alexandru Chiculita]

FEDropShadow and FEGaussianBlur are also affected, meaning that SVGFEDropShadowElement and SVGFEGaussianBlurElement also reproduce the issue.

[Michelangelo De Simone]

Alex's alter ego will take a look at this.:)

[Michelangelo De Simone]

Created attachment 191040 [details]
Patch

[WebKit Review Bot]

Attachment 191040 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-blur-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-blur-negative-radius.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius-expected.html', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius.html', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1
LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-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 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 191040 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:6276
> +        } else if (validUnit(val, FLength | FNonNeg, CSSStrictMode)) {

Do the specs say that negative radii are invalid, or clamped to zero?

[Michelangelo De Simone]

(In reply to comment #8)

> Do the specs say that negative radii are invalid, or clamped to zero?

Specs claim "A negative value is an error...", so I guess it's not to be clamped.

[Simon Fraser (smfr)]

Then that should cause the entire property to be ignored. Is that the behavior of your patch?

[Michelangelo De Simone]

(In reply to comment #10)

> Then that should cause the entire property to be ignored. Is that the behavior of your patch?

Yep.
Actually something like blur(-1px) was already covered by existing parsing tests.

[Dirk Schulze]

Comment on attachment 191040 [details]
Patch

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

The patch looks good to me in general. Just some change requests. Please check why the bots seems to have problems.

> Source/WebCore/ChangeLog:28
> +        (WebCore::FEGaussianBlur::calculateUnscaledKernelSize): Added "non-negative" assertion.
> +        * svg/SVGFEDropShadowElement.cpp:
> +        (WebCore::SVGFEDropShadowElement::build): If a negative standard deviation is encountered, don't

Can you write some more lines what you did? IIRC there even was a problem with not returning early here?

>> Source/WebCore/css/CSSParser.cpp:6276
>> +        } else if (validUnit(val, FLength | FNonNeg, CSSStrictMode)) {
> 
> Do the specs say that negative radii are invalid, or clamped to zero?

Your comment should really say that. The spec follows CSS background and borders. And CSS3BG says that negative values for the radius is not allowed.

> LayoutTests/css3/filters/effect-blur-negative-radius.html:10
> +            -webkit-filter: blur(-1px);

I wonder if we do not have negative tests for the -webkit-filter prefix? Maybe it is not necessary to add a ref test just for a parser check.

> LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js:96
> +testInvalidFilterRule("Negative radius", "drop-shadow(10px 10px -1px red)");

Here you go. This makes the other test unnecessary.

> LayoutTests/fast/box-shadow/box-shadow-negative-radius.html:35
> +    The following divs have box-shadow associated with them; every shadow is declared
> +    using a mix of invalid negative values, which are clearly wrong.

Ditto. Don't we have box-shadow parser tests? Since it should fail on parsing, this test seems to be no necessary. Thank you for taking the time to write this test :).

[Michelangelo De Simone]

Created attachment 191548 [details]
Patch

[Michelangelo De Simone]

(In reply to comment #12)

I've uploaded a revised patch according to your comments.

> The patch looks good to me in general. Just some change requests. Please check why the bots seems to have problems.

The check-webkit-style seems to be complaining about svn auto-props for PNGs, which AFAICT is a local configuration thingy not really related to the patch.
Anyhow, I've just filed bug 111474.

I'll eventually land this manually.

> Can you write some more lines what you did? IIRC there even was a problem with not returning early here?

Done.

> Your comment should really say that. The spec follows CSS background and borders. And CSS3BG says that negative values for the radius is not allowed.

Done.

> > LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js:96
> > +testInvalidFilterRule("Negative radius", "drop-shadow(10px 10px -1px red)");
> Here you go. This makes the other test unnecessary.

I've removed the blur-negative test, which was redundant.

> > LayoutTests/fast/box-shadow/box-shadow-negative-radius.html:35
> > +    The following divs have box-shadow associated with them; every shadow is declared
> > +    using a mix of invalid negative values, which are clearly wrong.
> Ditto. Don't we have box-shadow parser tests? Since it should fail on parsing, this test seems to be no necessary. Thank you for taking the time to write this test :).

I didn't find any existing parsing test for box-shadow so I thought to leave this test as it is; let me know if there's something I didn't check out.

Thanks!

[WebKit Review Bot]

Attachment 191548 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius-expected.html', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius.html', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1
LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-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 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 191548 [details]
Patch

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

Please look onto the style issue. Looks like you need to set the mime type manually? There is just one little change request. Other than that the patch looks great!

> LayoutTests/ChangeLog:16
> +        * fast/box-shadow/box-shadow-negative-radius-expected.html: Added.
> +        * fast/box-shadow/box-shadow-negative-radius.html: Added.

box shadow seems really to miss parsing tests. That is really really bad. Would it be possible that you create a box-shadow-parsing-invalid.html similar to other tests where you test just your part. Create a new bug report to add parser tests for box-shadow and add a FIXME into this new created file. I really appreciate if you do that. Thanks.

[Michelangelo De Simone]

Created attachment 191602 [details]
Patch

[Michelangelo De Simone]

Created attachment 192034 [details]
Patch

[Michelangelo De Simone]

Created attachment 192039 [details]
Patch

[Michelangelo De Simone]

(In reply to comment #16)

> Please look onto the style issue. Looks like you need to set the mime type manually? There is just one little change request. Other than that the patch looks great!

As far as I know webkit-patch land should take care of setting the mime type on PNGs automatically (see bug 75825).
I've a git only repo here so, in case this is not the case, I'll handle this manually.

> > LayoutTests/ChangeLog:16
> > +        * fast/box-shadow/box-shadow-negative-radius-expected.html: Added.
> > +        * fast/box-shadow/box-shadow-negative-radius.html: Added.
> box shadow seems really to miss parsing tests. That is really really bad. Would it be possible that you create a box-shadow-parsing-invalid.html similar to other tests where you test just your part. Create a new bug report to add parser tests for box-shadow and add a FIXME into this new created file. I really appreciate if you do that. Thanks.

As requested, I've added a new test for box-shadow invalid parsing test and filed a new bug for it.

Thanks.

[WebKit Review Bot]

Attachment 192039 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-parsing-invalid-expected.txt', u'LayoutTests/fast/box-shadow/box-shadow-parsing-invalid.html', u'LayoutTests/fast/box-shadow/script-tests/box-shadow-parsing-invalid.js', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1
LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-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 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 192039 [details]
Patch

LGTM. r=me. Let's see if the landing bot can handle it.

[WebKit Review Bot]

Comment on attachment 192039 [details]
Patch

Rejecting attachment 192039 [details] from commit-queue.

New failing tests:
fast/repaint/shadow-multiple-strict-vertical.html
fast/box-shadow/inset-shadow-large-offset.html
fast/box-shadow/spread-multiple-inset.html
fast/repaint/box-shadow-inset-repaint.html
http/tests/cache/subresource-failover-to-network.html
fast/borders/border-radius-with-box-shadow-01.html
fast/borders/border-shadow-large-radius.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
fast/box-shadow/spread.html
fast/repaint/shadow-multiple-strict-horizontal.html
fast/box-shadow/spread-multiple-normal.html
fast/dom/title-directionality-removeChild.html
fast/regions/element-region-overset-state-vertical-rl.html
fast/loader/text-document-wrapping.html
fast/repaint/box-shadow-h.html
fast/repaint/shadow-multiple-horizontal.html
fast/css/shadow-multiple.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
fast/dom/title-directionality.html
css3/filters/filter-repaint-shadow-rotated.html
fast/box-shadow/inset.html
fast/repaint/box-shadow-v.html
fast/box-shadow/box-shadow-clipped-slices.html
fast/repaint/shadow-multiple-vertical.html
fast/box-shadow/inset-box-shadows.html
fast/box-shadow/no-blur-multiple-offsets.html
fast/loader/javascript-url-in-object.html
css3/filters/filter-repaint-shadow-clipped.html
compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html
Full output: http://webkit-commit-queue.appspot.com/results/16991477

[Build Bot]

Comment on attachment 192039 [details]
Patch

Attachment 192039 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17025482

New failing tests:
platform/mac/editing/deleting/deletionUI-single-instance.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
svg/css/shadow-changes.svg
svg/filters/shadow-on-rect-with-filter.svg
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
fast/repaint/box-shadow-h.html
compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html
fast/repaint/box-shadow-v.html
fast/regions/element-region-overset-state-vertical-rl.html
svg/css/text-shadow-multiple.xhtml
svg/css/shadow-with-negative-offset.svg
fast/loader/javascript-url-in-object.html
transitions/cubic-bezier-overflow-shadow.html
media/video-played-collapse.html
svg/css/shadow-and-opacity.svg
compositing/filters/sw-shadow-overlaps-hw-shadow.html
http/tests/plugins/third-party-cookie-accept-policy.html

[Michelangelo De Simone]

Looks like there are "few" tests that need to be rebased.:-/

[Dean Jackson]

(In reply to comment #25)
> Looks like there are "few" tests that need to be rebased.:-/

Do these reproduce for you locally?

[WebKit Review Bot]

Comment on attachment 192039 [details]
Patch

Attachment 192039 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17000570

New failing tests:
fast/repaint/shadow-multiple-strict-vertical.html
fast/box-shadow/inset-shadow-large-offset.html
fast/box-shadow/spread-multiple-inset.html
fast/repaint/box-shadow-inset-repaint.html
fast/hidpi/video-controls-in-hidpi.html
http/tests/cache/subresource-failover-to-network.html
fast/borders/border-radius-with-box-shadow-01.html
fast/borders/border-shadow-large-radius.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
fast/box-shadow/spread.html
fast/repaint/shadow-multiple-strict-horizontal.html
fast/preloader/document-write-noscript.html
fast/regions/element-region-overset-state-vertical-rl.html
fast/loader/text-document-wrapping.html
fast/repaint/box-shadow-h.html
fast/repaint/shadow-multiple-horizontal.html
fast/css/shadow-multiple.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
css3/filters/filter-repaint-shadow-rotated.html
fast/box-shadow/inset.html
fast/repaint/box-shadow-v.html
fast/box-shadow/spread-multiple-normal.html
fast/box-shadow/box-shadow-clipped-slices.html
fast/repaint/shadow-multiple-vertical.html
fast/box-shadow/inset-box-shadows.html
fast/box-shadow/no-blur-multiple-offsets.html
fast/loader/javascript-url-in-object.html
css3/filters/filter-repaint-shadow-clipped.html
compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html
fast/repaint/text-shadow-horizontal.html

[Michelangelo De Simone]

(In reply to comment #26)

> Do these reproduce for you locally?

Almost all of them. I'm going over each one manually.

[Build Bot]

Comment on attachment 192039 [details]
Patch

Attachment 192039 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17136630

New failing tests:
compositing/filters/sw-layer-overlaps-hw-shadow.html
svg/filters/shadow-on-rect-with-filter.svg
svg/css/shadow-changes.svg
fast/repaint/box-shadow-h.html
compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html
svg/css/shadow-and-opacity.svg
fast/regions/element-region-overset-state-vertical-rl.html
svg/css/shadow-with-negative-offset.svg
transitions/multiple-shadow-transitions.html
svg/css/text-shadow-multiple.xhtml
transitions/svg-text-shadow-transition.html
transitions/cubic-bezier-overflow-shadow.html
fast/repaint/box-shadow-v.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html

[Michelangelo De Simone]

Created attachment 194530 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194530 [details]
Patch

Attachment 194530 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17141553

New failing tests:
svg/filters/feGaussianBlur-zero-deviation.svg
svg/filters/feDropShadow-zero-deviation.svg

[WebKit Review Bot]

Created attachment 194539 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Michelangelo De Simone]

(In reply to comment #31)

> New failing tests:
> svg/filters/feGaussianBlur-zero-deviation.svg
> svg/filters/feDropShadow-zero-deviation.svg

These two tests seem to be false positives.

[Michelangelo De Simone]

Created attachment 194577 [details]
Patch

[Dirk Schulze]

Comment on attachment 194577 [details]
Patch

LGTM. r=me

[WebKit Review Bot]

Comment on attachment 194577 [details]
Patch

Clearing flags on attachment: 194577

Committed r146762: <http://trac.webkit.org/changeset/146762>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.