cc: Indicate that we've updated a tiling if we invalidated it.

cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6ee1182bdabeb7da02485c30b7f9f7142eef25c1
Cr-Commit-Position: refs/heads/master@{#385020}

[vmpstr, 2016-03-30 22:49:33]

Description was changed from

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj
BUG=593449
==========

to

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-03-30 22:50:10]

Please take a look. I'll add a test if it looks OK.

[varkha, 2016-03-31 19:39:03]

varkha@chromium.org changed reviewers:
+ varkha@chromium.org

[varkha, 2016-03-31 19:39:04]

With this change done locally I couldn't repro the flicker anymore on a
chromeos=1 build on Linux - including the black flash on bookmarks bar. If the
underlying problem in the single threaded scheduler proves too long /
controversial to fix would this be a safer patch for M-50?

[enne (OOO), 2016-03-31 19:46:43]

I think it's good to be robust to this if this is the only complication it adds.
 I kind of wish we had a pipeline that didn't make this necessary, but that's a
bit of a larger problem.

[varkha, 2016-04-01 20:00:06]

Does this look promising enough to get in M-50? It would be good to unblock the
release branch.

[vmpstr, 2016-04-04 18:36:15]

vmpstr@chromium.org changed reviewers:
+ enne@chromium.org
- varkha@chromium.org

[vmpstr, 2016-04-04 18:36:16]

Added a unittest here. 

After talking to sunnyps@ on Friday, I went back to this patch, since I still
think it's the correct fix.

What I think is happening is that we get a SetNeedsRedrawOnImpl in STP, which
causes us to start an impl frame. Then, we get input which causes us to
SetNeedsCommit and we send the begin main frame before the impl deadline. So
far, everything is valid in terms of ordering (afaict)

Then, the impl deadline happens, which computes tile priority rects. Note that
this compute tile priority rects is only needed because we can get input mid
frame, something that should be fixed by UBF (afaik). 

Then, since we already sent the begin main frame, begin main frame happens and
we commit and invalidate. This, of course, causes a compute tile priority rects
again. However, since we're using the impl frame time as the timestamp, this is
the second compute tile priority rects with the same timestamp and viewport. We
early out (return false). When we get false from every layers, we do not
schedule a PrepareTiles and say we're ready to draw, which means next draw the
tiles aren't ready and we flicker.

Note that the early out is only needed because PrepareTiles might not be cheap,
so it's a performance optimization that causes a visual glitch. 

This patch ensures that the performance optimization isn't causing incorrect
behavior. As for the ordering of events, I _think_ they are correct or at least
they can happen in this order. 

I can file a follow-up P2 to fix this ordering if that's something that people
want.

[vmpstr, 2016-04-04 18:36:28]

Description was changed from

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-04-04 18:43:51]

Thanks for all the explanation.  I think we should just be robust to this and do
this as our short-term fix.

In the future with unified begin frame and better input ordering, we can make
all of this a lot more sane, but we're not there yet.

https://codereview.chromium.org/1841083004/diff/20001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1841083004/diff/20001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling_unittest.cc:1787: TEST(PictureLayerTilingTest,
InvalidateAfterComputeTilePriorityRects) {
WTB a comment for the scenario that this is testing.

[vmpstr, 2016-04-04 19:45:30]

PTAL

https://codereview.chromium.org/1841083004/diff/20001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1841083004/diff/20001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling_unittest.cc:1787: TEST(PictureLayerTilingTest,
InvalidateAfterComputeTilePriorityRects) {
On 2016/04/04 18:43:51, enne wrote:
> WTB a comment for the scenario that this is testing.

Done.

[enne (OOO), 2016-04-04 19:48:03]

Thanks, lgtm!

[danakj, 2016-04-04 20:40:47]

ok LGTM, thanks for investigating this thoroughly.

[vmpstr, 2016-04-04 20:41:24]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-04-04 20:41:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841083004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841083004/60001

[commit-bot: I haz the power, 2016-04-04 21:52:37]

Description was changed from

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-04 21:52:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-04 21:53:58]

Description was changed from

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Indicate that we've updated a tiling if we invalidated it.

This patch ensures that we inform the caller that the tiling was updated
if it was invalidated since the last call to ComputeTilePriorityRects.

This ensures that we don't bypass TileManager::PrepareTiles call.

R=danakj, enne
BUG=593449
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6ee1182bdabeb7da02485c30b7f9f7142eef25c1
Cr-Commit-Position: refs/heads/master@{#385020}
==========

[commit-bot: I haz the power, 2016-04-04 21:53:59]

Patchset 4 (id:??) landed as
https://crrev.com/6ee1182bdabeb7da02485c30b7f9f7142eef25c1
Cr-Commit-Position: refs/heads/master@{#385020}