Bug 95614

Summary: [META][CSS Blending] Add support for blendmode to platform code for mac
Product: WebKit Reporter: Rik Cabanier <cabanier>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: achicu, dglazkov, dino, donggwan.kim, eric, krit, mihnea, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 129661, 131354, 132675, 133178, 98450, 99119, 99200, 125804, 126158, 126160, 128805, 129154, 130331, 130337, 130582, 130664, 130891, 130892, 130922, 131153, 131355, 131436, 131824, 131889, 132325, 132578, 132600, 136563    
Bug Blocks: 91908    
Attachments:
Description Flags
First try
webkit.review.bot: commit-queue-
updated expectations file for chromium
simon.fraser: commit-queue-
renamed function per Simon Fraser's request
simon.fraser: review-, simon.fraser: commit-queue-
Updated patch because it became out-of-date
none
Updated patch because previous version was out-of-date webkit.review.bot: commit-queue-

Description Rik Cabanier 2012-08-31 15:03:55 PDT
Previous patch added support for parsing and storing the value in the engine.
This patch will add support to the platform specific code for mac so blend modes should start working for that target.
Comment 1 Rik Cabanier 2012-08-31 16:46:16 PDT
Created attachment 161789 [details]
First try
Comment 2 WebKit Review Bot 2012-09-01 01:58:38 PDT
Comment on attachment 161789 [details]
First try

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

New failing tests:
css3/compositing/effect-blend-mode.html
Comment 3 WebKit Review Bot 2012-09-01 02:53:33 PDT
Comment on attachment 161789 [details]
First try

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

New failing tests:
css3/compositing/effect-blend-mode.html
Comment 4 Rik Cabanier 2012-09-01 20:29:25 PDT
Created attachment 161837 [details]
updated expectations file for chromium
Comment 5 Simon Fraser (smfr) 2012-09-04 16:54:41 PDT
Comment on attachment 161837 [details]
updated expectations file for chromium

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1081
> +#if ENABLE(CSS_COMPOSITING)
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateLayersCompositing();
> +#endif

I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2782
> +void GraphicsLayerCA::updateLayersCompositing()

This should become updateBlending() or updateBlendMode(). The 'layer' in the name is redundant.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:383
> +        CompositingChanged = 1 << 26,

blending.

> Source/WebCore/rendering/RenderLayerBacking.cpp:253
> +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)

'layer' here is redundant.
Comment 6 Rik Cabanier 2012-09-04 20:14:02 PDT
(In reply to comment #5)
> (From update of attachment 161837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161837&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1081
> > +#if ENABLE(CSS_COMPOSITING)
> > +    if (m_uncommittedChanges & CompositingChanged)
> > +        updateLayersCompositing();
> > +#endif
> 
> I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2782
> > +void GraphicsLayerCA::updateLayersCompositing()
> 
> This should become updateBlending() or updateBlendMode(). The 'layer' in the name is redundant.
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:383
> > +        CompositingChanged = 1 << 26,
> 
> blending.

the blendmode is only the first item.
The spec defines keywords for compositing as well and they will follow the same codepath.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> 
> 'layer' here is redundant.

I will fix this.
Comment 7 Rik Cabanier 2012-09-07 00:33:01 PDT
> > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> 
> 'layer' here is redundant.

It's the same as updateLayerFilters, updateLayerOpacity & updateLayerTransform.
Should those functions change as well?


> I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?

To clarify, this function will also be used to set up the compositing modes and group isolation.
Comment 8 Simon Fraser (smfr) 2012-09-07 08:13:12 PDT
(In reply to comment #7)
> > > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> > 
> > 'layer' here is redundant.
> 
> It's the same as updateLayerFilters, updateLayerOpacity & updateLayerTransform.
> Should those functions change as well?

Yes, but you don't have to do that in this patch.

> > I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?
> 
> To clarify, this function will also be used to set up the compositing modes and group isolation.

I still think using the term "compositing" unqualified in the context of GraphicsLayer is very confusing. I think a less confusing but not quite accurate name would be better.
Comment 9 Rik Cabanier 2012-09-11 02:42:50 PDT
Created attachment 163315 [details]
renamed function per Simon Fraser's request
Comment 10 Simon Fraser (smfr) 2012-09-18 14:47:18 PDT
Comment on attachment 163315 [details]
renamed function per Simon Fraser's request

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2794
> +void GraphicsLayerCA::updateBlending()
> +{
> +    primaryLayer()->setBlendMode(m_blendMode);
> +    
> +    if (LayerMap* layerCloneMap = primaryLayerClones()) {
> +        LayerMap::const_iterator end = layerCloneMap->end();
> +        for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it) {
> +            if (m_replicaLayer && isReplicatedRootClone(it->first))
> +                continue;
> +            it->second->setBlendMode(m_blendMode);
> +        } 
> +    }
> +}

Does this play nicely with -webkit-transform: preserve-3d? Before, we've had spew from Core Animation when setting filters on a CATransformLayer.
Comment 11 Rik Cabanier 2012-09-18 15:21:48 PDT
(In reply to comment #10)
> (From update of attachment 163315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163315&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2794
> > +void GraphicsLayerCA::updateBlending()
> > +{
> > +    primaryLayer()->setBlendMode(m_blendMode);
> > +    
> > +    if (LayerMap* layerCloneMap = primaryLayerClones()) {
> > +        LayerMap::const_iterator end = layerCloneMap->end();
> > +        for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it) {
> > +            if (m_replicaLayer && isReplicatedRootClone(it->first))
> > +                continue;
> > +            it->second->setBlendMode(m_blendMode);
> > +        } 
> > +    }
> > +}
> 
> Does this play nicely with -webkit-transform: preserve-3d? Before, we've had spew from Core Animation when setting filters on a CATransformLayer.

It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d. I did not see any warnings.

If you set a blend (or opacity or a filter) on an element, preserve-3d is supposed to be ignored but that doesn't happen today. That would be a new bug if you want me to look into that...
Comment 12 Simon Fraser (smfr) 2012-09-18 15:28:32 PDT
> It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d.
preserve-3d doesn't create a 3D transform. It causes us to create a CATransformLayer, which may not be able to handle compositing filters.
Comment 13 Rik Cabanier 2012-09-18 15:45:10 PDT
(In reply to comment #12)
> > It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d.
> preserve-3d doesn't create a 3D transform. It causes us to create a CATransformLayer, which may not be able to handle compositing filters.

I morphed the example from http://www.webkit.org/blog-files/3d-transforms/transform-style.html.
What I see is that blending is limited to within the group that has the preserve-3d applied. If I apply it on the parent, it doesn't work.

I don't think that that's a big deal because we should fix the bug that blending/opacity/filters removes the effect of the preserve-3d flag.
Comment 14 Simon Fraser (smfr) 2012-10-03 13:47:21 PDT
Comment on attachment 163315 [details]
renamed function per Simon Fraser's request

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1080
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateBlending();

The 'changed' enum name should match the updateFoo.

> LayoutTests/css3/compositing/effect-blend-mode-expected.txt:3
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600

Is this useful as render tree ouput?
Comment 15 Simon Fraser (smfr) 2012-10-03 13:55:09 PDT
Also, this isn't going to work in WebKit1, where parts of the page content are rendered into the window, outside of Core Animation.
Comment 16 Rik Cabanier 2012-10-08 16:41:25 PDT
Created attachment 167643 [details]
Updated patch because it became out-of-date
Comment 17 Rik Cabanier 2012-10-08 16:42:16 PDT
Created attachment 167644 [details]
Updated patch because previous version was out-of-date
Comment 18 WebKit Review Bot 2012-10-08 18:50:03 PDT
Comment on attachment 167644 [details]
Updated patch because previous version was out-of-date

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

New failing tests:
css3/compositing/effect-blend-mode.html
Comment 19 Build Bot 2012-10-08 19:30:50 PDT
Comment on attachment 167644 [details]
Updated patch because previous version was out-of-date

Attachment 167644 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14211600

New failing tests:
css3/compositing/effect-blend-mode.html