cc: Consider visible rect for update tile priority early out.

cc: Consider visible rect for update tile priority early out.

This patch adds a visible rect check for an early out from update
tile priorities.

The problem is that we can update tile priorities, then update
the visible rect in animation, and call update tile priorities
again. The second one would normally early out since it's called
in the same frame. This ensures the early out doesn't happen in
these cases.

BUG=423455
R=danakj, brianderson, enne

Committed: https://crrev.com/cc0e08bd762154a6e31de5a2e3b2aa5b23af0eae
Cr-Commit-Position: refs/heads/master@{#300526}

[vmpstr, 2014-10-20 23:32:47]

This is still a bit of a WIP, but this patch seems to fix the animation freeze.
I
haven't run tests yet, but I will do that a bit later today.

One thing to consider: this will cause (possibly significantly) more work on m39
and earlier, since prior to iterators, update tile priorities is heavy. So I
don't know how possible the merge is. The other (lesser) concern is that our
skewport calculation is dependent on the frame time, so if we end up updating
tile priorities twice per frame, then we'll have a "has a skewport, doesn't have
a skewport, has a skewport" type of pattern, which might do either unnecessary
work or fail to prepaint the skewport.

Basically, I gotta do more testing, but let me know if anyone can think of a
better solution instead.

[brianderson, 2014-10-20 23:37:26]

+Tim so he's aware.

[brianderson, 2014-10-21 00:10:13]

lgtm assuming we need this as a quick fix for M38.

Otherwise, we should spend a bit more time trying to understand the tradeoffs
and the real root cause for why the state of the world is different even though
the timestamps haven't changed.

[vmpstr, 2014-10-21 01:30:39]

On 2014/10/21 00:10:13, brianderson wrote:
> lgtm assuming we need this as a quick fix for M38.
> 
> Otherwise, we should spend a bit more time trying to understand the tradeoffs
> and the real root cause for why the state of the world is different even
though
> the timestamps haven't changed.

enne, danakj, can you take a look? This seems to work in the sense that it fixes
the issue. My reasoning is that if we call UpdateTiles, we don't really have a
good excuse not to call UpdateTilePriorities. FWIW, on ToT, this isn't much of a
performance hit for two reasons: we don't typically UpdateTilePriorities more
than once per frame, and the function itself is not that heavy. Merging to 38 is
a different issue, since although we don't typically update tile priorities more
than once, it is a pretty heavy function there... All in all, I think we should
fix this on ToT first, then worry about the merge

[Sami, 2014-10-21 09:52:09]

Can we add a test that replicates the bug?

[enne (OOO), 2014-10-21 17:12:52]

Given that there's a manual repro to test that the fix works, that this is a bad
bug, and there's a 5pm branch cutoff, this lgtm to land now and follow up with a
regression unit test if you'd like.

[enne (OOO), 2014-10-21 17:37:08]

still lgtm

[vmpstr, 2014-10-21 17:37:53]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-21 17:39:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667053003/40001

[commit-bot: I haz the power, 2014-10-21 18:31:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-21 18:32:32]

Patchset 3 (id:??) landed as
https://crrev.com/cc0e08bd762154a6e31de5a2e3b2aa5b23af0eae
Cr-Commit-Position: refs/heads/master@{#300526}

[mithro-old, 2014-10-27 13:20:36]

mithro@mithis.com changed reviewers:
+ mithro@mithis.com

[mithro-old, 2014-10-27 13:20:37]

https://codereview.chromium.org/667053003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/667053003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:499: base::TimeTicks()).InSecondsF();
Just a FYI, these lines will totally not do what you expect if called at the
wrong time. It's unclear to me when exactly this function is called, so I'm not
sure if that is possible?

When not in the background, the value of CurrentBeginFrameArgs is set in
WillBeginImplFrame and cleared by calling
ResetCurrentBeginFrameArgsForNextFrame.

When in the background this happens in the
LayerTreeHostImplTimeSourceAdapter::OnTimerTick() method.

When outside of the period between Update and Reset calls the
CurrentBeginFrameArgs method just returns Now().

All the above is kind of seriously horrible and really needs to be fixed. To
make matters even more confusing, this value won't match the the last vsync
value as it's overridden to now to preserve monotonicity guarantees caused by
returning Now() at other times! :(

Does the "NeedsUpdateForFrameAtTime" method depend on the frame time being
useful, or does it just need a monotonic increasing value?