| Summary: | FEGaussianBlur::calculateUnscaledKernelSize does unspeakable things with signed and unsigned values | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||
| Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, dino, kondapallykalyan, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 246582 [details]
Patch
Comment on attachment 246582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479 > + kernelSize.setWidth(static_cast<int>(clampedSize)); I'd rather use clampTo and not static casting. Comment on attachment 246582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review >> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479 >> + kernelSize.setWidth(static_cast<int>(clampedSize)); > > I'd rather use clampTo and not static casting. OK! Created attachment 246591 [details]
Patch
Created attachment 246593 [details]
Patch
Comment on attachment 246593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:469 > +static int clampedKernelSize(float value) clampedToKernelSize() might be a better name. > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:475 > + unsigned clampedSize = std::min(size, static_cast<unsigned>(gMaxKernelSize)); > + return clampTo<int>(clampedSize); the clampedSize variable is a bit redundant, i'd just go with clampTo<int>(std::min(size, static_cast<unsigned>(gMaxKernelSize))); Committed r180147: <http://trac.webkit.org/changeset/180147> Comment on attachment 246593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:473 > + unsigned size = std::max<unsigned>(2, static_cast<unsigned>(floorf(value * gaussianKernelFactor() + 0.5f))); Why not just do this math as floats, then clamp to int directly without going via unsigned? |
The 'calculateUnscaledKernelSize' method improperly treats signed and unsigned values as interchangeable. This creates problems when we have extremely large stdDevication values (such as in the test case "css3/filters/huge-blur-value.html"). When the standard deviation supplied to the routine overflows a int value, we pass In this pathological case, the Std Deviation used for the blur is the float 2.14748365E+9. When this is cast as an unsigned, then assigned to a signed integer, we end up with -257759744. The minimum of -257759744 and 500 turns out to be -257759744. IntSize kernelSize; // Limit the kernel size to 500. A bigger radius won't make a big difference for the result image but // inflates the absolute paint rect too much. This is compatible with Firefox' behavior. if (stdDeviation.x()) { int size = std::max<unsigned>(2, static_cast<unsigned>(floorf(stdDeviation.x() * gaussianKernelFactor() + 0.5f))); kernelSize.setWidth(std::min(size, gMaxKernelSize)); } Later, this kernelSize is cast back to an unsigned, bypassing this ceiling, and subjecting other code to values far in excess of 500.