Bug 95614 - [META][CSS Blending] Add support for blendmode to platform code for mac
Summary: [META][CSS Blending] Add support for blendmode to platform code for mac
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
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
Blocks: 91908
  Show dependency treegraph
 
Reported: 2012-08-31 15:03 PDT by Rik Cabanier
Modified: 2014-09-11 01:07 PDT (History)
9 users (show)

See Also:


Attachments
First try (476.21 KB, patch)
2012-08-31 16:46 PDT, Rik Cabanier
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
updated expectations file for chromium (477.09 KB, patch)
2012-09-01 20:29 PDT, Rik Cabanier
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
renamed function per Simon Fraser's request (478.44 KB, patch)
2012-09-11 02:42 PDT, Rik Cabanier
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Updated patch because it became out-of-date (1.30 KB, patch)
2012-10-08 16:41 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Updated patch because previous version was out-of-date (477.85 KB, patch)
2012-10-08 16:42 PDT, Rik Cabanier
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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