Bug 78527 - [chromium] CSS Filters on accelerated layers: Brightness and Contrast don't work, Saturation is wrong
Summary: [chromium] CSS Filters on accelerated layers: Brightness and Contrast don't ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 13:46 PST by Stephen White
Modified: 2012-02-13 14:23 PST (History)
4 users (show)

See Also:

Attachments
Patch (3.75 KB, patch)
2012-02-13 13:53 PST, Stephen White 		no flags Details | Formatted Diff | Diff
unskip the css3/filters layout tests (4.98 KB, patch)
2012-02-13 14:00 PST, Stephen White 		kbr: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-02-13 13:46:47 PST 
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).
Comment 1 Stephen White 2012-02-13 13:53:21 PST 
Created attachment 126824 [details]
Patch
Comment 2 Kenneth Russell 2012-02-13 13:58:12 PST 
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.
Comment 3 Stephen White 2012-02-13 14:00:13 PST 
(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.
Comment 4 Stephen White 2012-02-13 14:00:39 PST 
Created attachment 126828 [details]
unskip the css3/filters layout tests
Comment 5 Kenneth Russell 2012-02-13 14:07:57 PST 
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!
Comment 6 Kenneth Russell 2012-02-13 14:08:16 PST 
Comment on attachment 126828 [details]
unskip the css3/filters layout tests

Still looks good.
Comment 7 Stephen White 2012-02-13 14:23:11 PST 
Committed r107611: <http://trac.webkit.org/changeset/107611>