Bug 70442 – [chromium] Track when CCLayerImpl properties have changed

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

When drawing layers in chromium, there are two major ways that the actual pixels drawn by a layer can be changed:
   - some content of the layer was updated (done by https://bugs.webkit.org/show_bug.cgi?id=69441)
   - a property changes (requiring the entire layer and the old exposed regions to be redrawn)

This bug is to modify CCLayerImpl to track when the appropriate properties change.

[Shawn Singh]

Created attachment 112388 [details]
Please review, tests will be added soon

[Shawn Singh]

Hi all,

It may be important to note that "layer change" affects screen damage.  if does not imply that layers can be skipped when drawing.  All relevant layers will still need to draw onto the damaged screen areas.

[Dana Jansens]

(In reply to comment #0)
>    - a property changes (requiring the entire layer and the old exposed regions to be redrawn)

If a layer's opacity (or opaque) property changes, the old exposed regions may not be enough.  This may add new exposed regions.

[Shawn Singh]

(In reply to comment #3)
> (In reply to comment #0)
> >    - a property changes (requiring the entire layer and the old exposed regions to be redrawn)
> 
> If a layer's opacity (or opaque) property changes, the old exposed regions may not be enough.  This may add new exposed regions.

You're right - poor wording on my part =).   They are "newly exposed areas", which are known based on "previous draw rects".  

FYI, the exposed rects will come on the final scissor patch, where this "rect buffer" is maintained in the LayerRendererChromium.  I'll be happy to describe my intended changes on that scissor, if anyone asks.

[Vangelis Kokkevis]

Comment on attachment 112388 [details]
Please review, tests will be added soon

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:230
> +    for (size_t i = 0; i < m_children.size(); ++i)

You can call setLayerHasChangedForDescendants here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:240
> +void CCLayerImpl:: setBounds(const IntSize& bounds)

extra space before setBounds

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:104
> +    void setAnchorPoint(const FloatPoint& anchorPoint) { LAYER_CHANGED_IF_NOT_EQUAL(m_anchorPoint, anchorPoint); }

Changing the anchor point will affect the transforms of the entire subtree.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:113
> +    void setOpacity(float opacity) { LAYER_CHANGED_IF_NOT_EQUAL(m_opacity, opacity); }

opacity will change the subtree as well

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:162
> +    void setContentBounds(const IntSize& contentBounds) { LAYER_CHANGED_IF_NOT_EQUAL(m_contentBounds, contentBounds); }

I don't see where contentBounds get used!! :)

[Shawn Singh]

Created attachment 112425 [details]
addressed comments and added unit test

[Shawn Singh]

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:162
> > +    void setContentBounds(const IntSize& contentBounds) { LAYER_CHANGED_IF_NOT_EQUAL(m_contentBounds, contentBounds); }
> 
> I don't see where contentBounds get used!! :)

it gets used implicitly; the LayerChromium object has a virtual accessor contentBounds() which gives information to the CCLayerImpl.  For example, image layers retrieve their content bounds from the Image object itself.

I think its correct to say that a change in contentBounds will change the layer, but not the subtree.  Please let me know if I'm missing something!

[WebKit Review Bot]

Comment on attachment 112425 [details]
addressed comments and added unit test

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

[Dana Jansens]

(In reply to comment #5)
> opacity will change the subtree as well

I can see that opacity affects all layers underneath it, which are parents or siblings or cousins.. But I dont understand why it would specifically affect its children (and not siblings above it in z-order equally).

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #5)
> > opacity will change the subtree as well
> 
> I can see that opacity affects all layers underneath it, which are parents or siblings or cousins.. But I dont understand why it would specifically affect its children (and not siblings above it in z-order equally).

It's my understanding here (please correct me if I'm wrong), that when a layer is marked as "changed" either directly or recursively in this patch, it's implicitly saying that the part of the screen that the layer covers needs to be redrawn.  It's not about which layers need to be redrawn per se and more about which screen space rects need to be redrawn.

[Shawn Singh]

Created attachment 112557 [details]
this should fix the linux build error

[Shawn Singh]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > opacity will change the subtree as well
> > 
> > I can see that opacity affects all layers underneath it, which are parents or siblings or cousins.. But I dont understand why it would specifically affect its children (and not siblings above it in z-order equally).
> 
> It's my understanding here (please correct me if I'm wrong), that when a layer is marked as "changed" either directly or recursively in this patch, it's implicitly saying that the part of the screen that the layer covers needs to be redrawn.  It's not about which layers need to be redrawn per se and more about which screen space rects need to be redrawn.

Would it be more clear if we changed this boolean flag from "layerHasChanged" to "damagesScreen" ?

[Dana Jansens]

(In reply to comment #12)
> Would it be more clear if we changed this boolean flag from "layerHasChanged" to "damagesScreen" ?

IMO yes, or maybe "layerIsDamaged".

[James Robinson]

Comment on attachment 112557 [details]
this should fix the linux build error

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:79
> +    void setMaskLayer(PassRefPtr<CCLayerImpl> maskLayer) { SUBTREE_CHANGED_IF_NOT_EQUAL(m_maskLayer, maskLayer); }

there's no point in inlining these functions now - they are too big. please move the implementation to the .cpp

[Shawn Singh]

Created attachment 112585 [details]
moved implementations to .cpp file

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:61:  The parameter name "maskLayer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:64:  The parameter name "replicaLayer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:78:  The parameter name "drawsContent" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:86:  The parameter name "anchorPoint" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:89:  The parameter name "anchorPointZ" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:92:  The parameter name "masksToBounds" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:95:  The parameter name "opacity" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:98:  The parameter name "position" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:101:  The parameter name "preserves3D" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:110:  The parameter name "sublayerTransform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:113:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:120:  The parameter name "c" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:122:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:143:  The parameter name "contentBounds" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:146:  The parameter name "scrollPosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:152:  The parameter name "scrollDelta" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:161:  The parameter name "doubleSided" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 6 files


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

[Shawn Singh]

Created attachment 112587 [details]
forgot to commit changes

[Shawn Singh]

Hi all,

I did not re-name "layerHasChanged" yet.  I'm willing to do it, but I felt that "damagesScreen" and "layerIsDamaged" are actually misleading and incorrect.

details if you care to read on:

damagesScreen is misleading because it implies that "false" means it does not damage the screen.  but the updateRect would still potentially damage the screen.  But we need this flag to be false in that scenario.

layerIsDamaged doesnt feel right because that sounds like the layer needs repainting/updating.

[James Robinson]

Comment on attachment 112587 [details]
forgot to commit changes

How about this (partially cribbed from GraphicsLayerCA):

layerPropertyChanged()
subtreeChanged()

?

[James Robinson]

To expand a bit I don't think we are changing the layer in any cases here - the layer is still the same layer - we're either changing things about the layer (properties) or things about the layer's descendants (structure or properties on the descendants). If it's useful we can track the type of property change - see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h#L315 - but for now it seems we just want to know if anything happened.

[Shawn Singh]

(In reply to comment #20)
> To expand a bit I don't think we are changing the layer in any cases here - the layer is still the same layer - we're either changing things about the layer (properties) or things about the layer's descendants (structure or properties on the descendants). If it's useful we can track the type of property change - see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h#L315 - but for now it seems we just want to know if anything happened.

layerPropertyChanged is very agreeable with me =)

where does subtreeChanged() come in?  there wasn't going to be any accessor like that...

[James Robinson]

I meant that for the setter, not the getter.  GraphicsLayerCA uses 'noteXXX' for the setters - for example 'noteLayerPropertyChanged', I'm not sure what convention you wanted to follow.

[Shawn Singh]

Created attachment 112603 [details]
renamed the bool flag

hopefully this naming scheme is good

[James Robinson]

Comment on attachment 112603 [details]
renamed the bool flag

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

I don't think the macros in CCLayerImpl.cpp are really worthwhile here - they aren't used all that much (1, 4, and 12 times by my count) and will make debugging a lot harder - for example you can't practically single-step, if we get crashes the line numbers will be bogus.  I'd prefer that you just expand these out.

Other than that I think this is good to go.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:50
> +#define DESCENDANTS_CHANGED_IF_NOT_EQUAL(member, newvalue)   \

there's only one user of this macro, so it doesn't seem worthwhile having it. Just expand it out.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:327
> +void CCLayerImpl::setDebugBorderColor(Color c)
> +{
> +    LAYER_CHANGED_IF_NOT_EQUAL(m_debugBorderColor, c);

we don't typically use 1-letter variable names in WebKit. I think 'color' would be better than 'c', or possibly 'debugBorderColor'

[Shawn Singh]

Created attachment 112744 [details]
addressed reviewer comments

[James Robinson]

Comment on attachment 112744 [details]
addressed reviewer comments

R=me

[Shawn Singh]

Created attachment 112769 [details]
Patch

[Shawn Singh]

the most recent patch is the same, but resolves a conflict with the new setOpaque() in CCLayerImpl (and the appropriate added lines to the test)

[James Robinson]

Comment on attachment 112769 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:242
> +    if (m_bounds != bounds) {

actually now that i look at this again we'd normally do this with early-return to avoid the extra indentation level. nothin' but nitpicks here!

this is fine to land as-is, i think we've gone through enough iterations

[WebKit Review Bot]

Comment on attachment 112769 [details]
Patch

Clearing flags on attachment: 112769

Committed r98667: <http://trac.webkit.org/changeset/98667>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.