[Webkit-unassigned] [Bug 97859] [CSS Shaders] Implement all composite operators.

Fri Sep 28 13:32:22 PDT 2012

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

--- Comment #10 from Max Vujovic <mvujovic at adobe.com>  2012-09-28 13:32:47 PST ---
(From update of attachment 166202)
Thanks for the patch! Looking good. I have a couple of comments:

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

> Source/WebCore/ChangeLog:3
> +        [CSS Shaders] Implement all composite operators.

You haven't implemented "destination" from the Compositing and Blending spec [1], so this title should probably say "Implement all composite operators except destination".

We'll need to figure out what to do with destination eventually, but it doesn't have to be in this bug :).
We'll need a new CompositeOperator, CompositeDestination. Unrelated to the shaders implementation, there's no kCGBlendModeDestination in CoreGraphics [2].

[1]: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#alpha-compositing 
[2]: Search for "CGBlendMode" in http://developer.apple.com/library/ios/#documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html

> Source/WebCore/css/CSSParser.cpp:-7467
> -    // FIXME: Add CSSValueDestination and CSSValueLighter when the Compositing spec updates.

You haven't implemented "destination", so this FIXME needs to be updated to say that. It shouldn't be removed quite yet.

> Source/WebCore/css/CSSParser.cpp:7467
> +    return ident >= CSSValueClear && ident <= CSSValuePlusLighter && ident != CSSValuePlusDarker;

@achicu, @dino: Can one of you comment on the following issue?

According to the Compositing and Blending spec [1], "plus-lighter" is not a valid CSS value. You want "lighter".

However, we can't just add the "lighter" CSS value and map it to CompositePlusLighter. The CSS value "plus-lighter" for -webkit-background-composite is already mapped to CompositePlusLighter in CSSPrimitiveValueMappings.h [3].

We have a couple of options for resolving this:
(1) Change the CSS value "plus-lighter" to "lighter" and break -webkit-background-composite. Then, we should probably also change "plus-darker" to "darker".
(2) Keep the CSS value "plus-lighter". Add the CSS value "lighter". Add CompositeOperatorLighter. Then, we have two synonymous CompositeOperators, CompositeOperatorPlusLighter and CompositeOperatorLighter. Platform code might have to deal with these two CompositeOperators, which is not ideal.
(3) Create separate CSSValue <-> CompositeOperator mappings for -webkit-background-composite and shaders/CSS compositing. For shaders, we would have CSSValueLighter <-> CompositeOperatorPlusLighter. For -webkit-background-composite, we would have CSSValuePlusLighter <-> CompositeOperatorPlusLighter. I'm not sure how to implement this nicely, though.

[3]: Search for "template<> inline CSSPrimitiveValue::operator CompositeOperator() const" in CSSPrimitiveValueMappings.h.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:251
>      case CompositeClear:

The expressions look good.

> LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:5
> +    <!-- This test passes if each horizontal pair of squares are exactly the same color. -->

@achicu, @dino: Could one of you chime in on this following point? I wouldn't want to make Huang rewrite his tests if it's just my opinion.

This type of test is particularly good for testing blend modes, which define how to mix two RGB colors. However, I'm not sure it's the best way to test alpha compositing operators.

Conventionally, alpha compositing operators are related to which parts of the resulting image come from the source (css_MixColor) and which parts come from the destination (the element texture), based on the alpha channel.

That's why the spec [4] shows examples that composite an offset blue square surrounded by transparency with a differently offset yellow square surrounded by transparency. If you look at our existing compositing test for "source-atop" [5], you'll see we use that technique. I would prefer that we use that same technique to test all of the compositing operators. It makes it easy to visually verify the correctness of the test, since it looks exactly like the classic compositing images from the spec. Also, each test case verifies multiple different combinations of alpha values for each composite operator, instead of just one combination per composite operator.

I do like that you used a ref test instead of a pixel test, and I like that you put all of the composite operators in this single ref test. I suggest you keep the same overall structure, where each row is a test case, and the left side and right side of each row must match. However, instead of single-color squares, each "square" should look like one of compositing result images from the spec. In the test file, the left image can be rendered with shaders, and the right image can be rendered with divs. In the reference file, both sides can be rendered with divs. 

[4]: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#porterduffcompositingoperators_clear
[5]: LayoutTests/css3/filters/custom/custom-filter-composite-source-atop.html

> LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:47
> +            Fa = Î±b; Fb = 1 â€“ Î±s

I'm seeing non-ASCII characters in these expressions on my machine, such as a "plus equals" sign [6].

[6]: http://www.w3.org/TR/MathML2/glyphs/02A/U02A72.png

> LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:279
> +        -webkit-filter: custom(none mix(url('../resources/mix-color.fs') normal plus-lighter), mix_color 0.0 0.6 0.5 0.5);

Same as before. "plus-lighter" is not a valid CSS compositing operator according to the spec.

> LayoutTests/css3/filters/custom/custom-filter-property-parsing-invalid-expected.txt:-82
> -Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite : custom(none mix(url(shader) plus-lighter))

Please keep this text expectation in place.

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:-46
> -testInvalidFilterRule("Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite", "custom(none mix(url(shader) plus-lighter))");

Please keep this test case in place.

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