Bug 72520 – [chromium] Create CCDamageTracker class to determine regions of change for a surface

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This patch will provide a CCDamageTracker class and the associated tests.  It is the "core guts" of the scissoring optimization, encapsulated into a single class.   This class computes the region of a surface that has been "damaged" and would need to be redrawn.  In combination with clipping, this damage will be used to compute the final scissor.

[Shawn Singh]

Created attachment 115462 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115462 [details]
Patch

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

[Nat Duca]

Comment on attachment 115462 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:336
> +                renderSurface->updateDamageRectForNextFrame();

Why is this calling through the RenderSurface? I think this should be explicit renderSurface->setDamanageRect(...)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:84
> +    bool useDamageTracking;

Tracking->Tracker

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:107
> +    void updateDamageRectForNextFrame() { m_damageRect = m_damageTracker->computeDamageForNextFrame(); }

Delete this function. Add setDamageRect. Have the place that calls this do setDamageRect(renderSurface->damageTracker()->getDamangeForNextFrame(...));

it will be more obvious what its doing that way.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:132
> +    FloatRect m_damageRect;

Who consumes this value? I'd prefer you not add it here if its not used... personally, I'd prefer to see this passed around to functions that need it rather than persisted. Again, this is because it lying here begs the question "when is this valid, when is it stale, when is it correct?"

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:42
> +#define VERIFY_FLOAT_RECTS_ALMOST_EQUAL(expected, actual)               \

EXPECT_FLOAT_RECT_EQ

This should go in a common file and the other tests using it shoudl be modified too

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:50
> +    // FIXME: this function is redundant with similar code from CCLayerTreeHostCommonTest.

Make a CCLayerTreeTestCommon.h

We established this convention with CCSchedulerTestCommon.h

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:51
> +    void executeCalculateDrawTransformsAndVisibility(CCLayerImpl* root)

If this isn't using state from CCDamageTrackerTest then it shouldn't be a member function

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:62
> +    void resetUpdateRectsRecursive(CCLayerImpl* layer)

Should this just be a method on the CCLayerImpl? It feels like it. CCLayerImpl::setUpdateRectRecursive(const FloatRect& value) --- rather than having a reset

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:75
> +        root->resetPropertyChangedFlagForSubtree();

Why do we sometimes say "ForSubtree" and other times Rescursive? Its the same thing, right?

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:78
> +    void emulateRenderingForSimpleTree(CCLayerImpl* root, FloatRect& rootDamageRect)

Rendering? We draw...

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:86
> +    void emulateRenderingForComplexTree(CCLayerImpl* root, FloatRect& rootDamageRect, FloatRect& childDamageRect)

Try to fuse complex tree and simple tree? I am confused what the difference is.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:100
> +    PassRefPtr<CCLayerImpl> setUpSimpleLayerTree()

Lets have a a header full of different types of representative layer trees. Or a directory test/trees/ with .h files that create different kinds of trees. But "simple" should be a LOT more descriptive. That way someone else who needs a layer tree of a certain type can grab one easily.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:176
> +        emulateRenderingForComplexTree(root.get(), rootDamageRect, childDamageRect);

I'm confused because here you're mixing creation of a tree with testing things about the damage tracker. That should be part of a test. YOu mihgt just want a test that is "sanity check that the damage rect is the world when you create it"

[Shawn Singh]

Created attachment 115740 [details]
addressed reviewer comments

I think this is finally reaching a good state for official review; I'll be happy to break it into smaller patches if this is too much to gulp down at once. thanks\!

[Nat Duca]

Comment on attachment 115740 [details]
addressed reviewer comments

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:42
> +    static WTF::PassOwnPtr<CCDamageTracker> create(CCRenderSurface* owningSurface);

Why didn't you remove the ownership here? That's a big no-no in my opinion, and I haven't heard you make a compelling argument otherwise.

[Shawn Singh]

Created attachment 115829 [details]
removed owning surface

[Shawn Singh]

Created attachment 115844 [details]
updated to tip of tree and removed CCRenderSurface pointer on damage tracker interface

[WebKit Review Bot]

Comment on attachment 115844 [details]
updated to tip of tree and removed CCRenderSurface pointer on damage tracker interface

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

[Shawn Singh]

Created attachment 115853 [details]
hopefully fixes compile errors

[Adrienne Walker]

Comment on attachment 115853 [details]
hopefully fixes compile errors

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

Awesome tests.  Can you add a few more?

* damage on masks
* damage on replica masks as well
* a transform change that causes the layer rect edges to not be parallel to its surface's edges, e.g. rotation or perspective?
* damage on a layer where bounds() != contentBounds()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:46
> +static FloatRect HACKYScrollFix(CCLayerImpl* layer, const FloatRect& rect)

I think all changes to scrolling I was planning on should be made at this point.  Can we manage without this now?

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:132
> +    // Swap next and previous history maps, simply by changing the index.
> +    m_currentRectHistoryIndex = (m_currentRectHistoryIndex + 1) % 2;

nit: I think this would be cleaner to have two RectMaps and do a swap() and clear(), rather than needing to do indexing and modulo math everywhere.  You wouldn't even need the saveRect and removeRect functions and could just ask the correct rectmap directly.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:141
> +        if (layer->renderSurface() && layer->id() != targetSurfaceLayerID)

We use this logic all over the place.  Can you add maybe something like a layer->isRenderSurfaceForTarget(id) function?

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:214
> +        if (!oldLayerRect.isEmpty())
> +            targetDamageRect.uniteIfNonZero(oldLayerRect);

nit: No need for this conditional.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:252
> +        if (!oldSurfaceRect.isEmpty())
> +            targetDamageRect.uniteIfNonZero(oldSurfaceRect);

nit: No need for this conditional.

[Nat Duca]

Comment on attachment 115853 [details]
hopefully fixes compile errors

I'm happy. Enne's got great points. When you have their lgtm, you have mine as well.

[Vangelis Kokkevis]

Comment on attachment 115853 [details]
hopefully fixes compile errors

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

Great stuff!

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:191
> +    //        - the old layer region also damges the surface, because this region is now exposed.

typo: damages

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:212
> +        // Note oldLayerRect is already in target space.

If the RS content rect changed then the oldLayerRect won't necessarily be in the correct space.  Please see the following comment too.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:246
> +    if (renderSurface->surfacePropertyChanged()) {

Since if surfacePropertyChanged() we mark the entire RS as damaged, should we take that into account in extendDamageForLayer() ?  Is there any point in computing a damageRect for it layer by layer?

[Shawn Singh]

Comment on attachment 115853 [details]
hopefully fixes compile errors



Thanks very much for the reviews.  I'll submit a new patch soon!   There was only one item to reply:

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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:246
>> +    if (renderSurface->surfacePropertyChanged()) {
> 
> Since if surfacePropertyChanged() we mark the entire RS as damaged, should we take that into account in extendDamageForLayer() ?  Is there any point in computing a damageRect for it layer by layer?

I think there is still a case to compute damageRect: for example, a portion of a "descendant surface" changes because of a layer, and that same surface also translates.  In that case, when we draw the descendant surface, we should be able to draw only the small region that changed.  But when we draw the ancestor surface, however, then the entire surface damages the target.

It seemed reasonable to try and design the damage tracker to support this from the beginning, but it should be pointed out that the first version of scissoring won't be able to take advantage of this.  We will only be able to use a different scissor for each render surface once we've been able to cache those textures across frames.  For now, we'll just have to use the root surface's damageRect as the scissor for all surfaces.

Does this sound reasonable?

[Shawn Singh]

Created attachment 116707 [details]
addressed reviewers feedback

[WebKit Review Bot]

Attachment 116707 [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/WebKit/chromium/tests/CCDamageTrackerTest.cpp:295:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:563:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 20 files


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

[Shawn Singh]

Created attachment 116708 [details]
forgot to commit style fix, should be fixed now

[Shawn Singh]

Notes about the most recent patch submitted:

(1) Enne: a lot has changed, could you please give it one more careful review before we ask James?

(2) Nat: can you please check the new if-statement at the beginning of CCDamageTracker::updateDamageRectForNextFrame() and please let me know if you would prefer that we pass that surfacePropertyChanged flag as a function arg instead?  For now, I felt that this is likely to change soon anyway, and adding more args will become unwieldy.

(3) Everyone: All unit tests pass, but I suspect there are still some final remaining issues with contentRect changes and transform hierarchy of replicas/masks.  I have created bug https://bugs.webkit.org/show_bug.cgi?id=73055 to follow up after this lands, after having the opportunity to test the damage tracker in action.

Thanks!

[Adrienne Walker]

Comment on attachment 116708 [details]
forgot to commit style fix, should be fixed now

Thanks for making m_currentRectHistory and m_nextRectHistory explicit and not using modulo arithmetic.  I find this code a lot easier to read.  And, nice work sorting out masks and replica masks--I've broken that code a lot, so more testing is great to have.

I forget what the resolution to our discussion about layers where content bounds != bounds.  Did that (or should that?) get filed as a separate bug against update rect calculations?

[Shawn Singh]

(In reply to comment #18)
> (From update of attachment 116708 [details])
> Thanks for making m_currentRectHistory and m_nextRectHistory explicit and not using modulo arithmetic.  I find this code a lot easier to read.  And, nice work sorting out masks and replica masks--I've broken that code a lot, so more testing is great to have.
> 
> I forget what the resolution to our discussion about layers where content bounds != bounds.  Did that (or should that?) get filed as a separate bug against update rect calculations?

We had agreed that this was an issue with the updateRect correctness, not the damage rect.  So we filed https://bugs.webkit.org/show_bug.cgi?id=72919

[Adrienne Walker]

(In reply to comment #19)

> We had agreed that this was an issue with the updateRect correctness, not the damage rect.  So we filed https://bugs.webkit.org/show_bug.cgi?id=72919

I tried searching for that but couldn't dig that up.  Thanks.  :)

This is looking good to me.  My only question at this point is how antialiased layer edges interact with damage tracking.  Is that also an updateRect issue or does that affect how you apply damage to surfaces?

[Shawn Singh]

(In reply to comment #20)
> (In reply to comment #19)
> 
> > We had agreed that this was an issue with the updateRect correctness, not the damage rect.  So we filed https://bugs.webkit.org/show_bug.cgi?id=72919
> 
> I tried searching for that but couldn't dig that up.  Thanks.  :)
> 
> This is looking good to me.  My only question at this point is how antialiased layer edges interact with damage tracking.  Is that also an updateRect issue or does that affect how you apply damage to surfaces?

So here's how I imagined it working:

Currently damageRect is just a floatRect.  I'm expecting that the enclosingIntRect conversion will handle it correctly when we actually apply the scissor itself in the next patch.

If we find that is incorrect, then it should be straightforward to add an "if layer should use AA, then inflate damage by 0.5" few lines of code to the damage tracker inside of the extendDamageXXX functions, to mimic exactly the same behavior that happens when the layer is actually drawn.

~Shawn

[Adrienne Walker]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)

> > This is looking good to me.  My only question at this point is how antialiased layer edges interact with damage tracking.  Is that also an updateRect issue or does that affect how you apply damage to surfaces?
> 
> So here's how I imagined it working:
> 
> Currently damageRect is just a floatRect.  I'm expecting that the enclosingIntRect conversion will handle it correctly when we actually apply the scissor itself in the next patch.
> 
> If we find that is incorrect, then it should be straightforward to add an "if layer should use AA, then inflate damage by 0.5" few lines of code to the damage tracker inside of the extendDamageXXX functions, to mimic exactly the same behavior that happens when the layer is actually drawn.

Yeah, I think inflating the damage rect by 0.5 pixels should be sufficient when a layer is using antialiasing.  We might need to expose some more data to know when to do this, since the use antialiased edges are dependent on several layer properties.  That all seems pretty straightforward to add in a later patch.  Just curious, thanks.  :)

This patch looks good to me, then.

[James Robinson]

Comment on attachment 116708 [details]
forgot to commit style fix, should be fixed now

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

Enne covered the logic so I have nothin' but style nits. R=me, but a decent number of things to follow up on.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:54
> +void CCDamageTracker::updateDamageRectForNextFrame(const WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer)

nit: remove the WTF:: from WTF::Vector, there's a using declaration in Vector.h that pulls Vector into the global namespace and WebKit style is to leave out the qualifier

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:153
> +FloatRect CCDamageTracker::computeDamageFromActiveLayers(const WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)

drop WTF::

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:215
> +    originTransform.translate3d(-0.5 * layer->bounds().width(), -0.5 * layer->bounds().height(), 0);

should just be translate()

can you comment (briefly) why this translation is taking place?

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:278
> +            replicaOriginTransform.translate3d(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y(), 0);

why translate3d(.., ..., 0) instead of translate(.., ...)? the latter will save a round of multiplications per component

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:280
> +            replicaOriginTransform.translate3d(-layer->replicaLayer()->position().x(), -layer->replicaLayer()->position().y(), 0);

translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:294
> +        replicaOriginTransform.translate3d(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y(), 0);

translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:296
> +        replicaOriginTransform.translate3d(-layer->replicaLayer()->position().x(), -layer->replicaLayer()->position().y(), 0);

same here - should just be translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:42
> +    static WTF::PassOwnPtr<CCDamageTracker> create();

remove WTF:: please

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:43
> +    virtual ~CCDamageTracker() { }

please out-of-line this destructor (leave just a declaration here and put the definition in the .cpp), the OwnPtr<HashMap> destructor is pretty darn big and we don't want it in the header.

should this be virtual? i don't see any subclasses or protected members

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:45
> +    void updateDamageRectForNextFrame(const WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer);

drop WTF::

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:51
> +    FloatRect computeDamageFromActiveLayers(const WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID);

remove WTF:: please

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:89
> +    root->renderSurface()->setContentRect(IntRect(IntPoint(), IntSize(500.0f, 500.0f)));

drop the .0f bit from here - we don't use those in WebKit except when it's necessary to force math to happen in a certain type, and in this case it's just passed to an int parameter anyway. See http://www.webkit.org/coding/coding-style.html "Floating point literals"

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:91
> +    child->setPosition(FloatPoint(100.0f, 100.0f));

drop the .0f's here too

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:117
> +    root->renderSurface()->setContentRect(IntRect(IntPoint(), IntSize(500.0f, 500.0f)));

drop .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:119
> +    child1->setPosition(FloatPoint(100.0f, 100.0f));

drop .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:122
> +    child1->setOpacity(0.5f); // with a child that drawsContent, this will cause the layer to create its own renderSurface.

0.5 is fine here

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:125
> +    child2->setPosition(FloatPoint(11.0f, 11.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:130
> +    grandChild1->setPosition(FloatPoint(200.0f, 200.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:135
> +    grandChild2->setPosition(FloatPoint(190.0f, 190.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:188
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(0.0f, 0.0f, 500.0f, 500.0f), rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:210
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(190.0f, 190.0f, 16.0f, 18.0f), childDamageRect);
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(0.0f, 0.0f, 500.0f, 500.0f), rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:220
> +    child->setUpdateRect(FloatRect(10.0f, 11.0f, 12.0f, 13.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:225
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(110.0f, 111.0f, 12.0f, 13.0f), rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:229
> +    child->setUpdateRect(FloatRect(10.0f, 11.0f, 12.0f, 13.0f));

ditto on .0f

i won't annotate the rest in this patch

[Shawn Singh]

Comment on attachment 116708 [details]
forgot to commit style fix, should be fixed now


OK I'll make all those fixes right now.


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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:215
>> +    originTransform.translate3d(-0.5 * layer->bounds().width(), -0.5 * layer->bounds().height(), 0);
> 
> should just be translate()
> 
> can you comment (briefly) why this translation is taking place?

The translate is part of computing the origin transform itself, since its not stored on layers.  I'll put that in a comment.

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:43
>> +    virtual ~CCDamageTracker() { }
> 
> please out-of-line this destructor (leave just a declaration here and put the definition in the .cpp), the OwnPtr<HashMap> destructor is pretty darn big and we don't want it in the header.
> 
> should this be virtual? i don't see any subclasses or protected members

I'll just remove the destructor entirely, since its empty anyway.

[James Robinson]

(In reply to comment #24)
> >> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:43
> >> +    virtual ~CCDamageTracker() { }
> > 
> > please out-of-line this destructor (leave just a declaration here and put the definition in the .cpp), the OwnPtr<HashMap> destructor is pretty darn big and we don't want it in the header.
> > 
> > should this be virtual? i don't see any subclasses or protected members
> 
> I'll just remove the destructor entirely, since its empty anyway.

It actually isn't empty, running the destructor for this class invokes the destructors for all members including the two OwnPtr<HashMap<...> >'s. These destructors are pretty big so we want them to be compiled in the .cpp instead of in the .h so we don't get a copy in every translation unit that #includes this file. So please do declare it in the header and define it in the .cpp even though it looks empty. Otherwise the compile and link time overhead just gets crazy (and it's already pretty bad for WebKit).

[Shawn Singh]

Created attachment 116829 [details]
fixed style nits, moved non-virtual destructor to .cpp

[James Robinson]

Comment on attachment 116829 [details]
fixed style nits, moved non-virtual destructor to .cpp

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:42
> +    static WTF::PassOwnPtr<CCDamageTracker> create();

still have a WTF:: here, please remove

[Shawn Singh]

Created attachment 116839 [details]
oops, removed remaining WTF now

[Shawn Singh]

Created attachment 116860 [details]
previous patch is missing an include, fails commit queue

[Shawn Singh]

Created attachment 116862 [details]
remove one more unnecessary include, this time checked that it compiles

[WebKit Review Bot]

Comment on attachment 116862 [details]
remove one more unnecessary include, this time checked that it compiles

Clearing flags on attachment: 116862

Committed r101318: <http://trac.webkit.org/changeset/101318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.