Bug 104012 – [CSS Shaders] Add the last blending step

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

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)

[Michelangelo De Simone]

Created attachment 181190 [details]
Patch

[Build Bot]

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

[WebKit Review Bot]

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

[Michelangelo De Simone]

Created attachment 181737 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Michelangelo De Simone]

Created attachment 182397 [details]
Patch

[Build Bot]

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

[Michelangelo De Simone]

Created attachment 183302 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Max Vujovic]

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.

[Michelangelo De Simone]

Created attachment 183342 [details]
Patch

[Build Bot]

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

[Max Vujovic]

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.

[Michelangelo De Simone]

Created attachment 183511 [details]
Patch

[Build Bot]

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

[Michelangelo De Simone]

Created attachment 186715 [details]
Patch

[Dean Jackson]

Is this supposed to be for review?

[Michelangelo De Simone]

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

[Build Bot]

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

[Build Bot]

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

[Michelangelo De Simone]

Created attachment 186938 [details]
Patch

[Michelangelo De Simone]

(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...;)

[Max Vujovic]

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"

[Max Vujovic]

Comment on attachment 186938 [details]
Patch

Not sure why Bugzilla said I set cq?. Setting it back to cq- as it was before.

[Michelangelo De Simone]

Created attachment 187168 [details]
Patch

[Max Vujovic]

(In reply to comment #28)
> Created an attachment (id=187168) [details]
> Patch

Looks good. Thanks for the changes.

[WebKit Review Bot]

Comment on attachment 187168 [details]
Patch

Clearing flags on attachment: 187168

Committed r142190: <http://trac.webkit.org/changeset/142190>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.