[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 #14 from Huang Dongsung <luxtella at company100.net>  2012-10-11 16:48:44 PST ---
(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).

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