Bug 106226 – [CSS Shaders] Implement color and luminosity non-separable blend modes

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

Support for non-separable blend modes is missing and shell be added.

[Michelangelo De Simone]

Created attachment 188387 [details]
Patch not for formal review

[Max Vujovic]

Comment on attachment 188387 [details]
Patch not for formal review

Looks good overall. Here's some preliminary feedback. I'm still looking at the patch though, so I might have more feedback.

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

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:421
> +        mediump float lum(mediump vec3 c)

Two issues here:

1) You need to prepend "css_" to the new function names. If you just define a function named lum(vec3), then the author code cannot define its own function named lum(vec3) since GLSL ES does not allow redefinitions of functions [1]. That's why we reserve the "css_" prefix for all globally scoped variables and function we need to define. Also, capitalize the next character to follow convention. For example, "lum" will become "css_Lum",

2) We should not write these functions in all of our shaders. We should only write them in shaders that actually need them. This is so the GPU driver doesn't have to spend extra time parsing and potentially compiling them.

From the spec, we can see:
- 'hue' blend mode needs SetLum, SetSat, Set, Lum
- ‘saturation’ blend mode needs SetLum, SetSat, Sat, Lum
- ‘color’ blend mode needs SetLum, Lum
- ‘luminosity’ blend mode needs SetLum, Lum

You could create local booleans for each of the functions like "needsSetLumFunction, needsSetSatFunction, ..." and set them in the appropriate cases. Then, you can add the function to the shader only if their flag is set. Since there's a pattern, you could also create two booleans like needsLuminosityHelperFunctions and needsSaturationHelperFunctions. Up to you :)

[1]: GLSL ES 1.0.17 Spec, Section 4.2.7: http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf

[Max Vujovic]

Comment on attachment 188387 [details]
Patch not for formal review

Your explanation in the test case is awesome. Just a couple more nits:

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

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:423
> +            return 0.3 * c.r + 0.59 * c.g + 0.11 * c.b;

We've been using the same capitalization as the spec in the implementation (e.g. Cs, Cb). Can you match the capitalization with the spec here? So, c -> C.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:425
> +        mediump vec3 clipColor(mediump vec3 c)

c -> C

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:427
> +            mediump float l = lum(c);

l -> L

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:436
> +        mediump vec3 setLum(mediump vec3 c, mediump float l)

c -> C

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color-expected.html:7
> +            /* This is the EXACT color value we expect. Some platforms may have slight different

Nit: No need to capitalize exact :). Also, you're missing an "a" and a "ly". I would change this sentence to "Some platforms may have a slightly different color result."

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:13
> +            background-color: rgb(100%, 0%, 0%);

I would avoid blending a pure red with a pure green because it doesn't seem to test all of the color channels thoroughly. Could you blend two different colors where each color component has unique, non-zero, non-one value.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:22
> +    The final color of #destination depends on the "luminosity" blend mode, which is a non-separable

This is excellent documentation. Thank you very much.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:23
> +    blend mode. This means that colors are blended togheter as a whole, not component by component.

Together is misspelled.

[Michelangelo De Simone]

Created attachment 189196 [details]
Patch

[Max Vujovic]

Comment on attachment 189196 [details]
Patch

This looks good to me. I just commented on two tiny spelling nits. Thanks for making the changes!

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

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color-expected.html:9
> +            background-color: rgb(108, 157, 255);

These values look good. I also sanity checked them with Photoshop's "color" blend mode :)

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:23
> +    blend mode. This means that colors are blended togheter as a whole, not component by component.

"togheter" should be "together".

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity-expected.html:9
> +            background-color: rgb(98, 33, 0);

This also matches Photoshop's result.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:23
> +    blend mode. This means that colors are blended togeter as a whole, not component by component.

"togeter" should be "together".

[Michelangelo De Simone]

(In reply to comment #5)

> "togheter" should be "together".

This word is my everlasting curse.:)

> "togeter" should be "together".

Ditto.:)

I'm gonna update the patch with this small fix as soon as the EWS acknowleges that it's fine.
Thank you very much for your informal, though thorough, review.:)

[Michelangelo De Simone]

Created attachment 189335 [details]
Patch

[Rik Cabanier]

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

Please also add tests that use alpha in the background and foreground colors. This is to make sure that we're handling premultiplied colors correctly.

[Max Vujovic]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=189335) [details] [details]
> > Patch
> 
> Please also add tests that use alpha in the background and foreground colors. This is to make sure that we're handling premultiplied colors correctly.

I don't think it is necessarily this test's responsibility to verify that we are handling premultiplied alpha correctly. We have other tests for specifically that, which use fractional alpha values in the background and foreground. See:
custom-filter-composite-fractional-source-alpha.html
custom-filter-composite-fractional-destination-alpha.html

The intention of this test was to only test the new blending function, which doesn't operate on alpha values.

Nonetheless, we could add fractional alpha values to the background and foreground colors to make these tests verify the entire pipeline. Given the existing tests I mentioned, do you think that would be useful?

[Rik Cabanier]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created an attachment (id=189335) [details] [details] [details]
> > > Patch
> > 
> > Please also add tests that use alpha in the background and foreground colors. This is to make sure that we're handling premultiplied colors correctly.
> 
> I don't think it is necessarily this test's responsibility to verify that we are handling premultiplied alpha correctly. We have other tests for specifically that, which use fractional alpha values in the background and foreground. See:
> custom-filter-composite-fractional-source-alpha.html
> custom-filter-composite-fractional-destination-alpha.html
> 
> The intention of this test was to only test the new blending function, which doesn't operate on alpha values.
> 
> Nonetheless, we could add fractional alpha values to the background and foreground colors to make these tests verify the entire pipeline. Given the existing tests I mentioned, do you think that would be useful?

That is not enough to verify that the blend modes are handling alpha correctly. You should add tests (like canvas-blend-solid.html) to try all combinations.

[Michelangelo De Simone]

(In reply to comment #10)

> That is not enough to verify that the blend modes are handling alpha correctly. You should add tests (like canvas-blend-solid.html) to try all combinations.

SG. As of now, I'm testing basically testing (BlendMode: Cb-Cs):

Color: solid-solid
Luminosity: solid-solid

You suggest that we should also have:

Color: alpha-solid
Color: solid-alpha
Color: alpha-alpha
Luminosity: alpha-solid
Luminosity: solid-alpha
Luminosity: alpha-alpha

Is that correct?

[Rik Cabanier]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > That is not enough to verify that the blend modes are handling alpha correctly. You should add tests (like canvas-blend-solid.html) to try all combinations.
> 
> SG. As of now, I'm testing basically testing (BlendMode: Cb-Cs):
> 
> Color: solid-solid
> Luminosity: solid-solid
> 
> You suggest that we should also have:
> 
> Color: alpha-solid
> Color: solid-alpha
> Color: alpha-alpha
> Luminosity: alpha-solid
> Luminosity: solid-alpha
> Luminosity: alpha-alpha
> 
> Is that correct?

yes. This shouldn't be very hard. Use Photoshop or Illustrator to get the result values.

[Max Vujovic]

(In reply to comment #10)
> That is not enough to verify that the blend modes are handling alpha correctly. You should add tests (like canvas-blend-solid.html) to try all combinations.

I like that test a lot. Thanks for pointing that out.

[Michelangelo De Simone]

Created attachment 189595 [details]
Patch

[Max Vujovic]

Comment on attachment 189595 [details]
Patch

Looks good. Just some formatting nits:

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

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color-expected.html:8
> +        #destination {

I would call these divs ["solid-solid", "alpha-solid", "solid-alpha", "alpha-alpha"], or ["destination-solid-source-solid", "destination-alpha-source-solid", ...].

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:16
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') color source-atop), mix_color 0.2 0.4 0.8 1.0);

Nit: Indentation is off.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:22
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') color source-atop), mix_color 0.2 0.4 0.8 1.0);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:28
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') color source-atop), mix_color 0.2 0.4 0.8 0.5);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-color.html:34
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') color source-atop), mix_color 0.2 0.4 0.8 0.5);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:16
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') luminosity source-atop), mix_color 0.1 0.2 0.4 1.0);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:22
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') luminosity source-atop), mix_color 0.2 0.4 0.8 1.0);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:28
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') luminosity source-atop), mix_color 0.2 0.4 0.8 0.5);

Nit: Indentation.

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity.html:34
> +          -webkit-filter: custom(none mix(url('../resources/mix-color.fs') luminosity source-atop), mix_color 0.2 0.4 0.8 0.5);

Nit: Indentation.

[Michelangelo De Simone]

Created attachment 190084 [details]
Patch

[Dean Jackson]

Comment on attachment 190084 [details]
Patch

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

Looks good. Upload a patch with the small changes and I'll cq+.

> Source/WebCore/ChangeLog:10
> +            - Lum(C): returns the luminosity for the color C
> +            - ClipColor(C): clips color C
> +            - SetLum(C, l): sets the luminosity l on the color C

Might as well include the css_ prefix here to make it clear.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:426
> +    if (needsLuminosityHelperFunctions)
> +        blendFunctionString.append(SHADER(
> +            mediump float css_Lum(mediump vec3 C)

Since the single statement here spans multiple lines, you should use braces. "One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines." - http://www.webkit.org/coding/coding-style.html

> LayoutTests/css3/filters/custom/custom-filter-nonseparable-blend-mode-luminosity-expected.html:7
> +        /* There are exactly the color values we expect. Some platforms may have slight different
> +           color result. */

You forgot the typo that Max pointed out earlier -> "may have a slightly different"

[Michelangelo De Simone]

Created attachment 190355 [details]
Patch

[Michelangelo De Simone]

Created attachment 190357 [details]
Patch for landing

[Michelangelo De Simone]

Created attachment 190358 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190358 [details]
Patch for landing

Clearing flags on attachment: 190358

Committed r144118: <http://trac.webkit.org/changeset/144118>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.