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.
Created attachment 112388 [details] Please review, tests will be added soon
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.
(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.
(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.
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!! :)
Created attachment 112425 [details] addressed comments and added unit test
> > 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!
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
(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).
(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.
Created attachment 112557 [details] this should fix the linux build error
(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" ?
(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".
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
Created attachment 112585 [details] moved implementations to .cpp file
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.
Created attachment 112587 [details] forgot to commit changes
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.
Comment on attachment 112587 [details] forgot to commit changes How about this (partially cribbed from GraphicsLayerCA): layerPropertyChanged() subtreeChanged() ?
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.
(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...
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.
Created attachment 112603 [details] renamed the bool flag hopefully this naming scheme is good
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'
Created attachment 112744 [details] addressed reviewer comments
Comment on attachment 112744 [details] addressed reviewer comments R=me
Created attachment 112769 [details] Patch
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)
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
Comment on attachment 112769 [details] Patch Clearing flags on attachment: 112769 Committed r98667: <http://trac.webkit.org/changeset/98667>
All reviewed patches have been landed. Closing bug.