Bug 103728

Summary: Optimize ColorMatrix filter
Product: WebKit Reporter: Zoltan Herczeg <zherczeg>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, krit, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch krit: review+

Zoltan Herczeg
Reported 2012-11-30 02:56:53 PST
ColorMatrix recalculates a lot of values in every pixels.
Attachments
patch (6.58 KB, patch)
2012-11-30 03:06 PST, Zoltan Herczeg
krit: review+
Zoltan Herczeg
Comment 1 2012-11-30 03:06:03 PST
Zoltan Herczeg
Comment 2 2012-11-30 07:07:25 PST
The strange thing is that the result is not claped between [0..1]. And I couldn't find anything in the specification what happens if the result is > 1 or < 0. Anyway, this is a really powerful filter.
Dirk Schulze
Comment 3 2012-12-04 08:52:45 PST
(In reply to comment #2) > The strange thing is that the result is not claped between [0..1]. And I couldn't find anything in the specification what happens if the result is > 1 or < 0. Anyway, this is a really powerful filter. We clapped in the past, but decided to remove it from the spec and follow Opera (which did not clamp). Does the your result clamp?
Dirk Schulze
Comment 4 2012-12-04 08:55:57 PST
Comment on attachment 176934 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=176934&action=review Looks reasonable to me. r=me. I am just not sure about the following: > Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:136 > + if (filterType == FECOLORMATRIX_TYPE_SATURATE) { > + components[0] = (0.213 + 0.787 * values[0]); > + components[1] = (0.715 - 0.715 * values[0]); > + components[2] = (0.072 - 0.072 * values[0]); > + components[3] = (0.213 - 0.213 * values[0]); > + components[4] = (0.715 + 0.285 * values[0]); > + components[5] = (0.072 - 0.072 * values[0]); > + components[6] = (0.213 - 0.213 * values[0]); > + components[7] = (0.715 - 0.715 * values[0]); > + components[8] = (0.072 + 0.928 * values[0]); > + } else if (filterType == FECOLORMATRIX_TYPE_HUEROTATE) { > + float cosHue = cos(values[0] * piFloat / 180); > + float sinHue = sin(values[0] * piFloat / 180); > + components[0] = 0.213 + cosHue * 0.787 - sinHue * 0.213; > + components[1] = 0.715 - cosHue * 0.715 - sinHue * 0.715; > + components[2] = 0.072 - cosHue * 0.072 + sinHue * 0.928; > + components[3] = 0.213 - cosHue * 0.213 + sinHue * 0.143; > + components[4] = 0.715 + cosHue * 0.285 + sinHue * 0.140; > + components[5] = 0.072 - cosHue * 0.072 - sinHue * 0.283; > + components[6] = 0.213 - cosHue * 0.213 - sinHue * 0.787; > + components[7] = 0.715 - cosHue * 0.715 + sinHue * 0.715; > + components[8] = 0.072 + cosHue * 0.928 + sinHue * 0.072; > + } Does it make sense to move this in inline functions? It looks so strange here and makes it less readable. Please consider it before landing.
Zoltan Herczeg
Comment 5 2012-12-05 02:38:11 PST
Thanks for the review! > Does it make sense to move this in inline functions? It looks so strange here and makes it less readable. Please consider it before landing. Yes, I did it. Patch landed as: http://trac.webkit.org/changeset/136661
Note You need to log in before you can comment on or make changes to this bug.