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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98504





--- Comment #13 from Max Vujovic <mvujovic at adobe.com>  2012-10-11 16:17:21 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.