Bug 78527 – [chromium] CSS Filters on accelerated layers: Brightness and Contrast don't work, Saturation is wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Stephen White]

The Brightness and Contrast filters are unimplemented on accelerated layers.  The saturation filter is wrong (colours are inverted -- see css3/filters/effect-saturate-hw.html).

[Stephen White]

Created attachment 126824 [details]
Patch

[Kenneth Russell]

Comment on attachment 126824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126824&action=review

Looks good to me though I'm no expert on the math. Couple of minor nits.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:43
> +    memset(matrix, 0, 20 * sizeof(SkScalar));

Instead of "20 * sizeof(SkScalar)" how about just sizeof(matrix)?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:58
> +    memset(matrix, 0, 20 * sizeof(SkScalar));

Same here.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 126824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126824&action=review
> 
> Looks good to me though I'm no expert on the math. Couple of minor nits.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:43
> > +    memset(matrix, 0, 20 * sizeof(SkScalar));
> 
> Instead of "20 * sizeof(SkScalar)" how about just sizeof(matrix)?

Doesn't work for array parameters, since they're actually pointers.  You get sizeof(SkScalar*).  (I actually wrote it this way first, and only a clang warning saved me!)

> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:58
> > +    memset(matrix, 0, 20 * sizeof(SkScalar));
> 
> Same here.

[Stephen White]

Created attachment 126828 [details]
unskip the css3/filters layout tests

[Kenneth Russell]

Comment on attachment 126824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126824&action=review

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:43
>>> +    memset(matrix, 0, 20 * sizeof(SkScalar));
>> 
>> Instead of "20 * sizeof(SkScalar)" how about just sizeof(matrix)?
> 
> Doesn't work for array parameters, since they're actually pointers.  You get sizeof(SkScalar*).  (I actually wrote it this way first, and only a clang warning saved me!)

Huh, I didn't know that!

[Kenneth Russell]

Comment on attachment 126828 [details]
unskip the css3/filters layout tests

Still looks good.

[Stephen White]

Committed r107611: <http://trac.webkit.org/changeset/107611>