Summary: | Filters should test for area instead of single dimension | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adenilson Cavalcanti Silva <savagobr> | ||||||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ap, buildbot, commit-queue, dino, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, kondapallykalyan, krit, pdr, rniwa, schenney, sergio, simon.fraser, zimmermann | ||||||||||||||||||
Priority: | P3 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | OS X 10.8 | ||||||||||||||||||||
Attachments: |
|
Description
Adenilson Cavalcanti Silva
2013-11-03 17:54:49 PST
Created attachment 223741 [details]
Should draw a green rectangle in patched engine
Created attachment 224089 [details]
Patch
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.
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. 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? 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. Created attachment 224112 [details]
Patch
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 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
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. Created attachment 225297 [details]
Patch
Created attachment 225411 [details]
Patch
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. Created attachment 225490 [details]
Patch
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.
Comment on attachment 225490 [details] Patch Clearing flags on attachment: 225490 Committed r164886: <http://trac.webkit.org/changeset/164886> All reviewed patches have been landed. Closing bug. 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? (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? 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 Having filters be slow enough to cause timing-out tests is crazy; the user experience will be terrible. 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 (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? The previous test is named huge-region.html, which sounds potentially related to "insanely big filters". 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? 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. (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. > 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.
(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. Coolio, I will do it. |