Bug 98962

Summary: [CSS Shaders] Clamp input colors before blending
Product: WebKit Reporter: Max Vujovic <mvujovic>
Component: Layout and RenderingAssignee: Michelangelo De Simone <michelangelo>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, dino, dongseong.hwang, kenneth, michelangelo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71392    
Attachments:
Description Flags
Patch not for formal review
none
Patch none

Max Vujovic
Reported 2012-10-10 15:53:13 PDT
In CustomFilterValidatedProgram.cpp, we use the following GLSL code to blend the element texture (premultiplied with css_ColorMatrix) and css_MixColor. mediump vec4 originalColor = texture2D(css_u_texture, css_v_texCoord); mediump vec4 multipliedColor = css_ColorMatrix * originalColor; mediump vec3 blendedColor = css_BlendColor(multipliedColor.rgb, css_MixColor.rgb); The css_BlendColor function and blending in general expects valid color values between [0.0, 1.0]. Thus, we should clamp multipliedColor and css_MixColor to [0.0, 1.0]. If we clamp the input colors to css_BlendColor, css_BlendColor will return a color in the range [0.0, 1.0].
Attachments
Patch not for formal review (10.17 KB, patch)
2012-11-26 15:45 PST, Michelangelo De Simone 		no flags
DetailsFormatted DiffDiff
Patch (10.73 KB, patch)
2012-11-27 11:53 PST, Michelangelo De Simone 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michelangelo De Simone
Comment 1 2012-11-26 15:45:31 PST
Created attachment 176096 [details] Patch not for formal review
Max Vujovic
Comment 2 2012-11-26 16:37:20 PST
Comment on attachment 176096 [details] Patch not for formal review Looks good! A couple of comments: View in context: https://bugs.webkit.org/attachment.cgi?id=176096&action=review > Source/WebCore/ChangeLog:8 > + Multiplied color values are now clamped in [0.0, 1.0]. I would mention css_ColorMatrix in this description because "multiplied color values" can be ambiguous. Multiplied color values can refer to either: 1) The result of the css_ColorMatrix * originalColor 2) The result of (css_ColorMatrix * originalColor) * css_MixColor, if the blend mode is multiply. Say something like: "The result of css_ColorMatrix multiplication is now clamped to [0.0, 1.0] before the blending step." And provide a link to the processing model diagram in the spec. > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:276 > + mediump vec4 multipliedColor = clamp(css_ColorMatrix * originalColor, 0.0, 1.0); Looks good. > LayoutTests/css3/filters/custom/custom-filter-color-clamping-expected.html:1 > +<!DOCTYPE HTML> Based on the previous point, make the name of these tests more specific like "custom-filter-clamp-css-color-matrix-result.html". > LayoutTests/css3/filters/custom/custom-filter-color-clamping-expected.html:5 > + <title>Color Clamping Test (negative)</title> Similarly, update the title to specifically say what is being tested: "Tests that the css_ColorMatrix multiplication result is clamped to 0." On the positive test, say: "Tests that the css_ColorMatrix multiplication result is clamped to 1." > LayoutTests/css3/filters/custom/custom-filter-color-clamping-expected.html:8 > + width: 300px; Nit: When there's no space issue, let's just stick with 100px divs in our tests to be consistent. > LayoutTests/css3/filters/custom/custom-filter-color-clamping-negative.html:24 > + background: rgb(45, 45, 45); If you take my suggestion below, change this to rgb(20%, 20%, 20%), because it's easier to do the math with floating point color values (e.g. 0.2) rather than 8-bit (e.g. 45). > LayoutTests/css3/filters/custom/custom-filter-color-clamping-negative.html:39 > + If div's background color is 0, 0, 0 the test has passed. This comment might be easier to follow if it was expressed as math rather than words, e.g.: Let: clamp(value) = A function that clamps values between 0.0 and 1.0 elementColor = vec4(0.2, 0.2, 0.2, 1.0) css_ColorMatrix = Matrix with diagonal (-10.0, -10.0, -10.0, 1.0) css_MixColor = vec4(-0.5, -0.5, -0.5, 1.0) Correct clamped result: result = clamp(css_ColorMatrix * elementColor) * css_MixColor result.r = clamp(-10.0 * 0.2) * 0.5 result.r = clamp(-2.0) * 0.5 result.r = 0.0 * -0.5 result.r = 0.0 result.r = result.g = result.b => black Incorrect unclamped result: result = (css_ColorMatrix * elementColor) * css_MixColor result.r = (-10.0 * 0.2) * 0.5 result.r = -2.0 * -0.5 result.r = 1.0 result.r = result.g = result.b => white > LayoutTests/css3/filters/custom/custom-filter-color-clamping.html:31 > + We're checking that multiplied color values are correctly clamped between [0.0, 1.0]; in order to achieve that Ditto. > LayoutTests/css3/filters/resources/custom-filter-color-clamping-negative.fs:1 > +precision mediump float; Also update the names of the shaders.
Michelangelo De Simone
Comment 3 2012-11-27 11:53:13 PST
Created attachment 176312 [details] Patch
WebKit Review Bot
Comment 4 2012-11-30 10:48:15 PST
Comment on attachment 176312 [details] Patch Clearing flags on attachment: 176312 Committed r136254: <http://trac.webkit.org/changeset/136254>
WebKit Review Bot
Comment 5 2012-11-30 10:48:19 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.