The result of the mixing function is modulated by the backdrop alpha. A fully opaque backdrop allows the mixing function to be fully realised. A transparent backdrop will cause the final result to be a weighted average between the source color and mixed color with the weight controlled by the backdrop alpha. The value of the new color becomes: Cr = (1 - αb) x Cs + αb x B(Cb, Cs)
Created attachment 181190 [details] Patch
Comment on attachment 181190 [details] Patch Attachment 181190 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15625964 New failing tests: fast/sub-pixel/sub-pixel-accumulates-to-layers.html fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html fast/sub-pixel/transformed-iframe-copy-on-scroll.html
Comment on attachment 181190 [details] Patch Attachment 181190 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15649750 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Created attachment 181737 [details] Patch
Comment on attachment 181737 [details] Patch Attachment 181737 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15763221 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Comment on attachment 181737 [details] Patch Attachment 181737 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15755298 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Comment on attachment 181737 [details] Patch Attachment 181737 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15759300 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Created attachment 182397 [details] Patch
Comment on attachment 182397 [details] Patch Attachment 182397 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15820023 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Created attachment 183302 [details] Patch
Comment on attachment 183302 [details] Patch Attachment 183302 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15940357 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Comment on attachment 183302 [details] Patch Attachment 183302 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15937401 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Comment on attachment 183302 [details] Patch Here's my informal review. This looks good overall. The math checks out. I just have a collection of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=183302&action=review > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:279 > + mediump vec3 weightedColor = (1.0 - multipliedColor.a) * css_MixColor.rgb + multipliedColor.a * blendedColor; This math looks right. > LayoutTests/ChangeLog:14 > + a slight different color space and this test overrides the generic one. I would say "Safari has slightly different color output (~1% difference on the blue channel)". I don't think we know whether color space conversions are causing the error, so I wouldn't say that. Also, no need to say that this test overrides the generic one. That is implied since its in the platform/mac directory. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha-expected.html:8 > + background-color: rgb(179, 166, 128); Add a comment above this line: /* This is the exact expected color. Some platforms may have slight color differences and use a platform-specific expectation file. */ > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:4 > + <meta charset="UTF-8"> No need for this tag. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:5 > + <title>Blending with fractional alpha on destination</title> We've been using a convention in custom filters that test descriptions start with "Tests that...". Let's update the title to: "Tests that blending with a translucent element texture computes the correct color value." > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:14 > + background-color: rgba(100%, 0%, 0%, .5); "0.5" instead of ".5" > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:24 > + The result of mixing function is modulated by destination's It might be more clear to say: "The result of the blending function", not mixing function. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:27 > + color with the weight controlled by the backdrop alpha. Let's use 100 character wrapping if we're going to wrap words. This seems like 60 character wrapping, which is odd to me. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:34 > + B: the formula that does the blending This sounds better: B: the blending function > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:36 > + as: the source alpha (used during the compositing) No need for "the" in "used during the compositing". > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:37 > + Cb: the backdrop color (aka "backdrop color", "original DOM element color") aka "backdrop color" is not needed, since you just said it :) > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:45 > + B(Cb, Cs) = Cb * Cs Because we're using the "multiply" blend operator Move the 'Because we're using the "multiply" blend operator:' comment just above this line, and add a colon to the end. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:51 > + Cr = 0.0, 0.5, 0.0 Wrap this color in parenthesis to be consistent. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:53 > + At this point we composite the texture with the Porter-Duff SourceAtop compositing At this point, <- put a comma. Say SourceAtop "composite" operator, not "compositing" operator, to be consistent with the rest of the text. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:62 > + In regard of the SourceAtop composite operator: "In regard of" sounds awkward. Say "With the SourceAtop composite operator:". > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:67 > + In this case we have Cs = Cr. To be more clear: "In this case, Cs = Cr, where Cr is the result of the blending function." > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:68 > + From this we can gather the following specific formula: Instead, let's say "Substituting Fa and Fb, we get following specific formula:". > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:75 > + The formula is then expanded into: Instead, let's say, "Substituting Cr for Cs:". > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:84 > + The resulting alpha is: This section seems extra indented for no reason. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:97 > + Final component value = Co[Component] * ao + (1.0 - ao) * White[Component] This section also seems indented twice. Also, no need for the [Component] braces in this equation. You don't use that convention anywhere else, so don't introduce it here. Just do: Expected color on screen = Co * ao + (1.0 - ao) * White > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:105 > + IMPORTANT: this is the exact expected color. Some platforms may have slight color differences and Capitalize "this". Add a "may" so that the second part reads "...and may have a platform-specific expectation file." > LayoutTests/platform/mac/css3/filters/custom/custom-filter-blend-fractional-destination-alpha-expected.html:8 > + background-color: rgb(179, 166, 131); Write a comment above this with the exact expected value, so it's easy to compare the color difference.
Created attachment 183342 [details] Patch
Comment on attachment 183342 [details] Patch Attachment 183342 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15946211 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Regarding the Mac EWS failure, you could try removing the black border around the div. It's also getting shaded, and the test expectation doesn't account for that.
Created attachment 183511 [details] Patch
Comment on attachment 183511 [details] Patch Attachment 183511 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15942626 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Created attachment 186715 [details] Patch
Is this supposed to be for review?
(In reply to comment #20) > Is this supposed to be for review? Not yet, I'm just trying to figure out a color issue. Thanks!
Comment on attachment 186715 [details] Patch Attachment 186715 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16372994 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Comment on attachment 186715 [details] Patch Attachment 186715 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16393072 New failing tests: css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html
Created attachment 186938 [details] Patch
(In reply to comment #13) > Here's my informal review. This looks good overall. The math checks out. I just have a collection of nits. Thanks for this great and "thorough" review. > > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:105 > > + IMPORTANT: this is the exact expected color. Some platforms may have slight color differences and I don't agree with the capital T after the semicolon...;)
Comment on attachment 186938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186938&action=review > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:4 > + <title>"Tests that blending with a translucent element texture computes the correct color value.</title> There is a random double quote here. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:22 > + The result of blending function is modulated by destination's alpha. A transparent backdrop will cause the final result Missing "the". The result of [the] blending function. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:23 > + to be a weighted average between the source color and mixed color with the weight controlled by the backdrop alpha. I don't think this statement is entirely accurate. It's not a weighted average with the source color; it's a weighted average with the destination color. I would replace this sentence with: "The result of the blending function is averaged with the destination color using the destination's alpha as the destination's weight." Notice I also didn't use "mixed color" because that's confusing, given that we have "css_MixColor," but that's not what you're talking about here. > LayoutTests/css3/filters/custom/custom-filter-blend-fractional-destination-alpha.html:101 > + IMPORTANT: this is the exact expected color. Some platforms may have slight color differences. Now that you mention it, a capital letter at the beginning of this sentence would be nice. -> "This" :) > LayoutTests/platform/mac/TestExpectations:946 > +# --- Custom Filters --- I would put the comment on why you're skipping this test here as well, referencing the other bug. "This test is currently skipped on Mac for slight color differences, please see the relevant bug: http://webkit.org/b/107487"
Comment on attachment 186938 [details] Patch Not sure why Bugzilla said I set cq?. Setting it back to cq- as it was before.
Created attachment 187168 [details] Patch
(In reply to comment #28) > Created an attachment (id=187168) [details] > Patch Looks good. Thanks for the changes.
Comment on attachment 187168 [details] Patch Clearing flags on attachment: 187168 Committed r142190: <http://trac.webkit.org/changeset/142190>
All reviewed patches have been landed. Closing bug.