Bug 90101 – [CSS Shaders] Parse mix function

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Max Vujovic]

The Filter Effects spec specifies the syntax for the mix function:

<fragment-shader>	none | <uri> | mix(<uri> [ <blend-mode> || <alpha-compositing> ]?)

This is part of the custom function:

custom(<vertex-shader> <fragment-shader>? [ , <vertex-mesh> ]? [ , <params> ]?)

(https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#FilterFunction)

For reference, the mix function processing model is described here: 
https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#processing-model-mix

[Max Vujovic]

Created attachment 152334 [details]
Patch

[Max Vujovic]

Created attachment 152343 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 152343 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Add tests for custom filter mix function parsing.

Maybe you could add a link to the spec here.

> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:-49
> -    

Nit: Avoid making empty changes.

> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:89
> +{

nit: Move "{" on the previous line.

> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:98
> +{

Ditto.

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:91
> +{

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:878
> +            } else {
> +                if (program->fragmentShader())

Nit: It seems like you could merge these two in a single } else if {

> Source/WebCore/css/CSSParser.cpp:7383
> +    CSSParserValueList* argsList = value->function->args.get();

Nit: You can exit early when argList has more than 3 elements and avoid the mixFunction creation.

> Source/WebCore/css/CSSParser.cpp:7384
> +    int argNumber = 0;

argsList already has m_current. I think we can expose that instead of keeping a separate counter.

> Source/WebCore/css/CSSParser.cpp:7415
> +    if (!mixFunction->length())

This should not be possible if we check for the size of the argsList and return early.

> Source/WebCore/css/CSSParser.cpp:7463
> +    int argNumber = 0;

argList should expose the internal m_current position.

> Source/WebCore/css/CSSParser.cpp:7475
> +            hadAtLeastOneCustomShader = value;

What about returning early from here?
if (!value)
   return 0;

> Source/WebCore/css/WebKitCSSMixFunctionValue.h:40
> +class WebKitCSSMixFunctionValue : public CSSValueList {

Don't we need an IDL file for this object?

[WebKit Review Bot]

Comment on attachment 152343 [details]
Patch

Attachment 152343 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13237293

New failing tests:
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-property-parsing-invalid.html
css3/filters/custom/custom-filter-property-parsing.html

[WebKit Review Bot]

Created attachment 152371 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 152343 [details]
Patch

Attachment 152343 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13236330

New failing tests:
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-property-parsing-invalid.html
css3/filters/custom/custom-filter-property-parsing.html

[WebKit Review Bot]

Created attachment 152388 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 153877 [details]
Patch

Updated patch with changes from Alex's review.

[WebKit Review Bot]

Attachment 153877 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:86:  The parameter name "properties" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Max Vujovic]

Created attachment 153882 [details]
Patch

Small fix for style bot.

[Max Vujovic]

Comment on attachment 152343 [details]
Patch

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

Thanks for the review, Alex. I've posted a new patch with the requested changes.

>> LayoutTests/ChangeLog:8
>> +        Add tests for custom filter mix function parsing.
> 
> Maybe you could add a link to the spec here.

Done.

>> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:-49
>> -    
> 
> Nit: Avoid making empty changes.

Done. (My editor was stripping trailing whitespace)

>> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:89
>> +{
> 
> nit: Move "{" on the previous line.

Done.

>> LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:98
>> +{
> 
> Ditto.

Done.

>> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:91
>> +{
> 
> Ditto.

Done.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:878
>> +                if (program->fragmentShader())
> 
> Nit: It seems like you could merge these two in a single } else if {

Done.

>> Source/WebCore/css/CSSParser.cpp:7383
>> +    CSSParserValueList* argsList = value->function->args.get();
> 
> Nit: You can exit early when argList has more than 3 elements and avoid the mixFunction creation.

Done.

>> Source/WebCore/css/CSSParser.cpp:7384
>> +    int argNumber = 0;
> 
> argsList already has m_current. I think we can expose that instead of keeping a separate counter.

Done. I like that.

>> Source/WebCore/css/CSSParser.cpp:7415
>> +    if (!mixFunction->length())
> 
> This should not be possible if we check for the size of the argsList and return early.

Yes, I agree. Removed.

>> Source/WebCore/css/CSSParser.cpp:7463
>> +    int argNumber = 0;
> 
> argList should expose the internal m_current position.

Done.

>> Source/WebCore/css/CSSParser.cpp:7475
>> +            hadAtLeastOneCustomShader = value;
> 
> What about returning early from here?
> if (!value)
>    return 0;

Done.

>> Source/WebCore/css/WebKitCSSMixFunctionValue.h:40
>> +class WebKitCSSMixFunctionValue : public CSSValueList {
> 
> Don't we need an IDL file for this object?

Sure- It would be nice so that the object shows up in CSSOM as WebKitCSSMixFunctionValue instead of CSSValueList. I've added WebKitCSSMixFunctionValue.idl to the patch.

[WebKit Review Bot]

Comment on attachment 153882 [details]
Patch

Attachment 153882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13328232

New failing tests:
compositing/checkerboard.html
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
animations/3d/matrix-transform-type-animation.html
compositing/direct-image-compositing.html
compositing/backface-visibility/backface-visibility-non3d.html
compositing/backing/no-backing-for-clip-overlap.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/backface-visibility/backface-visibility-3d.html
compositing/clip-change.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/generated-content.html
compositing/animation/computed-style-during-delay.html
animations/3d/change-transform-in-end-event.html
animations/animation-direction-reverse-hardware.html
compositing/flat-with-transformed-child.html
animations/3d/transform-perspective.html
compositing/bounds-in-flipped-writing-mode.html
compositing/scrollbar-painting.html
animations/3d/transform-origin-vs-functions.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/backface-visibility/backface-visibility-image.html
compositing/compositing-visible-descendant.html
animations/animation-direction-reverse-fill-mode-hardware.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
animations/animation-direction-reverse-timing-functions-hardware.html

[WebKit Review Bot]

Created attachment 153908 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 153882 [details]
Patch

Attachment 153882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13329215

New failing tests:
compositing/checkerboard.html
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
animations/3d/matrix-transform-type-animation.html
compositing/direct-image-compositing.html
compositing/backing/no-backing-for-clip-overlap.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/backface-visibility/backface-visibility-3d.html
compositing/clip-change.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/generated-content.html
compositing/animation/computed-style-during-delay.html
animations/3d/change-transform-in-end-event.html
animations/animation-direction-reverse-hardware.html
compositing/flat-with-transformed-child.html
animations/3d/transform-perspective.html
compositing/bounds-in-flipped-writing-mode.html
compositing/scrollbar-painting.html
animations/3d/transform-origin-vs-functions.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/backface-visibility/backface-visibility-image.html
compositing/compositing-visible-descendant.html
animations/animation-direction-reverse-fill-mode-hardware.html
compositing/overflow-trumps-transform-style.html
compositing/color-matching/image-color-matching.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-perspective.html

[WebKit Review Bot]

Created attachment 153926 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 154184 [details]
Patch

Refactored the patch a little bit:

Renamed "CustomFilterProgramProperties" to "BlendingAndCompositingSettings". Created platform/graphics/BlendingAndCompositingSettings.h.

Removed the cr-linux expectations because they should be the same as chromium.

[WebKit Review Bot]

Comment on attachment 154184 [details]
Patch

Attachment 154184 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13339069

New failing tests:
compositing/checkerboard.html
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
animations/3d/matrix-transform-type-animation.html
compositing/direct-image-compositing.html
compositing/backface-visibility/backface-visibility-non3d.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/backface-visibility/backface-visibility-3d.html
compositing/clip-change.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/generated-content.html
compositing/animation/computed-style-during-delay.html
animations/3d/change-transform-in-end-event.html
animations/animation-direction-reverse-hardware.html
compositing/flat-with-transformed-child.html
animations/3d/transform-perspective.html
compositing/bounds-in-flipped-writing-mode.html
compositing/scrollbar-painting.html
animations/3d/transform-origin-vs-functions.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/backface-visibility/backface-visibility-image.html
compositing/compositing-visible-descendant.html
animations/animation-direction-reverse-fill-mode-hardware.html
compositing/overflow-trumps-transform-style.html
compositing/backface-visibility/backface-visibility-simple.html
compositing/backface-visibility/backface-visibility-webgl.html
animations/animation-direction-reverse-timing-functions-hardware.html

[WebKit Review Bot]

Created attachment 154212 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 156115 [details]
Patch

Re-running the bots.

Increased ClassTypeBits in CSSValue.h from 5 to 6 to fit the new enum value WebKitCSSMixFunctionValueClass. I think this was the cause of previous cr-linux EWS failures in animations and compositing.

Removed the WebKitCSSMixFunctionValue.idl from this patch. The IDL should probably come in a follow-up patch since this one is already getting big.

[Max Vujovic]

Comment on attachment 156115 [details]
Patch

Bots are green. Setting review flag.

I've split out the IDL for WebKitCSSMixFunctionValue into bug 93011.

[Dirk Schulze]

Comment on attachment 156115 [details]
Patch

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

I am not very happy about the duplication of code that we get. See inline comments.

> Source/WebCore/css/CSSParser.cpp:7433
> +    case CSSValueClear:
> +    case CSSValueSrc:
> +    case CSSValueDst:
> +    case CSSValueSrcOver:
> +    case CSSValueDstOver:
> +    case CSSValueSrcIn:
> +    case CSSValueDstIn:
> +    case CSSValueSrcOut:
> +    case CSSValueDstOut:
> +    case CSSValueSrcAtop:
> +    case CSSValueDstAtop:
> +    case CSSValueXor:
> +    case CSSValuePlus:
> +        return true;

The spec changed to take the long version instead of the shorten one.

> Source/WebCore/css/CSSParser.cpp:7503
> +    // blend-mode: normal | multiply | screen | overlay | darken | lighten | color-dodge |
> +    //             color-burn | hard-light | soft-light | difference | exclusion | hue |
> +    //             saturation | color | luminosity
> +    // alpha-compositing: clear | src | dst | src-over | dst-over | src-in | dst-in |
> +    //                    src-out | dst-out | src-atop | dst-atop | xor | plus

comment needs to be updated as well

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3479
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(EAlphaCompositingMode alphaCompositingMode)
> +    : CSSValue(PrimitiveClass)

needs update as well.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3525
> +template<> inline CSSPrimitiveValue::operator EAlphaCompositingMode() const

This is partly redundant to CSSPrimitiveValue::operator CompositeOperator(). I would like to avoid that. But I see the difficulty. They are not supported by Canvas not by background-composite. Can these value still support a subset of these and it is checked during the parsing of these properties?

> Source/WebCore/css/CSSValueKeywords.in:970
> +src
> +dst
> +src-over
> +dst-over
> +src-in
> +dst-in
> +src-out
> +dst-out
> +src-atop
> +dst-atop
> +// xor
> +plus

Long version of the name please.

> Source/WebCore/platform/graphics/BlendingAndCompositingSettings.h:33
> +#if ENABLE(CSS_SHADERS)

I expect it should work with other features as well. Also we use some of these values on other places already. Can we make sure that they are used on all places? I am also not sure if the bundling is that useful. Even if blend and compositing mode go together, background-compositing just needs EAlphaCompositing.

[Max Vujovic]

Comment on attachment 156115 [details]
Patch

Thanks for review, Dirk! My responses are inline.

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

>> Source/WebCore/css/CSSParser.cpp:7433
>> +        return true;
> 
> The spec changed to take the long version instead of the shorten one.

So it has :)

>> Source/WebCore/css/CSSParser.cpp:7503
>> +    //                    src-out | dst-out | src-atop | dst-atop | xor | plus
> 
> comment needs to be updated as well

Agreed.

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:3479
>> +    : CSSValue(PrimitiveClass)
> 
> needs update as well.

Agreed.

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:3525
>> +template<> inline CSSPrimitiveValue::operator EAlphaCompositingMode() const
> 
> This is partly redundant to CSSPrimitiveValue::operator CompositeOperator(). I would like to avoid that. But I see the difficulty. They are not supported by Canvas not by background-composite. Can these value still support a subset of these and it is checked during the parsing of these properties?

Now that the Compositing spec has changed from short names to long names (e.g. src-over to source-over), we do have the opportunity to share some code, but it seems a little tricky.

Here are the values for CompositeOperator. The ones in [] are not used in the Compositing spec. The others map directly to the Compositing Spec.

CompositeClear
[CompositeCopy]
CompositeSourceOver
CompositeSourceIn
CompositeSourceOut
CompositeSourceAtop
CompositeDestinationOver
CompositeDestinationIn
CompositeDestinationOut
CompositeDestinationAtop
CompositeXOR
[CompositePlusDarker]
[CompositePlusLighter]
[CompositeDifference]

The following are missing from CompositeOperator, but defined in the Compositing spec:
"plus" (CompositePlusLighter has the same effect.)
"source" (CompositeCopy has the same effect.)
"destination" (There is no analogous CompositeOperator for this.)

I'm not sure how to reuse CompositorOperator cleanly while respecting these differences. Here are some options:

(1) Do we introduce CompositePlus, CompositeSource, and CompositeDestination?
Then, (CompositePlus, CompositePlusLighter) and (CompositeSource, CompositeCopy) are semantic duplicates.
We'll also need to support these at the platform level. CompositePlus and CompositePlusLighter will need to fall through to the same case in the platform code.
We'll have to figure out what to do with CompositeDestination on Mac because I don't see a kCGBlendModeDestination in the docs [1], but that's a separate issue.

(2) Do we introduce CompositeDestination, and map (CSSValueSource => CompositeCopy) and (CSSValuePlus => CompositePlusLighter)?
Then, we have a problem in CSSPrimitiveValueMappings because we need a one to many mapping for CompositeOperator. For -webkit-background-composite, we want (CompositeCopy => CSSValueCopy), but for the alpha-compositing property from the spec, we want (CompositeCopy => CSSValueSource).

What are your thoughts?

[1] http://developer.apple.com/library/ios/#DOCUMENTATION/GraphicsImaging/Reference/CGContext/Reference/reference.html

>> Source/WebCore/css/CSSValueKeywords.in:970
>> +plus
> 
> Long version of the name please.

Agreed.

>> Source/WebCore/platform/graphics/BlendingAndCompositingSettings.h:33
>> +#if ENABLE(CSS_SHADERS)
> 
> I expect it should work with other features as well. Also we use some of these values on other places already. Can we make sure that they are used on all places? I am also not sure if the bundling is that useful. Even if blend and compositing mode go together, background-compositing just needs EAlphaCompositing.

I see your point. Bundling blending and compositing is convenient for shaders. However, it doesn't seem as useful for the Blending and Compositing spec, since blending and compositing are separate properties there. I'll see what I can do about this.

[Max Vujovic]

Created attachment 156495 [details]
Patch

Updated the patch based on Dirk's review. Running the bots.

In this patch:

- I removed the short-form CSS values for alpha compositing that I had added before. Instead, I reused the long-form ones used by -webkit-background-composite that match the spec. I put a FIXME to add "lighter" and "destination", since these are being discussed on www-style right now.
- I moved the blend modes to GraphicsTypes.h, right after the CompositeOperator enum.
- I removed the BlendingAndCompositingSettings.h file. I renamed the BlendingAndCompositingSettings struct to CustomFilterProgramMixSettings, and moved it to CustomFilterProgram.h. Now, it's specific to Shaders, since we determined that bundling blending and compositing is useful for Shaders but not for Blending and Compositing.

[Max Vujovic]

Comment on attachment 156495 [details]
Patch

Bots are green. Setting r?.

[Dirk Schulze]

Comment on attachment 156495 [details]
Patch

Looks great. r=me.

[WebKit Review Bot]

Comment on attachment 156495 [details]
Patch

Rejecting attachment 156495 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
bkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
ValuePool.o
cc1plus: warnings being treated as errors
Source/WebCore/css/CSSValue.cpp: In member function 'void WebCore::CSSValue::reportMemoryUsage(WebCore::MemoryObjectInfo*) const':
Source/WebCore/css/CSSValue.cpp:161: error: enumeration value 'WebKitCSSMixFunctionValueClass' not handled in switch
make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSValue.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13451001

[Max Vujovic]

Created attachment 156759 [details]
Patch

Rebased patch for landing. Added WebKitCSSMixFunctionValue::reportDescendantMemoryUsage.

[Dirk Schulze]

Comment on attachment 156759 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 156759 [details]
Patch

Clearing flags on attachment: 156759

Committed r124820: <http://trac.webkit.org/changeset/124820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.