Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150973004:
Fixes the rendering of a SVG filter (e.g. feFlood) by testing against total (Closed)
|
Created:
6 years, 10 months ago by Savago-old Modified:
6 years, 10 months ago CC:
blink-reviews, jamesr, krit, jbroman, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, danakj, ed+blinkwatch_opera.com, Rik, gyuyoung.kim_webkit.org, jchaffraix+rendering, pdr, pdr., rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFixes the rendering of a SVG filter (e.g. feFlood) by testing against total
area instead of single dimension.
While fixing this bug, I noticed also layer violations in FilterEffectRenderer
and RenderSVGResourceFilter accessing the maximum size defined in FilterEffect
or replicating the test logic for maximum dimension. This patch also fixes
those issues.
BUG=240184
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167564
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed reviewer's comments: static variable, compare function, #
Total comments: 1
Patch Set 3 : FloatRect(IntRect()) #
Total comments: 5
Patch Set 4 : Added tests, proper handling area x scale. #Patch Set 5 : Added rebaseline test. #Messages
Total messages: 26 (0 generated)
No review yet, but I noticed you don't have a layout test. We'll need one - if you're not familiar, take a look under LayoutTests/svg/ for some/lots of examples. Basically you want an SVG doc that renders correctly with the patch, and incorrectly without. The convention is green (square) == correct, any visible red == failure. In this case I'd suggest drawing a large green filtered element on top of a red rect: if the filter draws, then it covers the rect and all is green. Maybe put everything under a 100x100 clip to bring it down to the standard test size. I think we can also do this as a ref-test (http://trac.webkit.org/wiki/Writing%20Reftests), but let's start small :) Tomorrow I'll have more time and I can help you if needed.
https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right): https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() * scale.width() > FilterEffect::maxFilterArea()) { This compares a distance to an area. https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:117: if (size.height() * scale.height() > FilterEffect::maxFilterArea()) { Same as above. https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.cpp:39: const float FilterEffect::kMaxFilterArea(2 << 21); Nit: I think this would read more easily as "2048 * 2048" - or at least "1 << 22" - the "2" is pretty confusing and easy to miss. Also, isn't this a tad small? https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterEffect.h (right): https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.h:183: static const float kMaxFilterArea; Nit: Any reason to have this in class scope? (I.e., why not just leave as static in the .cpp file)
Dear fs Thanks for the review, I will comment inline. > https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... > Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() * > scale.width() > FilterEffect::maxFilterArea()) { > This compares a distance to an area. > The code originally called the control variable kMaxFilterSize, since it used it as a single dimension for the filter i.e. width. I renamed the variable to represent that now it really is an area and agree that is a bit odd to compare an area with a distance. What if we rename the member function to 'maxFilterPixels()' instead? > https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... > Source/core/rendering/svg/RenderSVGResourceFilter.cpp:117: if (size.height() * > scale.height() > FilterEffect::maxFilterArea()) { > Same as above. > Ditto. > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > Source/platform/graphics/filters/FilterEffect.cpp:39: const float > FilterEffect::kMaxFilterArea(2 << 21); > Nit: I think this would read more easily as "2048 * 2048" - or at least "1 << > 22" - the "2" is pretty confusing and easy to miss. > > Also, isn't this a tad small? > If the value is the same, the way to represent it is a bit subjective. It is possible to argue that maybe hexa/oct/etc would be more readable, in the end it is really personal taste. I don't feel strong about neither, so I can change it. The top size could be 4096x4096=2^24 as suggested by schenney in the chromium bug report. > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > File Source/platform/graphics/filters/FilterEffect.h (right): > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > Source/platform/graphics/filters/FilterEffect.h:183: static const float > kMaxFilterArea; > Nit: Any reason to have this in class scope? (I.e., why not just leave as static > in the .cpp file)
Sorry, I missed this one: > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > > File Source/platform/graphics/filters/FilterEffect.h (right): > > > > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > > Source/platform/graphics/filters/FilterEffect.h:183: static const float > > kMaxFilterArea; > > Nit: Any reason to have this in class scope? (I.e., why not just leave as > static > > in the .cpp file) I moved the variable to be part of the FilterEffect and made it protected to *ensure* that no other class would be accessing it without using the newly added class member function. The original code had 2 other classes using the kmaxFilter variable directly, which is a layer violation. Being part of the class helps to make it more encapsulated and avoid a non member to directly access it (e.g. a static function in the .cpp could access the value directly if it is declared/defined there). In the way the patch is implemented, it would need to use the public static member function to access the value.
On 2014/02/10 19:53:36, Savago wrote: > Dear fs > > Thanks for the review, I will comment inline. > > > https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... > > Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() * > > scale.width() > FilterEffect::maxFilterArea()) { > > This compares a distance to an area. > > > > The code originally called the control variable kMaxFilterSize, since it used it > as a single dimension for the filter i.e. width. > > I renamed the variable to represent that now it really is an area and agree that > is a bit odd to compare an area with a distance. What if we rename the member > function to 'maxFilterPixels()' instead? The interesting point isn't 'area' vs. 'pixels' in the name (they would both work equally well IMHO) - it's rather that this changes from comparing against a dimension (of 5000) to a dimension of 2^22. I.e. you end up allowing areas as large as 2^22 * 2^22 - which is not what you intended, right? I would rather expect something along lines of: FloatRect scaledSize = size; scaledSize.scale(scale.width(), scale.height()); if (scaledSize.width() * scaledSize.height() > FilterEffect::maxFilterArea()) /* Do something about it. */; > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > > Source/platform/graphics/filters/FilterEffect.cpp:39: const float > > FilterEffect::kMaxFilterArea(2 << 21); > > Nit: I think this would read more easily as "2048 * 2048" - or at least "1 << > > 22" - the "2" is pretty confusing and easy to miss. > > > > Also, isn't this a tad small? > > > > If the value is the same, the way to represent it is a bit subjective. It is > possible to argue that maybe hexa/oct/etc would be more readable, in the end it > is really personal taste. > > I don't feel strong about neither, so I can change it. > > The top size could be 4096x4096=2^24 as suggested by schenney in the chromium > bug report. I'll leave this for owners to decide.
On 2014/02/10 19:58:19, Savago wrote: > Sorry, I missed this one: > > > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > > > File Source/platform/graphics/filters/FilterEffect.h (right): > > > > > > > > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > > > Source/platform/graphics/filters/FilterEffect.h:183: static const float > > > kMaxFilterArea; > > > Nit: Any reason to have this in class scope? (I.e., why not just leave as > > static > > > in the .cpp file) > > I moved the variable to be part of the FilterEffect and made it protected to > *ensure* that no other class would be accessing it without using the newly added > class member function. > > The original code had 2 other classes using the kmaxFilter variable directly, > which is a layer violation. Being part of the class helps to make it more > encapsulated and avoid a non member to directly access it (e.g. a static > function in the .cpp could access the value directly if it is declared/defined > there). That a function in the same compilation unit has access to the constant, I wouldn't see as a problem (more a strength actually). I'll defer to owners here as well.
Mostly I agree with fs. And you need a test. Actually, you need 3 or more tests to cover all the cases: - Width greater than 4096 but area < - Ditto for height - Area too big with no scale - Area too bug with scale You might have to resort to tricks, like defining a very large SVG but putting it in a small viewport so it appears on screen for a ref-test. Others? https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right): https://codereview.chromium.org/150973004/diff/1/Source/core/rendering/svg/Re... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:110: bool RenderSVGResourceFilter::fitsInMaximumImageSize(const FloatSize& size, FloatSize& scale) The current implementation is wrong. It should compute the area, compare to the max area, and reduce the scale by the sqrt of the ratio. (Preserve the aspect ratio.) https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.cpp:39: const float FilterEffect::kMaxFilterArea(2 << 21); On 2014/02/07 10:17:51, fs wrote: > Nit: I think this would read more easily as "2048 * 2048" - or at least "1 << > 22" - the "2" is pretty confusing and easy to miss. > > Also, isn't this a tad small? Yeah. I wanted 4096 x 4096. static const float kMaxFilterArea = 4096 * 4096; From memory compilers support constant expressions to define static constants. https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.cpp:65: bool FilterEffect::isFilterSizeValid(FloatRect rect) Should be const FloatRect&. https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.cpp:75: bool FilterEffect::isFilterSizeValid(IntRect rect) Should be const IntRect&. https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterEffect.h (right): https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.h:183: static const float kMaxFilterArea; On 2014/02/07 10:17:51, fs wrote: > Nit: Any reason to have this in class scope? (I.e., why not just leave as static > in the .cpp file) In platform graphics it is typical to put these in the .cpp file of the class that uses them, in file scope. So remove this declaration here and remove the FilterEffect scope on the definition.
https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterEffect.cpp:78: || (rect.height() * rect.width() > kMaxFilterArea)) Is there is a danger of overflow here? The multiply will be done in integer, then promoted to float for the comparison. So it could overflow and fail the compare while being larger than max. It might actually be safer to simply construct a FloatRect from the argument, and pass that through to the other version. Then the multiply will be done in float. (There would be a loss of precision for very large ints, but I think it would still do the right thing, since at the point where this occurs, all values will be above max anyway and will pass the compare.) Another option would be to cast to int64_t before the multiply, but I think floats would give the same result with less code.
On 2014/02/11 15:20:34, Stephen White wrote: > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > File Source/platform/graphics/filters/FilterEffect.cpp (right): > > https://codereview.chromium.org/150973004/diff/1/Source/platform/graphics/fil... > Source/platform/graphics/filters/FilterEffect.cpp:78: || (rect.height() * > rect.width() > kMaxFilterArea)) > Is there is a danger of overflow here? The multiply will be done in integer, > then promoted to float for the comparison. So it could overflow and fail the > compare while being larger than max. > > It might actually be safer to simply construct a FloatRect from the argument, > and pass that through to the other version. Then the multiply will be done in > float. (There would be a loss of precision for very large ints, but I think it > would still do the right thing, since at the point where this occurs, all values > will be above max anyway and will pass the compare.) > > Another option would be to cast to int64_t before the multiply, but I think > floats would give the same result with less code. I agree with converting the IntRect to FloatRect and passing it through.
Gentlemen Thanks you all for the review, this next patch should address most of requests. Concerning the tests, I added 2 SVGs as suggested by Florian that will render as green rectangles in patched blink (and as red with vanilla). Since it is my first time writing such tests, what is the procedure concerning the test expectations (i.e. expected result)? Someone could give me some quick intro about the subject? fs: Now we do scale the image dimension in case of width or height being bigger than the sqrt (or the length) of maximum area. Stephen: for the last 2 test cases you commented, I feel that svg/filters/big-sized-filter.svg already stresses the code. What you think?
https://codereview.chromium.org/150973004/diff/170001/Source/platform/graphic... File Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/150973004/diff/170001/Source/platform/graphic... Source/platform/graphics/filters/FilterEffect.cpp:83: return isFilterSizeValid(higherPrec); Nit: Technically this isn't higher precision (there are some ints that can't be represented exactly as floats). I'd just do the conversion on the same line, as return isFilterSizeValid(FloatRect(rect));
On 2014/02/11 20:40:54, Savago wrote: > Gentlemen > > Thanks you all for the review, this next patch should address most of requests. > > Concerning the tests, I added 2 SVGs as suggested by Florian that will render as > green rectangles in patched blink (and as red with vanilla). Since it is my > first time writing such tests, what is the procedure concerning the test > expectations (i.e. expected result)? Someone could give me some quick intro > about the subject? The current two tests look like the could be turned into reftests, with a reference like: <svg xmlns="http://www.w3.org/2000/svg"> <rect width="100" height="100" fill="green"/> </svg> which would then be named <name of file excluding extension>-expected.svg and placed in the same directory as the test-file itself. (Additional info at http://dev.chromium.org/developers/testing/webkit-layout-tests and probably elsewhere on the wiki.) > Stephen: for the last 2 test cases you commented, I feel that > svg/filters/big-sized-filter.svg already stresses the code. What you think? You probably want a test for the area case as well.
https://codereview.chromium.org/150973004/diff/250001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right): https://codereview.chromium.org/150973004/diff/250001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() > FilterEffect::maxFilterLength()) { Now scale is no longer regarded in the condition - how come? One simple way to get a scale != 1 would be to set the 'filterRes' attribute on the 'filter' (and that's probably reasonable to test as well.)
fs I did an experiment using different filterRes values and identified what seems to be another bug: a small filterRes value will upscale the filtered SVG element. Further details here: http://code.google.com/p/chromium/issues/detail?id=343295
If that is ok, I would rather first fix the current issue (i.e. rendering of a SVG filter) and next move to the next issue (i.e. properly handling filterRes attribute). What you guys think about it?
Concerning the check in RenderSVGResourceFilter, what if the code tests for the scaled size area (as previously suggested by fs) and next adjust the scale factor using the maximum length? Something like: https://gist.github.com/Adenilson/8983451
Tests look great, but let's take fs' advice and turn them into ref-tests (those are always preferable since you don't have to manage/rebaseline the results). All you have to do is add two "-expected.svg" files (big-width-filter-expected.svg & big-height-filter-expected.svg) in the same dir as the actual tests, with the following content: <svg xmlns="http://www.w3.org/2000/svg"> <rect width="100" height="100" fill="green"/> </svg> Also, looking at trybot results, it seems the patch causes 3 tests to fail (different image results): https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/18... On the surface it seems they just need new baselines (minor rendering diffs), which you would achieve by marking them for rebaseline - just add these lines to LayoutTests/TestsExpectations and let the auto-rebaseline bot do the work: Bug(Savago) svg/filters/big-sized-filter.svg [ NeedsRebaseline ] Bug(Savago) svg/filters/fecomposite-two-regions.svg [ NeedsRebaseline ] Bug(Savago) svg/foreignObject/filter.html [ NeedsRebaseline ] But I'm a bit concerned because two of these (filters/fecomposite-two-regions.svg & foreignObject/filter.html) should not be anywhere near the max filter size. So I think we need to figure out why their output is affected by this change. https://codereview.chromium.org/150973004/diff/250001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right): https://codereview.chromium.org/150973004/diff/250001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() > FilterEffect::maxFilterLength()) { On 2014/02/12 08:16:37, fs wrote: > Now scale is no longer regarded in the condition - how come? One simple way to > get a scale != 1 would be to set the 'filterRes' attribute on the 'filter' (and > that's probably reasonable to test as well.) +1. I also think we should consider the scale here. https://codereview.chromium.org/150973004/diff/250001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGResourceFilter.cpp:113: if (size.width() > FilterEffect::maxFilterLength()) { Hmmm... so the concern this is attempting to address is allocating huge memory chunks due to large bitmaps => large *area*, not necessarily large any-one-length. Maybe we can focus on that instead, and scale uniformly? A uniform scale may also produce better results (the filter is "stretched" by the same factor in both dimensions). So how about this instead: bool RenderSVGResourceFilter::fitsInMaximumImageSize(const FloatSize& size, FloatSize& filterScale) { FloatSize scaledSize(size); scaledSize.scale(filterScale.width(), filterScale.height()); float scaledArea = scaledSize.width() * scaledSize.width(); if (scaledArea <= FilterEffect::maxFilterArea()) return true; filterScale.scale(sqrt(FilterEffect::maxFilterArea() / scaledArea)); return false; } Then we can drop maxFilterLength()& kMaxFilterLength altogether. https://codereview.chromium.org/150973004/diff/250001/Source/platform/graphic... File Source/platform/graphics/filters/FilterEffect.cpp (right): https://codereview.chromium.org/150973004/diff/250001/Source/platform/graphic... Source/platform/graphics/filters/FilterEffect.cpp:39: static const float kMaxFilterArea = 8192 * 8192; Let's keep this at the suggested 4096 * 4096 value - it's closer to the previous limit. https://codereview.chromium.org/150973004/diff/250001/Source/platform/graphic... Source/platform/graphics/filters/FilterEffect.cpp:60: float FilterEffect::maxFilterArea() This doesn't seem to be used at all ATM?
On 2014/02/12 22:49:22, Savago wrote: > If that is ok, I would rather first fix the current issue (i.e. rendering of a > SVG filter) and next move to the next issue (i.e. properly handling filterRes > attribute). > > What you guys think about it? Sounds good, these should be handled separately.
On 2014/02/15 16:00:44, Florin Malita wrote: > bool RenderSVGResourceFilter::fitsInMaximumImageSize(const FloatSize& size, > FloatSize& filterScale) Come to think of it, this method is showing its age: misleading name + unused return value. Should probably be re-declared as void adjustFilterScaleForMaximumImageSize(...) but it's really unrelated to the issue, so it's up to you if you want to do it in this patch.
On 2014/02/15 16:00:44, Florin Malita wrote: > But I'm a bit concerned because two of these > (filters/fecomposite-two-regions.svg & foreignObject/filter.html) should not be > anywhere near the max filter size. So I think we need to figure out why their > output is affected by this change. Gah, mystery solved: both of those tests forgot to specify filterUnits on their filters, hence are using the default 'objectBoundingBox' units while their size is specified in 'userSpaceOnUse' units: <filter id="filter" width="200" height="10"> ... <foreignObject width="200" height="100" filter="url(#filter)" opacity=".5"> That means the actual filter region gets resolved to 200*200 x 10*100 = 40000 x 1000 - which is well above the max, hence trimming is expected. So it's just a couple of bad tests, feel free to go ahead rebaselining them.
Florin Thanks for the review, I just uploaded a new patch version. I see that in mac_blink_rel, a test (big-sized-filter) has minor differences: https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/19... Should I rebaseline it?
Thanks Savago, lgtm. On 2014/02/20 00:54:20, Savago wrote: > Florin > > Thanks for the review, I just uploaded a new patch version. > > I see that in mac_blink_rel, a test (big-sized-filter) has minor differences: > https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/19... > > Should I rebaseline it? Yes, just add the following to LayoutTests/TestExpectations and the auto rebaseline bot should take care of it: Bug(Savago) svg/filters/big-sized-filter.svg [ NeedsRebaseline ]
The CQ bit was checked by a.cavalcanti@partner.samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.cavalcanti@partner.samsung.com/15097...
Florin Thanks for reviewing, last patch version has the rebaseline of the failing test.
Message was sent while issue was closed.
Change committed as 167564 |
