Bug 98504

Summary: [CSS Shaders] Implement overlay, color-dodge, color-burn, hard-light, soft-light blend modes.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, dino, mvujovic, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 93870    
Bug Blocks: 71392    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Dongseong Hwang 2012-10-05 03:05:03 PDT
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
Comment 1 Dongseong Hwang 2012-10-05 03:34:11 PDT
Created attachment 167299 [details]
Patch
Comment 2 Max Vujovic 2012-10-08 11:48:53 PDT
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".
Comment 3 Dongseong Hwang 2012-10-09 15:16:11 PDT
(In reply to comment #2)
I really appreciate your detailed and improved suggestions. All explanations are precise!
I'll update it. :)
Comment 4 Dongseong Hwang 2012-10-10 01:46:44 PDT
Created attachment 167959 [details]
Patch
Comment 5 Dongseong Hwang 2012-10-10 01:56:30 PDT
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.
Comment 6 Dongseong Hwang 2012-10-10 02:00:21 PDT
(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
Comment 7 Max Vujovic 2012-10-10 15:35:26 PDT
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
Comment 8 Max Vujovic 2012-10-10 16:04:19 PDT
(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.
Comment 9 Dongseong Hwang 2012-10-10 23:41:05 PDT
(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! :)
Comment 10 Dongseong Hwang 2012-10-11 15:34:59 PDT
Created attachment 168290 [details]
Patch
Comment 11 Dongseong Hwang 2012-10-11 15:36:44 PDT
(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.
Comment 12 Dean Jackson 2012-10-11 15:48:00 PDT
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.
Comment 13 Max Vujovic 2012-10-11 16:17:21 PDT
(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.
Comment 14 Dongseong Hwang 2012-10-11 16:48:44 PDT
(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).
Comment 15 Dongseong Hwang 2012-10-11 16:58:06 PDT
(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.
Comment 16 Dongseong Hwang 2012-10-12 23:04:05 PDT
Created attachment 168543 [details]
Patch
Comment 17 Dongseong Hwang 2012-10-12 23:23:48 PDT
(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.
Comment 18 WebKit Review Bot 2012-10-23 14:35:07 PDT
Comment on attachment 168543 [details]
Patch

Clearing flags on attachment: 168543

Committed r132268: <http://trac.webkit.org/changeset/132268>
Comment 19 WebKit Review Bot 2012-10-23 14:35:13 PDT
All reviewed patches have been landed.  Closing bug.