Bug 85421 – [chromium] Changes to layer tree structure from the public API need to be tracked properly

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

[Shawn Singh]

When the layer tree is explicitly manipulates by outside code (such as aura), it should be considered as damage.   This is a unique case because it is (for now?) the only property change that comes from the main thread without needing a repaint, and that's also why this bug did not arise until Aura is now trying to manipulate the layer tree though they don't need re-painting.

[Shawn Singh]

Created attachment 139891 [details]
Patch

[Shawn Singh]

Antoine, if this needs to land ASAP, then we may need to find another reviewer since they are out this week =)

[Dana Jansens]

Comment on attachment 139891 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:530
> +        layer->noteLayerPropertyChangedForSubtree();

I would suggest putting a differently named method on the CCLayerImpl public API that is specific to this. layer->setSubtreeStructureHasChanged()? And just have it call the note method so it can remain private. Wdyt?

[Shawn Singh]

(In reply to comment #3)
> (From update of attachment 139891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139891&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:530
> > +        layer->noteLayerPropertyChangedForSubtree();
> 
> I would suggest putting a differently named method on the CCLayerImpl public API that is specific to this. layer->setSubtreeStructureHasChanged()? And just have it call the note method so it can remain private. Wdyt?

The crazy thing is that I toyed with that idea and concluded that if I did that, reviewers would request me to just simplify it the other way. =)

I'm ok with doing it that way, though, it would be more consistent with other pushPropertiesTo methods.

[Dana Jansens]

Comment on attachment 139891 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:530
>>> +        layer->noteLayerPropertyChangedForSubtree();
>> 
>> I would suggest putting a differently named method on the CCLayerImpl public API that is specific to this. layer->setSubtreeStructureHasChanged()? And just have it call the note method so it can remain private. Wdyt?
> 
> The crazy thing is that I toyed with that idea and concluded that if I did that, reviewers would request me to just simplify it the other way. =)
> 
> I'm ok with doing it that way, though, it would be more consistent with other pushPropertiesTo methods.

Haha, damned if you do...

Everything else looks good, except the test. The current test adds + removes, and then you check for property changes. Can you have a test that makes sure both add and remove work as intended individually?

[Adrienne Walker]

Comment on attachment 139891 [details]
Patch

Can you explain a little bit more what this bug is about? It sounds like the problem is that Aura is reparenting layers (that don't need repainting, but just are moved to a different part of the tree), but damage tracking somehow missed this, so these changes get scissored out during partial swap.

I'm a little confused, though. I thought the damage tracker already handled layers that moved (in space), were just added, or were just removed.  What makes this case need to be explicitly marked?

[Shawn Singh]

(In reply to comment #6)
> (From update of attachment 139891 [details])
> Can you explain a little bit more what this bug is about? It sounds like the problem is that Aura is reparenting layers (that don't need repainting, but just are moved to a different part of the tree), but damage tracking somehow missed this, so these changes get scissored out during partial swap.
> 
> I'm a little confused, though. I thought the damage tracker already handled layers that moved (in space), were just added, or were just removed.  What makes this case need to be explicitly marked?

You're exactly right, Aura is "reparenting" layers, but its doing so in an effort to just re-order the layers in the tree.  So, in this case all layers remain in the tree, nothing has any other property change that is already being tracked, nothing needs repainting, the only thing going on is that the children just get re-ordered.  That change wasn't propagating to damage tracking.

Someone has an upcoming Aura patch that may want to do this, for example when mousing over the window's task icon, the window would rise to the top as a nifty UI feature.  That's when we found this issue, and it got reported in http://code.google.com/p/chromium/issues/detail?id=125855

Does that clear it up?

[Dana Jansens]

(In reply to comment #5)
> Everything else looks good, except the test. The current test adds + removes, and then you check for property changes. Can you have a test that makes sure both add and remove work as intended individually?

What if you removed a layer from parent A and added it to parent B, where A and B are at different positions. Would this do the right thing?

How come we don't have a problem when WebKit removed layers?

[Shawn Singh]

(In reply to comment #8)
> (In reply to comment #5)
> > Everything else looks good, except the test. The current test adds + removes, and then you check for property changes. Can you have a test that makes sure both add and remove work as intended individually?
> 
> What if you removed a layer from parent A and added it to parent B, where A and B are at different positions. Would this do the right thing?
> 
> How come we don't have a problem when WebKit removed layers?

The way this patch works, it damages the entire subtree (where the parent is the root, so the parent gets damaged, too) whenever a child is added, or whenever a child is removed.

[Dana Jansens]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > Everything else looks good, except the test. The current test adds + removes, and then you check for property changes. Can you have a test that makes sure both add and remove work as intended individually?
> > 
> > What if you removed a layer from parent A and added it to parent B, where A and B are at different positions. Would this do the right thing?
> > 
> > How come we don't have a problem when WebKit removed layers?
> 
> The way this patch works, it damages the entire subtree (where the parent is the root, so the parent gets damaged, too) whenever a child is added, or whenever a child is removed.

I was imagining the child is not contained within the parent's bounds, it seems like it would miss the layer where it was removed from?

[Shawn Singh]

Created attachment 139909 [details]
Addressed reviewers comments

[Adrienne Walker]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 139891 [details] [details])
> > Can you explain a little bit more what this bug is about? It sounds like the problem is that Aura is reparenting layers (that don't need repainting, but just are moved to a different part of the tree), but damage tracking somehow missed this, so these changes get scissored out during partial swap.
> > 
> > I'm a little confused, though. I thought the damage tracker already handled layers that moved (in space), were just added, or were just removed.  What makes this case need to be explicitly marked?
> 
> You're exactly right, Aura is "reparenting" layers, but its doing so in an effort to just re-order the layers in the tree.  So, in this case all layers remain in the tree, nothing has any other property change that is already being tracked, nothing needs repainting, the only thing going on is that the children just get re-ordered.  That change wasn't propagating to damage tracking.

Oh, I see.  When you change the ordering of children, the draw order changes for that entire subtree.  No layers get removed or added and no positions change from the perspective of the damage tracker, so you need to mark that subtree as damaged to pick up that change.  Sorry for my confusion; that wasn't really clear from the description of this patch or from what the unit tests are doing.

Why do you mark the parent as being damaged?

[Shawn Singh]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #5)
> > > > Everything else looks good, except the test. The current test adds + removes, and then you check for property changes. Can you have a test that makes sure both add and remove work as intended individually?
> > > 
> > > What if you removed a layer from parent A and added it to parent B, where A and B are at different positions. Would this do the right thing?
> > > 
> > > How come we don't have a problem when WebKit removed layers?
> > 
> > The way this patch works, it damages the entire subtree (where the parent is the root, so the parent gets damaged, too) whenever a child is added, or whenever a child is removed.
> 
> I was imagining the child is not contained within the parent's bounds, it seems like it would miss the layer where it was removed from?

Here's the breakdown of possible scenarios:

Within one frame, a child is added:  damage tracker notices its a new area and considers it damaged no matter what.

Within one frame, a child is removed: damage tracker notices this area was tracked previously but no longer exists, and considers it damaged.

Within one frame, the same child is removed, and then added somewhere else: the damage tracker did not notice this, and there's no clear way to notice it.  This is a property change that has to be tracked when the tree is manipulated.

Its very likely damaging the entire subtree is conservative, but personally I don't think this is something we need to take risks to aggressively optimize?


(In reply to comment #12)
> 
> Why do you mark the parent as being damaged?


Good question =)  Seems like its not needed.  I can submit a new patch that uses noteLayerPropertyChangedForDescendants() instead of noteLayerPropertyChangedForSubtree().

[Shawn Singh]

Created attachment 140083 [details]
Addressed reviewers comments

Tested on linux layout tests and unit tests, no obvious regressions

[Dana Jansens]

Comment on attachment 140083 [details]
Addressed reviewers comments

Thanks, the test looks nicer, and I can see why this will work for this Aura scenario. I think I've convinced myself that this should work for any future restacking Aura might want to do, so yay.

[Daniel Erat]

Confirming that the third patch appears to fix the issue that I initially reported in the chromium bug.

[Shawn Singh]

Created attachment 140132 [details]
Same patch updated to tip of tree

[Adrienne Walker]

Comment on attachment 140132 [details]
Same patch updated to tip of tree

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

> Source/WebCore/ChangeLog:8
> +        Unit test added to TreeSynchronizerTest.

Like layout tests, can you call out the unit test you added here, e.g. "Test: TreeSynchronizerTest.syncSimpleTreeAndTrackChangesProperly" so that if it starts failing it's a lot more obvious that it's related to this change?

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:177
> +    m_subtreeStructureHasChanged = true;

This still seems overly permissive to me.  Any time you remove one layer from a parent this will damage all of its children, even though you really want to damage just one child subtree, both in the case that it gets removed permanently or in the case that it gets reinserted into the same parent in a different place?

Maybe this flag should be on the child (in setParent) and not on the parent?

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:201
> +TEST(TreeSynchronizerTest, syncSimpleTreeAndTrackChangesProperly)

Sorry to be picky, but I have a few test nits here:

This test doesn't actually seem to be testing the scenario that this bug is about, i.e. removing and reinserting a layer.  It tests removing and adding but not both.

Can you test the result and not the implementation? In other words, removing or inserting or removing/reinserting a layer should cause damage, regardless of what internal flags are used to track it.  I think somebody should be able to change the implementation and verify that the test still passes.

I prefer finer test granularity rather than sticking a bunch of cases in the same test. If you don't want to repeat yourself, you can use a macro or TEST_F. That way when there's failure, you know that trackLayerRemoved failed vs. trackLayerReinserted just from name alone.

[Dana Jansens]

Comment on attachment 140132 [details]
Same patch updated to tip of tree

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

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:177
>> +    m_subtreeStructureHasChanged = true;
> 
> This still seems overly permissive to me.  Any time you remove one layer from a parent this will damage all of its children, even though you really want to damage just one child subtree, both in the case that it gets removed permanently or in the case that it gets reinserted into the same parent in a different place?
> 
> Maybe this flag should be on the child (in setParent) and not on the parent?

Moving one layer down may make any number of siblings become visible where it was.

[Adrienne Walker]

(In reply to comment #19)
> (From update of attachment 140132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140132&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:177
> >> +    m_subtreeStructureHasChanged = true;
> > 
> > This still seems overly permissive to me.  Any time you remove one layer from a parent this will damage all of its children, even though you really want to damage just one child subtree, both in the case that it gets removed permanently or in the case that it gets reinserted into the same parent in a different place?
> > 
> > Maybe this flag should be on the child (in setParent) and not on the parent?
> 
> Moving one layer down may make any number of siblings become visible where it was.

If you reinsert a layer in a different place in its parent, different siblings will become visible, but only where that subtree used to cover them up.  Damaging that entire subtree during reinsertion seems like it will give the correct target and screen space damage.  Am I missing something?

[Shawn Singh]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 140132 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140132&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:177
> > >> +    m_subtreeStructureHasChanged = true;
> > > 
> > > This still seems overly permissive to me.  Any time you remove one layer from a parent this will damage all of its children, even though you really want to damage just one child subtree, both in the case that it gets removed permanently or in the case that it gets reinserted into the same parent in a different place?
> > > 
> > > Maybe this flag should be on the child (in setParent) and not on the parent?
> > 
> > Moving one layer down may make any number of siblings become visible where it was.
> 
> If you reinsert a layer in a different place in its parent, different siblings will become visible, but only where that subtree used to cover them up.  Damaging that entire subtree during reinsertion seems like it will give the correct target and screen space damage.  Am I missing something?

Probably not missing anything, seems quite correct to me.  I'll test it out and submit a new patch soon.

[Shawn Singh]

I'm having second thoughts about the approach I'm taking on this patch.

I think maybe we should be implementing order-dependent damage tracking instead of order-independent tracking.

The damage tracker receives a layer list, and determines what has changed:
  - any layers in the list that have updated their contents
  - any layers in the list whose properties have changed
  - any layers that no longer exist
  - any layers that are new

  --->  the one that is missing is "changes to the draw order of layers in the list"


The patch I have uploaded now might be an unclean design; it categorizes "changes to draw order" as a "property change of each layer in the subtree", which seems like a bit of a hacky solution the more I think about it.

SO, there are three options:

(1) continue with this patch

(2) divert and do the order-dependent damage tracking solution (will not happen today, so we will have to merge back to 20 later)

(3) do this patch now just for m20, and make another bug that is intended to remove that and do it correctly as order-dependent damage tracking.


derat@, antoine@, enne@ - please let me know which option is better for your priorities.

[Antoine Labour]

Whatever you think is best.
I guess the short term issue is fixing the bug. A conservative approach is fine for that.
If you think you can do a better approach but it'll take longer and have more impact, I suspect it's better to leave it for post 20, and not try to merge it in.

Hence I'd say (3), unless you have an issue with it.

[Shawn Singh]

Created attachment 140342 [details]
Patch

[Shawn Singh]

(In reply to comment #24)
> Created an attachment (id=140342) [details]
> Patch

A note about the most recent patch:

Superficially it might seem like I didn't pay attention to reviewer comments, but actually they sort of canceled each other out.

  - flagging the child only allowed us to remove this flag from the removeChild() side.  That way, it turns out we don't need to test remove/add both.

  - but, to set up the correct scenario for the test case, I still had to set up the tree, and then remove the layer before adding it.  Otherwise there are other property changes that get triggered that make the test a false positive.


As Antoine suggested, we can go for option 3, asking to land this today/asap and I created https://bugs.webkit.org/show_bug.cgi?id=85677 to follow through with a cleaner fix.

[Shawn Singh]

I should also point out that Enne and I discussed offline, and it felt like testing all the way from main thread LayerChromium tweaks through the TreeSynchronizer and LTHi to check damage may be a little heavy handed... especially if our intention will be to undo this patch with a cleaner one after m20.

Thanks =)

[Dana Jansens]

Patch seems good, I agree with doing this right in DamageTracker after, I'm glad you thought of that.

[Adrienne Walker]

Comment on attachment 140342 [details]
Patch

I'm with piman.  Let's land this for now.

[WebKit Review Bot]

Comment on attachment 140342 [details]
Patch

Clearing flags on attachment: 140342

Committed r116195: <http://trac.webkit.org/changeset/116195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.