Bug 123716 – Filters should test for area instead of single dimension

• Component: SVG
• Status: RESOLVED FIXED

[Adenilson Cavalcanti Silva]

Created attachment 215882 [details]
PoC to trigger the bug

It renders fine in Firefox, won't render in WebKit, the filter option that triggers the issue is feFlood.

Initial bug report on Chrome (http://code.google.com/p/chromium/issues/detail?id=240184).

[Adenilson Cavalcanti Silva]

Created attachment 223741 [details]
Should draw a green rectangle in patched engine

[Adenilson Cavalcanti Silva]

Created attachment 224089 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 5 files


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

[Adenilson Cavalcanti Silva]

The current patch fixes the rendering issue and at same time, fixes some layer violations where FilterEffectRenderer and RenderSVGResourceFilter were directly accessing the kMax value in FilterEffect.

[Simon Fraser (smfr)]

Comment on attachment 224089 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        A filtered SVG element with width or length bigger than 5.000

You should write this as 5000 or 5,000. 5.000 sounds like decimal 5.

> Source/WebCore/ChangeLog:10
> +	(counting the margin/border) will fail to render. This patch will
> +	instead test against the total element area.

Weird indentation

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:42
> +static const float kMaxFilterLength = 8192;

Length is ambiguous here. Length of what? Dimension would be better.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:84
> +

Extra blank line.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:62
> +    static bool isFilterSizeValid(const IntRect&);
> +    static bool isFilterSizeValid(const FloatRect&);

Are both really necessary?

[Adenilson Cavalcanti Silva]

Thanks for the review, I will comment inline.

> > Source/WebCore/ChangeLog:8
> > +        A filtered SVG element with width or length bigger than 5.000
> 
> You should write this as 5000 or 5,000. 5.000 sounds like decimal 5.
> 
Fixed.

> > Source/WebCore/ChangeLog:10
> > +	(counting the margin/border) will fail to render. This patch will
> > +	instead test against the total element area.
> 
> Weird indentation
> 
Fixed.

> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:42
> > +static const float kMaxFilterLength = 8192;
> 
> Length is ambiguous here. Length of what? Dimension would be better.
> 
Fixed.

> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:84
> > +
> 
> Extra blank line.
Fixed.

> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.h:62
> > +    static bool isFilterSizeValid(const IntRect&);
> > +    static bool isFilterSizeValid(const FloatRect&);
> 
> Are both really necessary?

Yep, FilterEffectRenderer.cpp uses FloatRect and FilterEffect used IntRect.

[Adenilson Cavalcanti Silva]

Created attachment 224112 [details]
Patch

[Build Bot]

Comment on attachment 224112 [details]
Patch

Attachment 224112 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5655241085157376

New failing tests:
css3/filters/multiple-filters-invalidation.html

[Build Bot]

Created attachment 224130 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 224112 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:41
> +static const float kMaxFilterArea = 8192 * 8192;

You may have to reduce this given the timing out tests.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:67
> +float FilterEffect::maxFilterLength()

Would be nice if you renamed the functions too to include "dimension"

> Source/WebCore/platform/graphics/filters/FilterEffect.h:62
> +    static bool isFilterSizeValid(const IntRect&);
> +    static bool isFilterSizeValid(const FloatRect&);

We implicitly convert IntRect -> FloatRect so you don't need both.

[Adenilson Cavalcanti Silva]

Created attachment 225297 [details]
Patch

[Adenilson Cavalcanti Silva]

Created attachment 225411 [details]
Patch

[Dirk Schulze]

Comment on attachment 225411 [details]
Patch

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

The patched files look good. See comments to change log and testing.

> Source/WebCore/ChangeLog:10
> +        A filtered SVG element with width or length bigger than 5000
> +        (counting the margin/border) will fail to render. This patch will
> +        instead test against the total element area.

This might not be an actual fix. See later comment. If a filter > 4096x4096 still fails, this change log should change wording. Right now it implies that taking the are will fix the overall problem.

> Source/WebCore/ChangeLog:20
> +        * platform/graphics/filters/FilterEffect.cpp:

Could you do some inline comments here please? Just say what you changed in some of the named methods. This makes it easier to review the change log later.

> LayoutTests/svg/filters/big-height-filter-expected.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg">

You need to create a change log entry for tests as well. The change log is located at LayoutTests/ChangeLog

I wonder how the tests test the changed behavior. We do use buffers differently now. In general all filters should run. Even if the buffer size extends beyond 4096x4096. If the filters don't run anymore, we need a test for a filter that is bigger than this area. I support the change in general. If a filter still fails if it is bigger than 4096x4096, then we should open a new bug report and fix it there. This would not be the proper fix for that and I suggest renaming the bug report to reflect the change you made.

[Adenilson Cavalcanti Silva]

Created attachment 225490 [details]
Patch

[Dirk Schulze]

Comment on attachment 225490 [details]
Patch

LGTM. Please create a new bug report for the filters that expand your filter area. This is a regression.

[WebKit Commit Bot]

Comment on attachment 225490 [details]
Patch

Clearing flags on attachment: 225490

Committed r164886: <http://trac.webkit.org/changeset/164886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Looks like css3/filters/multiple-filters-invalidation.html started to crash on one bot after this change:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=css3%2Ffilters%2Fmultiple-filters-invalidation.html

Could someone who understands this change suggest the course of action soon please? Should we roll out, or skip this test? Is this caused by this changed, or coincidental?

[Dirk Schulze]

(In reply to comment #18)
> Looks like css3/filters/multiple-filters-invalidation.html started to crash on one bot after this change:
> 
> http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=css3%2Ffilters%2Fmultiple-filters-invalidation.html
> 
> Could someone who understands this change suggest the course of action soon please? Should we roll out, or skip this test? Is this caused by this changed, or coincidental?

I can't see the crash log. What happens?

[Simon Fraser (smfr)]

It's not a crash, it's just timing out.
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r164911%20(16432)/results.html

[Simon Fraser (smfr)]

Having filters be slow enough to cause timing-out tests is crazy; the user experience will be terrible.

[Simon Fraser (smfr)]

There's a sample:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r164911%20(16432/css3/filters/multiple-filters-invalidation-sample.txt

[Simon Fraser (smfr)]

Sorry:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r164911%20(16432)/css3/filters/multiple-filters-invalidation-sample.txt

[Dirk Schulze]

(In reply to comment #23)
> Sorry:
> http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r164911%20(16432)/css3/filters/multiple-filters-invalidation-sample.txt

The point is rather that the test should not be affected by this change at all. This patch is about insanely big filters. The test in question has a normal filter size that should not be affected. Might it be an issue with another test that run previous to this one?

[Alexey Proskuryakov]

The previous test is named huge-region.html, which sounds potentially related to "insanely big filters".

[Adenilson Cavalcanti Silva]

huge-region.html uses a blur filter in a huge element which previously wouldn't be filtered at all.

This test was timing out in an early version of this patch, which triggered the adjustment in the maximum kernel size (from 1000 to 500). In my machine (OSX 10.8.5, i7@2.4Ghz) both css3/filters and svg/filters are running fine, I tested it before sending the patch.

I see that there are the following issues:

a) The blur effect is a tad slower than it should be (but, I started looking on this by smfr suggestion #120813).

b) huge-region should really become 2 tests: one for invalid blur values (it uses 2147483648px which will be clamped to 500). And another for big regions.


How you gentlemen feel would be the best way to handle this?

[Adenilson Cavalcanti Silva]

The text in the test (i.e. huge-region.html) says "This element is too big to filter."

If the objective of this test is to *not* filter something way too big, it would require to have the following dimensions: 4096x4096 (a bit less if you consider Margin + Border).

The element has width (50px) and height (50px) with padding, which would still be within the area used as the upper limit in the patch (4096 x 4096). 

Another alternative is to simply change the patch to use a smaller maximum area.

[Dirk Schulze]

(In reply to comment #27)
> The text in the test (i.e. huge-region.html) says "This element is too big to filter."
> 
> If the objective of this test is to *not* filter something way too big, it would require to have the following dimensions: 4096x4096 (a bit less if you consider Margin + Border).
> 
> The element has width (50px) and height (50px) with padding, which would still be within the area used as the upper limit in the patch (4096 x 4096). 
> 
> Another alternative is to simply change the patch to use a smaller maximum area.

Make it two tests. Yes, having slow filters is not great, but it renders eventually. And there is not security thread that reveals privacy data from the user (we checked this in the WG before).

The limitation to 5000 before and the filter area limitation now are not performance related but shall avoid creating too big buffers and cause crashes.

[Alexey Proskuryakov]

> How you gentlemen feel would be the best way to handle this?

If the best fix is taking time, can we have something very quick in the meanwhile please? Maybe skip the test?

Having failing tests on bots - even when we know what the reason is - makes is much more difficult to spot further unrelated regressions.

[Dirk Schulze]

(In reply to comment #29)
> > How you gentlemen feel would be the best way to handle this?
> 
> If the best fix is taking time, can we have something very quick in the meanwhile please? Maybe skip the test?
> 
> Having failing tests on bots - even when we know what the reason is - makes is much more difficult to spot further unrelated regressions.

Agreed. (In reply to comment #29)
> > How you gentlemen feel would be the best way to handle this?
> 
> If the best fix is taking time, can we have something very quick in the meanwhile please? Maybe skip the test?
> 
> Having failing tests on bots - even when we know what the reason is - makes is much more difficult to spot further unrelated regressions.

Agreed. Savago, could you please edit TestExpectations in  LayoutTests/mac/ and other affected bots? This might be the fastest. Please open a new bug report to fix the test.

[Adenilson Cavalcanti Silva]

Coolio, I will do it.

[Adenilson Cavalcanti Silva]

Follow up bugs:
https://bugs.webkit.org/show_bug.cgi?id=129552
https://bugs.webkit.org/show_bug.cgi?id=129553
https://bugs.webkit.org/show_bug.cgi?id=129555