Bug 98504 – [CSS Shaders] Implement overlay, color-dodge, color-burn, hard-light, soft-light blend modes.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

Implement the overlay, color-dodge, color-burn, hard-light and soft-light blend mode in the CSS Shaders mix function. For example:
-webkit-filter: custom(none mix(shader.fs multiply));

This will blend the special css_MixColor symbol in the fragment shader with the DOM element texture.

This bug is follow-up of Bug 93870 to implement all of the following: overlay, color-dodge, color-burn, hard-light and soft-light.

CSS Shaders can support all of Separable blend modes after this bug. This bug does not cover Non-separable blend modes, which is tricky to implement.

https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingseparable
https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingnonseparable

[Dongseong Hwang]

Created attachment 167299 [details]
Patch

[Max Vujovic]

Comment on attachment 167299 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [CSS Shaders] Implement overlay, color-dodge, color-burn, hard-light, soft-light blend modes.

Thanks for the patch, Huang!

I have some ideas about how to factor this now that you're implementing the more complicated blend modes :)

Basically, I want to break up the GLSL css_Blend function into two functions, css_BlendColor and css_BlendComponent. If we need to do per component blending, css_BlendColor calls css_BlendComponent for each color component. I've provided the details below. Please let me know if something is not clear.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:189
>      const char* expression = 0;

Rename this blendColorExpression.

Give a default value of:
"Co = vec3(blendComponent(Cb.r, Cs.r), blendComponent(Cb.g, Cs.g), blendComponent(Cb.b, Cs.b));"

For the simpler blend modes that don't need to blend by component, we will overwrite this default value.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:190
> +    const char* auxiliaryExpression = "";

Rename this to blendComponentExpression.

Give a default value of:
"return 0.0;"

This default value is just so this function compiles when it is not used.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:193
>          expression = "Cs";

The existing blend mode expressions should change to this format:

blendColorExpression = "Co = Cs";
...

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:213
>      case BlendModeOverlay:

This entire case can be:

blendComponentExpression = SHADER(
    if (Cb <= 0.5)
        Co = Cs * (2.0 * Cb);
    else
        Co = Cs + (2.0 * Cb - 1.0) - (Cs * (2.0 * Cb - 1.0)); 
);

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:237
>      case BlendModeColorDodge:

This entire case can be:

blendComponentExpression = SHADER(
    float divisor = 1.0 - Cs;
    Co = (divisor != 0.0) ? (Cb / divisor) : 1.0;
);

Notice the divide by zero check I added. We need to make sure we don't ever divide by zero because different GPUs often behave differently in that case. I've seen some discard the entire fragment when a division by zero occurs in the shader.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:245
> +            mediump vec3 one = vec3(1.0, 1.0, 1.0);

Even though I'm asking you to remove this line, a nicer way to write it in GLSL is:
mediump vec3 one = vec3(1.0);

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:247
> +            if (Cs.r >= 1.0)

I think it's better to move this clamping operation to the css_BlendColor function below because we always want the blending result to be between 0.0 and 1.0. Check out the GLSL clamp function: http://www.opengl.org/sdk/docs/manglsl/xhtml/clamp.xml

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:256
>      case BlendModeColorBurn:

The rest of the cases can also be written in the format I mentioned.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:265
> +            mediump vec3 B = one - min(one, (one - Cb) / Cs);

When you write this expression, be careful to avoid the divide by zero.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:341
>      return String::format(SHADER(

Add a function:

mediump vec3 css_BlendComponent(mediump float Cb, mediump float Cs)
{
    float Co;
    %s;
    return Co;
}

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:342
>          mediump vec3 css_Blend(mediump vec3 Cb, mediump vec3 Cs)

Rename this function to css_BlendColor.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:345
>              return %s;

Replace this function body with:

vec3 Co;
%s
return clamp(Co, 0.0, 1.0);

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:347
> +    ), auxiliaryExpression, expression);

Replace this with "blendComponentExpression, blendColorExpression".

[Dongseong Hwang]

(In reply to comment #2)
I really appreciate your detailed and improved suggestions. All explanations are precise!
I'll update it. :)

[Dongseong Hwang]

Created attachment 167959 [details]
Patch

[Dongseong Hwang]

Comment on attachment 167959 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:234
> +        blendComponentExpression = SHADER(

I don't check the divide by zero, because 1.0 - Cs can not zero if Cs < 1.0.
I don't use clamp, because we are checking Cs < 1.0, not Co < 1.0. (clamp(genIType x, genIType  minVal, genIType  maxVal))
Please let me know if I misunderstand your suggestion.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:248
> +        blendComponentExpression = SHADER(

Ditto.

[Dongseong Hwang]

(In reply to comment #2)
> I have some ideas about how to factor this now that you're implementing the more complicated blend modes :)

I updated a new patch as you suggested.
Blend shader code becomes similar to blend formulas is spec! https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#blendingseparable

[Max Vujovic]

Comment on attachment 167959 [details]
Patch

This looks great! Thanks for making the changes. I don't have any other change requests.

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

>> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:234
>> +        blendComponentExpression = SHADER(
> 
> I don't check the divide by zero, because 1.0 - Cs can not zero if Cs < 1.0.
> I don't use clamp, because we are checking Cs < 1.0, not Co < 1.0. (clamp(genIType x, genIType  minVal, genIType  maxVal))
> Please let me know if I misunderstand your suggestion.

Ok, the divide by zero check should be fine.

Not using clamp should be fine, too.

Regarding clamp, my assumption was that some of these expressions could produce a Co that is out of the range [0.0, 1.0]. That would be a problem because the alpha compositing operation after this blending operation expects colors in the range [0.0, 1.0]. 

However, I analyzed the blending expressions and it appears they're all carefully crafted to make sure Co ends up in [0.0, 1.0] if Cs and Cb are in [0.0, 1.0]. Thus, I think what you have is ok, and we don't need an extra clamp(Co, 0.0, 1.0).

Unrelated to this bug, I think we need to clamp multipliedColor before passing it to css_BlendColor, so that Cs is in the [0.0, 1.0] range. A matrix multiplication can definitely push multipliedColor out of the [0.0, 1.0] range. I'll make a bug for this issue.

For the record, OS X Grapher [1] is amazing at analyzing these expressions. I made 3D graphs which mapped Co => z, Cb => x, Cb => y, and verified that z stayed between 0 and 1 while x and y were between 0 and 1.

[1]: http://en.wikipedia.org/wiki/Grapher

[Max Vujovic]

(In reply to comment #7)
> Unrelated to this bug, I think we need to clamp multipliedColor before passing it to css_BlendColor, so that Cs is in the [0.0, 1.0] range. A matrix multiplication can definitely push multipliedColor out of the [0.0, 1.0] range. I'll make a bug for this issue.

I've filed bug 98962.

[Dongseong Hwang]

(In reply to comment #7)
> Regarding clamp, my assumption was that some of these expressions could produce a Co that is out of the range [0.0, 1.0]. That would be a problem because the alpha compositing operation after this blending operation expects colors in the range [0.0, 1.0]. 

I see now why you mentioned clamp. Thank for explanation!

> For the record, OS X Grapher [1] is amazing at analyzing these expressions. I made 3D graphs which mapped Co => z, Cb => x, Cb => y, and verified that z stayed between 0 and 1 while x and y were between 0 and 1.

Wow! :)

[Dongseong Hwang]

Created attachment 168290 [details]
Patch

[Dongseong Hwang]

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

There is typo in the changed test. I commented out some elements. This patch fixes that.

[Dean Jackson]

Comment on attachment 168290 [details]
Patch

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

I think this looks great, but I have one minor concern that I'd like Max's feedback on: we're now doing more work on the simple compositing operations - calling per-component functions. I wonder if it would be worth only dropping to css_BlendComponent if necessary. 

Hmm... I see that Max did in fact make this suggestion "For the simpler blend modes that don't need to blend by component, we will overwrite this default value."

In that case I will r-. It should be pretty easy to add this.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:184
>      // Cs: is the source color
>      // Cb: is the backdrop color

This comment is now incorrect. These are color components (although see general comment on the patch)

> LayoutTests/css3/filters/custom/custom-filter-blend-modes-expected.html:54
> +    .hard-light-expected {
> +        background-color: rgb(0%, 44%, 50%);
> +    }

Is there a reason why you used % here but numbers elsewhere?

> LayoutTests/css3/filters/custom/custom-filter-blend-modes.html:213
> +    .color-burn-expected {
> +        /*
> +            ColorBurn:
> +
> +            Co = if(Cs > 0)
> +                     1 - min(1, (1 - Cb) / Cs)
> +                 else
> +                     0
> +
> +            r = 0
> +            g = 1 - min(1, (1 - 0.3) / 0.6) = 0
> +            b = 1 - min(1, (1 - 0.5) / 0.5) = 0
> +        */
> +        background-color: rgb(0, 0, 1);

I'm not sure why this is rgb(0, 0, 1) when you calculated 0, 0, 0.

Also, this shows that we may need a more complex test for burn, since a completely black result doesn't really exercise much.

[Max Vujovic]

(In reply to comment #12)
> (From update of attachment 168290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168290&action=review
> 
> I think this looks great, but I have one minor concern that I'd like Max's feedback on: we're now doing more work on the simple compositing operations - calling per-component functions. I wonder if it would be worth only dropping to css_BlendComponent if necessary. 
> 
> Hmm... I see that Max did in fact make this suggestion "For the simpler blend modes that don't need to blend by component, we will overwrite this default value."

Unless the driver compiler is very smart, it's more efficient if we avoid calling the per-component functions for the simpler blend modes.

I didn't comment on this again since Huang's variant looks a little better. It's only managing one string for css_BlendComponent instead of two for css_BlendColor and css_BlendComponent. It's a small performance / complexity tradeoff.

In this case, I'm leaning toward performance. Could you make that change please, Huang?

> 
> In that case I will r-. It should be pretty easy to add this.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:184
> >      // Cs: is the source color
> >      // Cb: is the backdrop color
> 
> This comment is now incorrect. These are color components (although see general comment on the patch)

If Huang makes the previous change, then it should be something like:
// Cs: is the source color in css_BlendColor() and the source color component in css_BlendComponent()
...

> 
> > LayoutTests/css3/filters/custom/custom-filter-blend-modes-expected.html:54
> > +    .hard-light-expected {
> > +        background-color: rgb(0%, 44%, 50%);
> > +    }
> 
> Is there a reason why you used % here but numbers elsewhere?

I think the % maps nicely to the hand calculations above, but sometimes the % causes color rounding problems and we have to use the 8-bit number variants. (I wish we didn't have to! It would be nice if we could specify a color tolerance per test.)

> 
> > LayoutTests/css3/filters/custom/custom-filter-blend-modes.html:213
> > +    .color-burn-expected {
> > +        /*
> > +            ColorBurn:
> > +
> > +            Co = if(Cs > 0)
> > +                     1 - min(1, (1 - Cb) / Cs)
> > +                 else
> > +                     0
> > +
> > +            r = 0
> > +            g = 1 - min(1, (1 - 0.3) / 0.6) = 0
> > +            b = 1 - min(1, (1 - 0.5) / 0.5) = 0
> > +        */
> > +        background-color: rgb(0, 0, 1);
> 
> I'm not sure why this is rgb(0, 0, 1) when you calculated 0, 0, 0.
> 
> Also, this shows that we may need a more complex test for burn, since a completely black result doesn't really exercise much.

Good point- perhaps we could use different input colors whose result isn't black.

[Dongseong Hwang]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 168290 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168290&action=review
> > 
> > I think this looks great, but I have one minor concern that I'd like Max's feedback on: we're now doing more work on the simple compositing operations - calling per-component functions. I wonder if it would be worth only dropping to css_BlendComponent if necessary. 
> > 
> > Hmm... I see that Max did in fact make this suggestion "For the simpler blend modes that don't need to blend by component, we will overwrite this default value."
> 
> Unless the driver compiler is very smart, it's more efficient if we avoid calling the per-component functions for the simpler blend modes.
> 
> I didn't comment on this again since Huang's variant looks a little better. It's only managing one string for css_BlendComponent instead of two for css_BlendColor and css_BlendComponent. It's a small performance / complexity tradeoff.
> 
> In this case, I'm leaning toward performance. Could you make that change please, Huang?

Of course, I'll change. I did miss those. sorry.

> 
> > 
> > In that case I will r-. It should be pretty easy to add this.
> > 
> > > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:184
> > >      // Cs: is the source color
> > >      // Cb: is the backdrop color
> > 
> > This comment is now incorrect. These are color components (although see general comment on the patch)
> 
> If Huang makes the previous change, then it should be something like:
> // Cs: is the source color in css_BlendColor() and the source color component in css_BlendComponent()
> ...

Thanks!

> 
> > 
> > > LayoutTests/css3/filters/custom/custom-filter-blend-modes-expected.html:54
> > > +    .hard-light-expected {
> > > +        background-color: rgb(0%, 44%, 50%);
> > > +    }
> > 
> > Is there a reason why you used % here but numbers elsewhere?
> 
> I think the % maps nicely to the hand calculations above, but sometimes the % causes color rounding problems and we have to use the 8-bit number variants. (I wish we didn't have to! It would be nice if we could specify a color tolerance per test.)

I tested this test on Chromium, Gtk and Qt. I approached native ways. If this test passed on all platforms, I leaved %. If not, I found numbers to make this test pass.
A color tolerance per test is holy grail. We really need it to reduce creating tests and maintenance cost for each platform.

> 
> > 
> > > LayoutTests/css3/filters/custom/custom-filter-blend-modes.html:213
> > > +        background-color: rgb(0, 0, 1);
> > 
> > I'm not sure why this is rgb(0, 0, 1) when you calculated 0, 0, 0.
> > 
> > Also, this shows that we may need a more complex test for burn, since a completely black result doesn't really exercise much.
> 
> Good point- perhaps we could use different input colors whose result isn't black.

Sadly, linux chromium failed when rgb(0, 0, 0).

[Dongseong Hwang]

(In reply to comment #12)
> (From update of attachment 168290 [details])
> Also, this shows that we may need a more complex test for burn, since a completely black result doesn't really exercise much.

I agree. I'll make new test or update existing test for burn, also.

[Dongseong Hwang]

Created attachment 168543 [details]
Patch

[Dongseong Hwang]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 168290 [details] [details] [details])
> > > Hmm... I see that Max did in fact make this suggestion "For the simpler blend modes that don't need to blend by component, we will overwrite this default value."

Done. I did misunderstand what Max instructed.

> > If Huang makes the previous change, then it should be something like:
> > // Cs: is the source color in css_BlendColor() and the source color component in css_BlendComponent()
> > ...

Done.

> > I think the % maps nicely to the hand calculations above, but sometimes the % causes color rounding problems and we have to use the 8-bit number variants. (I wish we didn't have to! It would be nice if we could specify a color tolerance per test.)> 
> A color tolerance per test is holy grail. We really need it to reduce creating tests and maintenance cost for each platform.

Currently, we can set the tolerance of image differences per execution of new-run-webkit-tests. I wish we could specify a color tolerance per test as Max said.

We need the way to send meta data per test to new-run-webkit-tests. I think there are two solutions.
1. Specify the tolerance in a file name. For example, custom-filter-blend-modes-TOLERANCE=5.html
2. Create a meta file. If a test does not have a meta file, apply the default setting. For example, custom-filter-blend-modes-meta.txt including TOLERANCE=5.

IMHO the pixel test is the only way to check the regression of CSS Shaders. All platforms output slightly different pixels. We need a solution soon.

> > > Also, this shows that we may need a more complex test for burn, since a completely black result doesn't really exercise much.
> > 
> > Good point- perhaps we could use different input colors whose result isn't black.

Done.

[WebKit Review Bot]

Comment on attachment 168543 [details]
Patch

Clearing flags on attachment: 168543

Committed r132268: <http://trac.webkit.org/changeset/132268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.