cc: Use multiple PrepareTiles approaches

Use multiple PrepareTiles approaches

PRIORITIZE_LATENCY:
  What we have before the patch.
  This approach calls PrepareTiles after every commit and after draws if
  there wasn't a commit for that frame. With this approach, we cannot trust
  NotifyReadyToActivate and NotifyReadyToDraw and they should only be used
  as hints. All other approaches have trustworthy NotifyReadyToActivate's
  and NotifyReadyToDraw's.
  Goes to ACTIVE_WORK on first checkerboard detection.

ACCURATE_ACTIVE_WORK:
  This approach only calls PrepareTiles at the start of a frame so new
  active tree work starts ASAP, but the pending tree work doesn't.
  Goes back to PRIORITIZE_LATENCY on idle.

The following major changes were needed to support this:
* PrepareTiles only initiated by the scheduler now.
* New DrawResult for when raster source for visible tile is missing.
* DrawAndSwapForced return value was ignored.
* DrawAndSwapIfPossible always fails on checkerboard now.
* Scheduler uses DrawAndSwapForced if it really wants to checkerboard.
* Scheduler decides how to handle high res required to draw.
* Update action after actually running it for DrawResult.
* Forced redraw logic removed.
* Deadline repost on aborted draw due to checkerboards.

BUG=486072, 475639
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[brianderson, 2015-05-16 02:39:56]

brianderson@chromium.org changed reviewers:
+ enne@chromium.org, vmpstr@chromium.org

[brianderson, 2015-05-16 02:39:57]

Don't need a detailed review yet, but looking for some high level feedback
regarding the patch description. It's basically the option 4 from my email.

Also - I'm wondering if I'm properly detecting if visible tiles a missing a
raster source.

It seems like I get NotifyReadyToDraws even if there a missing raster source,
which I'll need to fix - either by aborting multiple times or by having
PrepareTiles report immediately when a visible tile is missing it's raster
source. Not sure how difficult the later option would be.

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:278: bool raster_source_covers_tile = *iter;
Enne, Vlad: Is this correct?

[vmpstr, 2015-05-18 17:20:48]

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:278: bool raster_source_covers_tile = *iter;
On 2015/05/16 02:39:57, brianderson wrote:
> Enne, Vlad: Is this correct?

Not quite. We've talked about this offline a little bit, but I think we should
use CoversRect to detect this and we probably won't be able to get the number of
tiles out of this, so maybe just a bool for now.

[enne (OOO), 2015-05-18 21:40:50]

Am I reading this correctly that the goal here is to land this so that different
approaches can be played with, and that it's not clear at this point which one
should be used?

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.cc:278: bool raster_source_covers_tile = *iter;
On 2015/05/18 at 17:20:48, vmpstr wrote:
> On 2015/05/16 02:39:57, brianderson wrote:
> > Enne, Vlad: Is this correct?
> 
> Not quite. We've talked about this offline a little bit, but I think we should
use CoversRect to detect this and we probably won't be able to get the number of
tiles out of this, so maybe just a bool for now.

Yeah, false here means that the coverage iterator couldn't find tiles that were
ready to draw to cover it.  That could just mean that they weren't ready to
draw.  (It could theoretically mean that there weren't any tilings, but this
wouldn't happen in practice.)  It could mean that there weren't tiles at all,
and that usually implies that the raster source can't cover it.

We could get the number of tiles, but it'd kind of be a waste of work.  I like
vmpstr's idea of just one CoversRect and a bool.

https://codereview.chromium.org/1131633003/diff/1/cc/trees/layer_tree_settings.h
File cc/trees/layer_tree_settings.h (left):

https://codereview.chromium.org/1131633003/diff/1/cc/trees/layer_tree_setting...
cc/trees/layer_tree_settings.h:53: bool
timeout_and_draw_when_animation_checkerboards;
I like how this special case animation code goes away and gets folded into other
cases.

[brianderson, 2015-05-19 02:40:35]

Changed how I detect if there's a raster source. Ptal at the parts where I
commented "Is this correct?".

Turns out the Tesla page wasn't missing raster source like I thought - the work
just hadn't been queued yet.


> Am I reading this correctly that the goal here is to land this so that
different approaches can be played with, and that it's not clear at this point
which one should be used?

The last two approaches are definitely not viable right now. I will remove them.

The first two approaches are meant to complement each other and I was planning
to land this patch with both of them enabled - but could put it behind a flag if
it's too risky.

https://codereview.chromium.org/1131633003/diff/20001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:270:
append_quads_data->raster_source_covers_visible_high_res_tiles =
Is this correct?

https://codereview.chromium.org/1131633003/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1131633003/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:803: bool
PictureLayerTiling::RasterSourceCoversCurrentVisibleTiles() const {
Is this correct?

[enne (OOO), 2015-05-19 22:53:42]

On 2015/05/19 at 02:40:35, brianderson wrote:
> Changed how I detect if there's a raster source. Ptal at the parts where I
commented "Is this correct?".
> 
> Turns out the Tesla page wasn't missing raster source like I thought - the
work just hadn't been queued yet.

This sounds like the kind of error I was expecting to see.  Is this a missing
PrepareTiles after a scroll, causing no raster work to start being generated?

> > Am I reading this correctly that the goal here is to land this so that
different approaches can be played with, and that it's not clear at this point
which one should be used?
> 
> The last two approaches are definitely not viable right now. I will remove
them.
> 
> The first two approaches are meant to complement each other and I was planning
to land this patch with both of them enabled - but could put it behind a flag if
it's too risky.

I guess I am not clear on when we switch from one approach to the other?

https://codereview.chromium.org/1131633003/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1131633003/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:803: bool
PictureLayerTiling::RasterSourceCoversCurrentVisibleTiles() const {
On 2015/05/19 at 02:40:35, brianderson wrote:
> Is this correct?

Seems legit!

https://codereview.chromium.org/1131633003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:911: draw_result =
DRAW_ABORTED_MISSING_RASTER_SOURCE;
Is it possible to separate this change into two patches: one about PrepareTiles
in more cases, and the other about checkerboarding?

This is the part I'm most unclear about.  I'm not sure we want to always abort
draws in these case?  Will this ever draw and checkerboard and get forced?

[brianderson, 2015-05-20 01:33:17]

> This sounds like the kind of error I was expecting to see.  Is this a missing
PrepareTiles after a scroll, causing no raster work to start being generated?

Not 100% sure, need to double check.

> I guess I am not clear on when we switch from one approach to the other?

See inline comment.

https://codereview.chromium.org/1131633003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1131633003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:911: draw_result =
DRAW_ABORTED_MISSING_RASTER_SOURCE;
On 2015/05/19 at 22:53:41, enne wrote:
> Is it possible to separate this change into two patches: one about
PrepareTiles in more cases, and the other about checkerboarding?
> 

Definitely need to split this patch up - I'll see what I can do.

> This is the part I'm most unclear about.  I'm not sure we want to always abort
draws in these case?  Will this ever draw and checkerboard and get forced?

My intent is to only allow checkerboards if the draw is forced - so it is very
explicit when our draw might checkerboard. The scheduler will force the draw
after detecting checkerboard by doing the following:


PREPARE_TILES_APPROACH_PRIORITIZE_LATENCY only calls DrawAndSwapIfPossible
PREPARE_TILES_APPROACH_ACCURATE_ACTIVE_WORK only calls DrawAndSwapForced

The first time DrawAndSwapIfPossible aborts due to checker, the scheduler
transitions to PREPARE_TILES_APPROACH_ACCURATE_ACTIVE_WORK until we quiesce.
PREPARE_TILES_APPROACH_ACCURATE_ACTIVE_WORK helps avoid checkerboards by waiting
for NotifyReadyToDraw.

One thing this patch doesn't do right now is reschedule the deadline immediately
after DrawAndSwapIfPossible aborts instead of waiting for the next frame. I need
to fix that.

[mithro-old, 2015-05-27 06:13:47]

I'll try and look closer at this patch, but wanted to make some comments from
your CL description.

I'm a bit sceptical about anything which adds new "modes" into the scheduler,
they often have complex interactions with all the other "modes" that the
scheduler has.

I am however very supportive of a lot of things this CL proposes to do in the
process of trying to add modes. The idea that the scheduler is in control of
PrepareTiles and reworking the DrawAndSwap stuff sounds good to me. 

It definitely feels like choosing to checker board should be a scheduling
decision based on the scheduler's understanding about if the drawing is a
temporary stall or something which will continue for longer period.

[brianderson, 2015-05-27 17:01:07]

On 2015/05/27 06:13:47, mithro wrote:
> I'll try and look closer at this patch, but wanted to make some comments from
> your CL description.
> 
> I'm a bit sceptical about anything which adds new "modes" into the scheduler,
> they often have complex interactions with all the other "modes" that the
> scheduler has.

I would love to get away with not having two modes, but I don't think that will
be feasible until UpdateDrawProperties and PrepareTiles are both super cheap. My
previous attempt at this did not have modes and there were concerns around
regressing both latency and throughput. This two mode approach makes sure we
only regress performance in the <1% of cases where we encounter checkerboard.

> 
> I am however very supportive of a lot of things this CL proposes to do in the
> process of trying to add modes. The idea that the scheduler is in control of
> PrepareTiles and reworking the DrawAndSwap stuff sounds good to me. 
> 
> It definitely feels like choosing to checker board should be a scheduling
> decision based on the scheduler's understanding about if the drawing is a
> temporary stall or something which will continue for longer period.