Bug 103728 – Optimize ColorMatrix filter

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Zoltan Herczeg]

ColorMatrix recalculates a lot of values in every pixels.

[Zoltan Herczeg]

Created attachment 176934 [details]
patch

[Zoltan Herczeg]

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]

(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 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]

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