cc: Fix resource eviction with impl-side painting.

cc: Fix resource eviction with impl-side painting.

When evicting resources with impl-side painting we need wait until
the active tree is without evicted resources before we can draw.

This moves the ContentsTexturesPurged state from LTHI to the LTI
and makes LTHI::canDraw() return false when the active LTI has
ContentsTexturesPurged set. The result is no change in behavior
when we only have one tree, while with impl-side painting when
we have two trees, canDraw() will not return true until the new
tree without ContentsTexturesPurged set has been activated.

BUG=170157
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177532

[nduca, 2013-01-17 09:08:11]

Can you explain why "we need to wait until the active tree is without evicted
resources before we can draw" a bit more? I'm not sure i grok this very well,
but its mainly because I'm not too familiar with this state.

[reveman, 2013-01-17 16:48:50]

On 2013/01/17 09:08:11, nduca wrote:
> Can you explain why "we need to wait until the active tree is without evicted
> resources before we can draw" a bit more? I'm not sure i grok this very well,
> but its mainly because I'm not too familiar with this state.

When we tell the prioritized resource manager to reduce memory usage it will
evict resources as needed. The result is that the layers in the current active
tree might have invalid resources and if we draw these layers at this time bad
things happen.

This is currently solved by preventing the scheduler from issuing a draw until
we commit a new tree with only valid resources (see LTHI::canDraw() and
LTHI::m_contentsTexturesPurged).

With impl-side painting enabled, committing a new tree is not enough to
guarantee that we can draw without using invalid resources. Commit will in this
case only update the pending tree while the active tree with invalid resources
is unaffected until we activate the pending tree. In this case we need to
prevent drawing until we have a new active tree without invalid resources.

This only affects layers that use the prioritized resource manager to allocate
resources. Scrollbar layers are causing the crash reported in bug 170157.
Picture layers don't have this problem.

[enne (OOO), 2013-01-17 17:31:14]

I wonder if the tree activation function needs to take this into account so that
if the active tree is in this state, then we immediately activate the pending
tree.  Maybe checking canDraw() would be sufficient.

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc#newcode259
cc/layer_tree_host.cc:259: bool newImplTreeHasNoEvictedResources =
!m_contentsTextureManager->linkedEvictedBackingsExist();
Could you explain why this code moved from ThreadProxy to here?  I'm not sure I
follow.

[reveman, 2013-01-17 18:49:49]

I don't think we want to change pending tree activation. Activating it before
visible tiles are ready will cause the page to flash.

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc#newcode259
cc/layer_tree_host.cc:259: bool newImplTreeHasNoEvictedResources =
!m_contentsTextureManager->linkedEvictedBackingsExist();
On 2013/01/17 17:31:14, enne wrote:
> Could you explain why this code moved from ThreadProxy to here?  I'm not sure
I
> follow.

We need to call ResetContentsTexturesPurged() on the syncTree which is not the
same as the activeTree. Normally it's the same but in the case of impl-side
painting the syncTree is the pendingTree.

I found moving the code here much cleaner than adding some impl-side painting
conditional code to ThreadProxy.

[enne (OOO), 2013-01-17 18:57:17]

On 2013/01/17 18:49:49, David Reveman wrote:
> I don't think we want to change pending tree activation. Activating it before
> visible tiles are ready will cause the page to flash.

Hmm, I guess it's still showing the front buffer and hasn't drawn yet, so
activating would cause checkerboarding.  That makes sense.

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11886091/diff/1/cc/layer_tree_host.cc#newcode259
cc/layer_tree_host.cc:259: bool newImplTreeHasNoEvictedResources =
!m_contentsTextureManager->linkedEvictedBackingsExist();
On 2013/01/17 18:49:49, David Reveman wrote:
> On 2013/01/17 17:31:14, enne wrote:
> > Could you explain why this code moved from ThreadProxy to here?  I'm not
sure
> I
> > follow.
> 
> We need to call ResetContentsTexturesPurged() on the syncTree which is not the
> same as the activeTree. Normally it's the same but in the case of impl-side
> painting the syncTree is the pendingTree.
> 
> I found moving the code here much cleaner than adding some impl-side painting
> conditional code to ThreadProxy.

Ah, makes perfect sense.  Thanks for the explanation.

[enne (OOO), 2013-01-17 19:01:19]

lgtm

[ccameron, 2013-01-17 19:10:31]

Very nice, lgtm as well.  Would have made sense to attach this data to the LTI
in the first place.

[nduca, 2013-01-17 19:14:40]

lgtm on basis of these fine individuals liking it :)

[commit-bot: I haz the power, 2013-01-17 19:45:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11886091/1

[enne (OOO), 2013-01-17 20:34:34]

Patchset2 also lgtm.  Good catch.

[commit-bot: I haz the power, 2013-01-17 20:37:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11886091/21001

[commit-bot: I haz the power, 2013-01-17 23:42:02]

Change committed as 177532