SetNeedsRedraw directly when updating a visible tile.

SetNeedsRedraw directly when updating a visible tile.

Removes the UpdateVisibleTiles scheduler state, instead if the
current set of raster tasks starts with a visible tile, we
optimistically call SetNeedsRedraw() at the start of each impl
frame to cause us to try and draw. At draw time we
UpdateVisibleTiles in the tile manager to collect any completed
tasks.

To handle other cases, whenever a visible tile is completed, we
call SetNeedsRedraw() in addition to setting damage on the layer.

Last, when NotifyReadyToDraw() happens, we have a complete frame,
so we can stop optimistically calling SetNeedsRedraw() at the
start of each impl frame.

This allows us to remove the full viewport damage when ending a
pinch gesture.

This change adds 2 integration unit tests for pinch zoom. The
first ensures that when a pinch ends we don't leave blurry tiles
on the screen, but update them to ideal. The second ensures we
continuously try to draw while we have an incomplete frame.

BUG=427423
Committed: https://crrev.com/a819c2552ffe135e2e15fb0eb64341882a7d4912
Cr-Commit-Position: refs/heads/master@{#304147}

[danakj, 2014-10-28 00:18:50]

Please discuss :)  If we did this then I could remove a bunch of code instead of
just early-outting of stuff.

Is there something I should look for this breaking? Is this a good idea?

This is changing the approach taken in
https://chromiumcodereview.appspot.com/11879012

brianderson: I want to remove TileManager::UpdateVisibleTiles() and maybe the
whole scheduler state for checking for visible tile updates, and just SNRedraw()
when a visible tile changes (and maybe check !inside draw when doing so?) But i
see you explicitly went this other way. But then when we're not in that
scheduler state, and something changes (pinch ended) we don't do anything with
visible tiles being updated.

So uh.. wdyt?

[vmpstr, 2014-10-28 01:35:34]

(read comments in reverse order)

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:328: rasterizer_->CheckForCompletedTasks();
This would normally check completed tasks, but not if we scheduled everything
successfully last time (early out above; so like a page with < 32 tiles total)

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:437: rasterizer_->CheckForCompletedTasks();
We still need something to check for completed tasks before drawing, otherwise
we won't actually issue completion callbacks (and not initialize tiles). Maybe?

[enne (OOO), 2014-10-28 18:09:23]

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:437: rasterizer_->CheckForCompletedTasks();
On 2014/10/28 at 01:35:34, vmpstr wrote:
> We still need something to check for completed tasks before drawing, otherwise
we won't actually issue completion callbacks (and not initialize tiles). Maybe?

Yeah.  If we're using async uploads, we need to check for completed tasks or no
tile will be ready even if it is.

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:55: virtual void NotifyTileStateChanged(const Tile*
tile, bool visible_tile) = 0;
This seems redundant to pass along visible_tile.  You can get this information
from the tile itself?

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:55: virtual void NotifyTileStateChanged(const Tile*
tile, bool visible_tile) = 0;
This seems redundant to pass along visible_tile.  You can get this information
from the tile itself?

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1349: if (visible_tile &&
!client_->IsInsideDraw())
I don't think we want to redraw if a tile gets removed, since that could mean
that the new frame is strictly worse.

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3001: SetNeedsRedraw();
Do you need the set needs redraw as well? Is this redundant with the
NotifyTileStateChanged one?

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3001: SetNeedsRedraw();
Do you need the set needs redraw as well? Is this redundant with the
NotifyTileStateChanged one?

[danakj, 2014-10-28 18:24:49]

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1349: if (visible_tile &&
!client_->IsInsideDraw())
On 2014/10/28 18:09:22, enne wrote:
> I don't think we want to redraw if a tile gets removed, since that could mean
> that the new frame is strictly worse.

Ya.. though that's like flaky at best if you scroll it'd disappear anyway, for
example. Maybe I can pass that through with the Tile* though, not sure.

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3001: SetNeedsRedraw();
On 2014/10/28 18:09:21, enne wrote:
> Do you need the set needs redraw as well? Is this redundant with the
> NotifyTileStateChanged one?

We need this in order to trigger an UpdateDrawProperties and make the new raster
scale.

[enne (OOO), 2014-10-28 18:47:34]

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1349: if (visible_tile &&
!client_->IsInsideDraw())
On 2014/10/28 18:24:48, danakj wrote:
> On 2014/10/28 18:09:22, enne wrote:
> > I don't think we want to redraw if a tile gets removed, since that could
mean
> > that the new frame is strictly worse.
> 
> Ya.. though that's like flaky at best if you scroll it'd disappear anyway, for
> example. Maybe I can pass that through with the Tile* though, not sure.

No, it's true that it's flaky at best.  But, it seems like we shouldn't do work
to put up a worse frame unless we have to.  :P

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3001: SetNeedsRedraw();
On 2014/10/28 18:24:48, danakj wrote:
> On 2014/10/28 18:09:21, enne wrote:
> > Do you need the set needs redraw as well? Is this redundant with the
> > NotifyTileStateChanged one?
> 
> We need this in order to trigger an UpdateDrawProperties and make the new
raster
> scale.

As I said elsewhere, the redraw part of that seems racy with respect to tiles
being done.  I guess we'll early out of drawing if nothing has changed,
but...it's kind of weird vs. just doing an UDP.

[brianderson, 2014-10-28 21:07:50]

On 2014/10/28 00:18:50, danakj wrote:
> Please discuss :)  If we did this then I could remove a bunch of code instead
of
> just early-outting of stuff.
> 
> Is there something I should look for this breaking? Is this a good idea?
> 
> This is changing the approach taken in
> https://chromiumcodereview.appspot.com/11879012
> 
> brianderson: I want to remove TileManager::UpdateVisibleTiles() and maybe the
> whole scheduler state for checking for visible tile updates, and just
SNRedraw()
> when a visible tile changes (and maybe check !inside draw when doing so?) But
i
> see you explicitly went this other way. But then when we're not in that
> scheduler state, and something changes (pinch ended) we don't do anything with
> visible tiles being updated.
> 
> So uh.. wdyt?

Others have already commented on the code, but regarding the end goal:

I think there was a reason to have UpdateVisibleTiles initiated from the
scheduler way back when, but I remember reveman started moving away from it and
I support removing it from the scheduler all together since it makes more sense
for TileManager to update visible tile state via whatever polling/notification
mechanism it wants to use.

Just calling SNRedraw also sounds good; it will integrate really well with
@ernstm's ReadyToDraw signal since the scheduler could know not to automatically
trigger the deadline early after just 1 new texture.

To be pedantic, I'm tempted to keep
SchedulerStateMachine::swap_used_incomplete_tile_ around though since it
provides a hint that needs_redraw_ will become true soon and we can proactively
request BeginFrames. This is unlikely to help in practice though since we
already request BeginFrames for 1 extra vsync after becoming inactive (and I
hope we upload more than 1 visible texture per vsync!).

[danakj, 2014-10-29 19:56:56]

OK I did this for real and removed it entirely from the scheduler.

To give an idea of the complexity in this patch, without the new unit test it
would be: +43, -382 lines.

[enne (OOO), 2014-10-29 20:34:45]

I like this a lot in general, but have some questions about the details.  I
think if SchedulerStateMachine needs to ask for BeginFrames early, we can add
this back in.  In the meantime, this is a lot of indirection just to request a
redraw.

Just to restate what the state of the world is after this patch, it looks like
this vague call chain:

FooRasterWorkerPool::OnRasterFinished
  TileManager::DidFinishRunningTasks
    LayerTreeHostImpl::NotifyTileStateChanged
      PictureLayerImpl::NotifyTileStateChanged
        layer gets damaged, used on next draw
      if active/visible: SetNeedsRedraw
        (scheduled eventually) LayerTreeHostImpl::PrepareToDraw
          TileManager::UpdateVisibleTiles
            Rasterizer::CheckForCompletedTasks (for async uploads, I guess)
        drawing etc

So, what kicks off the entire process of redraws is now not the scheduler, but
instead a raster task *set* being finished.

The previous behavior was that the scheduler would previously periodically check
for new visible tiles.  And, if *any* tile was completed in that time, would
schedule a redraw.  And then if the frame was incomplete, it would repeat this
process.

It seems like the new behavior is to only schedule a redraw when a raster task
set has finished.  Maybe my TileManager knowledge is naive, but it seems like we
have two named task sets (required for activation, and all).  Here are some
worries that I have:

(1) If you have 10 checkerboard tiles and #1-9 take zero time to raster and #10
takes seconds to raster, then there wouldn't be a redraw for seconds even though
there is better content to display?
(2) Does ALL contain prepaint tiles too? So if maybe there is a 32 task limit,
but only 1 is an active visible tile, and the other 31 are active non-visible
tiles, would we not get a redraw until ALL was complete?

https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:320: rasterizer_->CheckForCompletedTasks();
Should this just be a call to UpdateVisibleTiles?

[brianderson, 2014-10-29 21:43:21]

Two things:

1) Regarding the task set grouping concerns: I wonder if we can add a
WillBeginImplFrameDeadline hook that the TileManager can use to call
TileManager::UpdateVisibleTiles if it hasn't already called SetNeedsRedraw.

2) Regarding SchedulerStateMachine::swap_used_incomplete_tile_:
I thought of another place where it would be useful. We should be using it in
Scheduler::AdjustedBeginImplFrameDeadline too. We currently rely on
needs_redraw_ as the only hint that the impl thread wants to produce something.
Maybe we should keep swap_used_incomplete_tile_ around for now?

Doing 2 will help make sure we call WillBeginImplFrameDeadline at a better time.

[danakj, 2014-10-30 15:04:52]

https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:320: rasterizer_->CheckForCompletedTasks();
On 2014/10/29 20:34:44, enne wrote:
> Should this just be a call to UpdateVisibleTiles?

Sure, done.

[danakj, 2014-10-30 15:06:28]

On 2014/10/29 20:34:45, enne wrote:
> I like this a lot in general, but have some questions about the details.  I
> think if SchedulerStateMachine needs to ask for BeginFrames early, we can add
> this back in.  In the meantime, this is a lot of indirection just to request a
> redraw.
> 
> Just to restate what the state of the world is after this patch, it looks like
> this vague call chain:
> 
> FooRasterWorkerPool::OnRasterFinished
>   TileManager::DidFinishRunningTasks
>     LayerTreeHostImpl::NotifyTileStateChanged
>       PictureLayerImpl::NotifyTileStateChanged
>         layer gets damaged, used on next draw
>       if active/visible: SetNeedsRedraw
>         (scheduled eventually) LayerTreeHostImpl::PrepareToDraw
>           TileManager::UpdateVisibleTiles
>             Rasterizer::CheckForCompletedTasks (for async uploads, I guess)
>         drawing etc

That's my understanding as well yes.

> So, what kicks off the entire process of redraws is now not the scheduler, but
> instead a raster task *set* being finished.
> 
> The previous behavior was that the scheduler would previously periodically
check
> for new visible tiles.  And, if *any* tile was completed in that time, would
> schedule a redraw.  And then if the frame was incomplete, it would repeat this
> process.
> 
> It seems like the new behavior is to only schedule a redraw when a raster task
> set has finished.  Maybe my TileManager knowledge is naive, but it seems like
we
> have two named task sets (required for activation, and all).  Here are some
> worries that I have:

Maybe I should have FooRasterWorkerPool call UpdateVisibleTiles on TileManager
for every finished task?

> (1) If you have 10 checkerboard tiles and #1-9 take zero time to raster and
#10
> takes seconds to raster, then there wouldn't be a redraw for seconds even
though
> there is better content to display?
> (2) Does ALL contain prepaint tiles too? So if maybe there is a 32 task limit,
> but only 1 is an active visible tile, and the other 31 are active non-visible
> tiles, would we not get a redraw until ALL was complete?

[danakj, 2014-10-30 15:13:36]

On 2014/10/29 21:43:21, brianderson wrote:
> Two things:
> 
> 1) Regarding the task set grouping concerns: I wonder if we can add a
> WillBeginImplFrameDeadline hook that the TileManager can use to call
> TileManager::UpdateVisibleTiles if it hasn't already called SetNeedsRedraw.

I didn't understand this suggestion. Right now TileManager calls LTHI with
NotifyTileStateChanged for each tile that's completed. And then LTHI calls
SetNeedsRedraw as needed.

When would this WillBeginImplFrameDeadline occur? Instead of SetNeedsRedraw? In
what conditions? Or is it supposed to be earlier somewhere? Like if it's
currently rastering a visible tile?

> 2) Regarding SchedulerStateMachine::swap_used_incomplete_tile_:
> I thought of another place where it would be useful. We should be using it in
> Scheduler::AdjustedBeginImplFrameDeadline too. We currently rely on
> needs_redraw_ as the only hint that the impl thread wants to produce
something.
> Maybe we should keep swap_used_incomplete_tile_ around for now?
> 
> Doing 2 will help make sure we call WillBeginImplFrameDeadline at a better
time.

I don't really know how AdjustedBeginImplFrameDeadline, so can I ask a bit more?
What does it do if input comes in after it's run but before the deadline? Then
the needs_redraw state would change there as well. It seems similar to how a
tile could be ready later. Maybe I can make this behave more like how input
would interact?

[danakj, 2014-10-30 15:14:04]

danakj@chromium.org changed reviewers:
+ reveman@chromium.org

[danakj, 2014-10-30 15:14:04]

+reveman

[danakj, 2014-10-30 15:37:33]

On 2014/10/29 21:43:21, brianderson wrote:
> Two things:
> 
> 1) Regarding the task set grouping concerns: I wonder if we can add a
> WillBeginImplFrameDeadline hook that the TileManager can use to call
> TileManager::UpdateVisibleTiles if it hasn't already called SetNeedsRedraw.

OK I stared at this some more I think I understand, I had it backward.

You're suggesting that LTHI::WillBeginImplFrame do an UpdateVisibleTiles. At the
front of the impl frame, before we decide the deadline.

IIUC, that would only collect completed tiles, which we'll already do in
PrepareToDraw. I don't *think* that would take us to DidFinishRunningTasks. But
maybe we can do something like check if TileManager has a visible tile
scheduled, and SetNeedsRedraw if so?

[danakj, 2014-10-30 15:59:03]

In the latest patch set I have added has_active_visible_tile_scheduled() to the
TileManager, and when it returns true in WillBeginImplFrame, we SetNeedsRedraw.

This causes us to try to draw a frame and do TileManager::UpdateVisibleTiles.

Is this enough? Or would this still not catch 1 tile if there was 32 scheduled
and they weren't all done yet?

[reveman, 2014-10-30 16:42:06]

On 2014/10/29 20:34:45, enne wrote:
> I like this a lot in general, but have some questions about the details.  I
> think if SchedulerStateMachine needs to ask for BeginFrames early, we can add
> this back in.  In the meantime, this is a lot of indirection just to request a
> redraw.
> 
> Just to restate what the state of the world is after this patch, it looks like
> this vague call chain:
> 
> FooRasterWorkerPool::OnRasterFinished
>   TileManager::DidFinishRunningTasks
>     LayerTreeHostImpl::NotifyTileStateChanged
>       PictureLayerImpl::NotifyTileStateChanged
>         layer gets damaged, used on next draw
>       if active/visible: SetNeedsRedraw
>         (scheduled eventually) LayerTreeHostImpl::PrepareToDraw
>           TileManager::UpdateVisibleTiles
>             Rasterizer::CheckForCompletedTasks (for async uploads, I guess)
>         drawing etc
> 
> So, what kicks off the entire process of redraws is now not the scheduler, but
> instead a raster task *set* being finished.

This is accurate but not the full story. We need perform a check for completed
tiles in ::DidFinishRunningTasks now that we have a limit to how many tasks can
be scheduled (32 tasks). Without that limit we could always leave it up to the
scheduler to check but with an internal limit in the task manager we need to be
able to make progress by performing an internal check for completed tasks and
schedule more work. The scheduler can also trigger a check for completed tiles
at any time by calling TileManager::UpdateVisibleTiles().

> 
> The previous behavior was that the scheduler would previously periodically
check
> for new visible tiles.  And, if *any* tile was completed in that time, would
> schedule a redraw.  And then if the frame was incomplete, it would repeat this
> process.

The scheduler should still perform a check when it makes sense to do so. It has
more context than the tile manager. The tile manager will only perform checks
for completed tasks when necessary to make progress and be able to generate the
NotifyReadyToActivate() call as quickly as possible.


> 
> It seems like the new behavior is to only schedule a redraw when a raster task
> set has finished.  Maybe my TileManager knowledge is naive, but it seems like
we
> have two named task sets (required for activation, and all).  Here are some
> worries that I have:
> 
> (1) If you have 10 checkerboard tiles and #1-9 take zero time to raster and
#10
> takes seconds to raster, then there wouldn't be a redraw for seconds even
though
> there is better content to display?

Correct. The scheduler will have to perform periodic checks to make this
situation better.

> (2) Does ALL contain prepaint tiles too? So if maybe there is a 32 task limit,
> but only 1 is an active visible tile, and the other 31 are active non-visible
> tiles, would we not get a redraw until ALL was complete?

As of now, yes. Unless the scheduler decides to call UpdateVisibleTiles. The
"ready-to-draw" signal would solve this situation:
https://codereview.chromium.org/672283003/

> 
>
https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/671653005/diff/40001/cc/resources/tile_manage...
> cc/resources/tile_manager.cc:320: rasterizer_->CheckForCompletedTasks();
> Should this just be a call to UpdateVisibleTiles?

[reveman, 2014-10-30 16:47:35]

On 2014/10/30 15:06:28, danakj wrote:
> On 2014/10/29 20:34:45, enne wrote:
> > I like this a lot in general, but have some questions about the details.  I
> > think if SchedulerStateMachine needs to ask for BeginFrames early, we can
add
> > this back in.  In the meantime, this is a lot of indirection just to request
a
> > redraw.
> > 
> > Just to restate what the state of the world is after this patch, it looks
like
> > this vague call chain:
> > 
> > FooRasterWorkerPool::OnRasterFinished
> >   TileManager::DidFinishRunningTasks
> >     LayerTreeHostImpl::NotifyTileStateChanged
> >       PictureLayerImpl::NotifyTileStateChanged
> >         layer gets damaged, used on next draw
> >       if active/visible: SetNeedsRedraw
> >         (scheduled eventually) LayerTreeHostImpl::PrepareToDraw
> >           TileManager::UpdateVisibleTiles
> >             Rasterizer::CheckForCompletedTasks (for async uploads, I guess)
> >         drawing etc
> 
> That's my understanding as well yes.
> 
> > So, what kicks off the entire process of redraws is now not the scheduler,
but
> > instead a raster task *set* being finished.
> > 
> > The previous behavior was that the scheduler would previously periodically
> check
> > for new visible tiles.  And, if *any* tile was completed in that time, would
> > schedule a redraw.  And then if the frame was incomplete, it would repeat
this
> > process.
> > 
> > It seems like the new behavior is to only schedule a redraw when a raster
task
> > set has finished.  Maybe my TileManager knowledge is naive, but it seems
like
> we
> > have two named task sets (required for activation, and all).  Here are some
> > worries that I have:
> 
> Maybe I should have FooRasterWorkerPool call UpdateVisibleTiles on TileManager
> for every finished task?

FooRasterWorkerPool can't really call UpdateVisibleTiles. UpdateVisibleTiles is
part of the tile manager's client API.

FooRasterWorkerPool could generate a signal for every task that completes but
that implies waking up the compositor thread after running each raster task and
that would have a huge performance impact and is not really an option.

> 
> > (1) If you have 10 checkerboard tiles and #1-9 take zero time to raster and
> #10
> > takes seconds to raster, then there wouldn't be a redraw for seconds even
> though
> > there is better content to display?
> > (2) Does ALL contain prepaint tiles too? So if maybe there is a 32 task
limit,
> > but only 1 is an active visible tile, and the other 31 are active
non-visible
> > tiles, would we not get a redraw until ALL was complete?

[reveman, 2014-10-30 16:58:17]

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:321: UpdateVisibleTiles();
UpdateVisibleTiles() is part of the public TileManager API. We shouldn't be
calling it internally. Something wrong with the design if that's necessary.

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
Not a fan of this. What does it mean to be scheduled? What if a raster task has
not been scheduled for a tile because of the limit of scheduled tasks but it's
still "scheduled" to be initialized by the tile manager? The term scheduled from
a TileManager client's perspective is different than internally to the tile
manager.

If there's a visible tile that is not initialized I think it can be assumed that
it's scheduled to be initialized from the tile manager client's pov.

[danakj, 2014-10-30 16:59:54]

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:321: UpdateVisibleTiles();
On 2014/10/30 16:58:17, reveman wrote:
> UpdateVisibleTiles() is part of the public TileManager API. We shouldn't be
> calling it internally. Something wrong with the design if that's necessary.

OK. It seems like we're just duplicating that function here.. but I can put that
back.

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
On 2014/10/30 16:58:17, reveman wrote:
> Not a fan of this. What does it mean to be scheduled? What if a raster task
has
> not been scheduled for a tile because of the limit of scheduled tasks but it's
> still "scheduled" to be initialized by the tile manager? The term scheduled
from
> a TileManager client's perspective is different than internally to the tile
> manager.
> 
> If there's a visible tile that is not initialized I think it can be assumed
that
> it's scheduled to be initialized from the tile manager client's pov.

How would you recommend setting this bool then? What should I be looking for?

[danakj, 2014-10-30 17:00:58]

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
On 2014/10/30 16:59:53, danakj wrote:
> On 2014/10/30 16:58:17, reveman wrote:
> > Not a fan of this. What does it mean to be scheduled? What if a raster task
> has
> > not been scheduled for a tile because of the limit of scheduled tasks but
it's
> > still "scheduled" to be initialized by the tile manager? The term scheduled
> from
> > a TileManager client's perspective is different than internally to the tile
> > manager.
> > 
> > If there's a visible tile that is not initialized I think it can be assumed
> that
> > it's scheduled to be initialized from the tile manager client's pov.
> 
> How would you recommend setting this bool then? What should I be looking for?

What I want is a bool that says "it's likely that a visible tile will complete
within the next vsync".

[danakj, 2014-10-30 17:20:41]

I added a LTH unittest that shows when we have a visible active tile scheduled
we continue to schedule draws (which will call UpdateVisibleTiles), and that
once we draw a complete frame we stop scheduling draws (and fixed a bug it
found)

[danakj, 2014-10-30 17:24:22]

Patchset #7 (id:120001) has been deleted

[danakj, 2014-10-30 17:32:06]

On 2014/10/30 16:58:17, reveman wrote:
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
> cc/resources/tile_manager.cc:321: UpdateVisibleTiles();
> UpdateVisibleTiles() is part of the public TileManager API. We shouldn't be
> calling it internally. Something wrong with the design if that's necessary.
> 
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manager.h
> File cc/resources/tile_manager.h (right):
> 
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
> cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
> Not a fan of this. What does it mean to be scheduled? What if a raster task
has
> not been scheduled for a tile because of the limit of scheduled tasks but it's
> still "scheduled" to be initialized by the tile manager? The term scheduled
from
> a TileManager client's perspective is different than internally to the tile
> manager.
> 
> If there's a visible tile that is not initialized I think it can be assumed
that
> it's scheduled to be initialized from the tile manager client's pov.

Isn't that the same as the tile being in the tiles_that_need_to_be_rasterized
list? This is currently checking if any tile in tiles_that_need_to_be_rasterized
is active+visible, and setting the bool then. Is that not what you just said?

[reveman, 2014-10-30 17:36:08]

On 2014/10/30 17:00:58, danakj wrote:
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manager.h
> File cc/resources/tile_manager.h (right):
> 
>
https://codereview.chromium.org/671653005/diff/80001/cc/resources/tile_manage...
> cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
> On 2014/10/30 16:59:53, danakj wrote:
> > On 2014/10/30 16:58:17, reveman wrote:
> > > Not a fan of this. What does it mean to be scheduled? What if a raster
task
> > has
> > > not been scheduled for a tile because of the limit of scheduled tasks but
> it's
> > > still "scheduled" to be initialized by the tile manager? The term
scheduled
> > from
> > > a TileManager client's perspective is different than internally to the
tile
> > > manager.
> > > 
> > > If there's a visible tile that is not initialized I think it can be
assumed
> > that
> > > it's scheduled to be initialized from the tile manager client's pov.
> > 
> > How would you recommend setting this bool then? What should I be looking
for?
> 
> What I want is a bool that says "it's likely that a visible tile will complete
> within the next vsync".

If a visible tile that has not been initialized exists, it's likely to complete
within the next vsync. I don't think it's a good idea to try get something more
precise out of the tile manager. A "might be initialized soon" concept in the
tile manager is going to be confusing and make the code harder to understand.

[danakj, 2014-10-30 17:58:26]

On Thu, Oct 30, 2014 at 1:36 PM, <reveman@chromium.org> wrote:

> If a visible tile that has not been initialized exists, it's likely to
> complete
> within the next vsync. I don't think it's a good idea to try get something
> more
> precise out of the tile manager. A "might be initialized soon" concept in
> the
> tile manager is going to be confusing and make the code harder to
> understand.
>

ScheduleTasks has the next 32 important tiles. If we're trying to raster
stuff on the active tree, then visible tiles will be at the front of those
32 tiles. If we're trying to raster stuff on the pending tree, maybe it
doesn't matter so much if we know about an active visible tile that might
be rastered after N raster tasks complete.

I suggest that looking at the top 32 (or maybe even just the front raster
task) could be sufficient for our needs..

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-10-30 19:37:42]

https://codereview.chromium.org/671653005/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
I think this is too implementation specific. For example, it will return very
different results depending on the value of scheduled_raster_task_limit_. I'm
also worried that exposing implementation specific details such as this will
create friction when trying to make changes to the tile manager.

Something like IsTopPriorityUninitializedTileVisibleAndOnActiveTree() would be
better as it's well defined. Not sure you need to reach into the tile manager to
answer that though.

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

https://codereview.chromium.org/671653005/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: SetNeedsRedraw();
So this is just speculative. Would be nice if we had a better way to inform the
scheduler that we might need to redraw than calling SetNeedsRedraw and
pretending that we really need to redraw.

[danakj, 2014-10-30 19:49:07]

https://codereview.chromium.org/671653005/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:109: bool has_active_visible_tile_scheduled() {
On 2014/10/30 19:37:42, reveman wrote:
> I think this is too implementation specific. For example, it will return very
> different results depending on the value of scheduled_raster_task_limit_. I'm

And I think that all of those results would be acceptable for our uses.

> also worried that exposing implementation specific details such as this will
> create friction when trying to make changes to the tile manager.

We just need a signal that there's some work being that would be visible. I
didn't write any tests that tie this into being very specific. The test I added
is causing some tiles at a new scale to be created and trying to draw until they
are done. That's the use case we care about, and as long as we satisfy that, we
can change the signal that the tile manager gives out as much as we like.

> Something like IsTopPriorityUninitializedTileVisibleAndOnActiveTree() would be
> better as it's well defined. Not sure you need to reach into the tile manager
to
> answer that though.

I think it has to come from the tile manager, cuz it needs to know what tiles
the raster iterators are going to spit out, which is generated from the tile
manager.

I agree IsTopPriorityUninitializedTileVisibleAndOnActiveTree would probably be
enough. It should be for the test case I've written, so I'll try that out.
Thanks!

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

https://codereview.chromium.org/671653005/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: SetNeedsRedraw();
On 2014/10/30 19:37:42, reveman wrote:
> So this is just speculative. Would be nice if we had a better way to inform
the
> scheduler that we might need to redraw than calling SetNeedsRedraw and
> pretending that we really need to redraw.

I could add something, but it wouldn't be that different in practice. When we
need to draw we go to PrepareToDraw, do UpdateVisibleTiles, see if there is any
damage, and early out. I think that's better than adding a CheckVisibleTiles
step, then check some state, then start a draw and have some state to prevent
check visible tiles again (which is more like what we had before). It grew to be
complicated and we can just try draw and early out it's much simpler.

[danakj, 2014-10-30 19:56:23]

PTAL it's now just using the top priority tile. The test cases still pass.

[brianderson, 2014-10-30 21:19:28]

Calling SetNeedsRedraw in WillBeginImplFrame is a nice way to get rid of the
need for swap_used_incomplete_tile in the scheduler. 

Does SetNeedsRedraw gets called immediately when
is_top_priority_uninitialized_tile_visible_and_active becomes false? Otherwise,
will there be any races when there are visible tiles that finished after the
last draw but before the WillBeginImplFrame?

Regarding my suggestion for adding WillBeginImplFrameDeadline. I was thinking
that could be a way to poll if any visible tiles were completed by the deadline
because enne's comments made it sound like the grouping of tasks might prevent
the scheduler from being notified of new visible tiles immediately. If no
visible tiles finished, then we could just skip the draw. It seems like we are
going to assume that we will finish at least 1 visible tile per vsync though (a
sane assumption), in which case we don't need all that complication and just
calling SetNeedsRedraw inside of WillBeginImplFrame is a simpler solution.

[danakj, 2014-10-30 21:22:13]

On Thu, Oct 30, 2014 at 5:19 PM, <brianderson@chromium.org> wrote:

> Calling SetNeedsRedraw in WillBeginImplFrame is a nice way to get rid of
> the
> need for swap_used_incomplete_tile in the scheduler.
>
> Does SetNeedsRedraw gets called immediately when
> is_top_priority_uninitialized_tile_visible_and_active becomes false?
> Otherwise,
> will there be any races when there are visible tiles that finished after
> the
> last draw but before the WillBeginImplFrame?
>

It's called at two times:

1) Any time a visible tile is completed
2) Inside WillBeginImplFrame if the highest priority tile to be rastered is
a visible one.

Tiles that finish inbetween will be case 1 and SetNeedsRedraw() from
NotifyTileStateChanged.


>
> Regarding my suggestion for adding WillBeginImplFrameDeadline. I was
> thinking
> that could be a way to poll if any visible tiles were completed by the
> deadline
> because enne's comments made it sound like the grouping of tasks might
> prevent
> the scheduler from being notified of new visible tiles immediately. If no
> visible tiles finished, then we could just skip the draw. It seems like we
> are
> going to assume that we will finish at least 1 visible tile per vsync
> though (a
> sane assumption), in which case we don't need all that complication and
> just
> calling SetNeedsRedraw inside of WillBeginImplFrame is a simpler solution.
>
> https://codereview.chromium.org/671653005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-10-31 00:44:01]

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

https://codereview.chromium.org/671653005/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: SetNeedsRedraw();
On 2014/10/30 19:49:07, danakj wrote:
> On 2014/10/30 19:37:42, reveman wrote:
> > So this is just speculative. Would be nice if we had a better way to inform
> the
> > scheduler that we might need to redraw than calling SetNeedsRedraw and
> > pretending that we really need to redraw.
> 
> I could add something, but it wouldn't be that different in practice. When we
> need to draw we go to PrepareToDraw, do UpdateVisibleTiles, see if there is
any
> damage, and early out. I think that's better than adding a CheckVisibleTiles
> step, then check some state, then start a draw and have some state to prevent
> check visible tiles again (which is more like what we had before). It grew to
be
> complicated and we can just try draw and early out it's much simpler.

Ok, SetNeedsRedraw really means something like SetNeedsRedrawIfDamageExists. You
can keep this as SetNeedsRedraw for now but we might want to follow up with some
naming changes if the redraw is conditional on damage to make the intent here a
bit more clear.

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:330: rasterizer_->CheckForCompletedTasks();
This call has a cost. It's done here so it can be avoided when we early out
above. I don't think we should move it as that will likely have a performance
impact.

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:323:
is_top_priority_uninitialized_tile_visible_and_active_ = false;
'false' might be incorrect. I guess we're assuming that ScheduleTasks will set
this to the correct value again below. That's a bit subtle.

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:678: tile->is_active_and_visible_priority();
is_top_priority_uninitialized_tile_visible_and_active_ might be set to what we
want when is_top_priority_uninitialized_tile_visible_and_active() is currently
called but it looks like it would be incorrect after a call to
UpdateVisibleTiles().

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
cc/resources/tile_manager.h:111: }
Still doesn't feel right to be reaching into the tile manager for this. Seems
backwards that LTHI should be asking the tile manager for this when LTHI is the
one providing the tile manager with a prioritized raster queue. It's like "hey
tile manager, I'm your client, here's the raster queue you're asking for", then
some time later, "oh, tile manager? you know that raster queue I gave you
earlier, did the top priority tile happen to be visible and on active tree by
any chance?"

Maybe some refactor can make this cleaner in the future but can you add a
LTHI::front_tile_in_last_built_raster_queue_was_visible_and_on_active_tree_
member that you set in BuildRasterQueue and use it in LTHI instead of this tile
manager function?

[danakj, 2014-10-31 15:38:38]

Patchset #16 (id:310001) has been deleted

[danakj, 2014-10-31 15:39:46]

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

https://codereview.chromium.org/671653005/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: SetNeedsRedraw();
On 2014/10/31 00:44:01, reveman wrote:
> On 2014/10/30 19:49:07, danakj wrote:
> > On 2014/10/30 19:37:42, reveman wrote:
> > > So this is just speculative. Would be nice if we had a better way to
inform
> > the
> > > scheduler that we might need to redraw than calling SetNeedsRedraw and
> > > pretending that we really need to redraw.
> > 
> > I could add something, but it wouldn't be that different in practice. When
we
> > need to draw we go to PrepareToDraw, do UpdateVisibleTiles, see if there is
> any
> > damage, and early out. I think that's better than adding a CheckVisibleTiles
> > step, then check some state, then start a draw and have some state to
prevent
> > check visible tiles again (which is more like what we had before). It grew
to
> be
> > complicated and we can just try draw and early out it's much simpler.
> 
> Ok, SetNeedsRedraw really means something like SetNeedsRedrawIfDamageExists.
You
> can keep this as SetNeedsRedraw for now but we might want to follow up with
some
> naming changes if the redraw is conditional on damage to make the intent here
a
> bit more clear.

We could, I lean slightly toward leaving it how it is. It's an old name and we
always draw "what is damaged". And it does try to redraw in the future
regardless of damage existing now or not, IfDamageExists kinda sounds like the
Set part early outs rather than the Draw part.

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/290001/cc/resources/tile_manag...
cc/resources/tile_manager.h:111: }
On 2014/10/31 00:44:01, reveman wrote:
> Still doesn't feel right to be reaching into the tile manager for this. Seems
> backwards that LTHI should be asking the tile manager for this when LTHI is
the
> one providing the tile manager with a prioritized raster queue. It's like "hey
> tile manager, I'm your client, here's the raster queue you're asking for",
then
> some time later, "oh, tile manager? you know that raster queue I gave you
> earlier, did the top priority tile happen to be visible and on active tree by
> any chance?"
> 
> Maybe some refactor can make this cleaner in the future but can you add a
> LTHI::front_tile_in_last_built_raster_queue_was_visible_and_on_active_tree_
> member that you set in BuildRasterQueue and use it in LTHI instead of this
tile
> manager function?

Ok thanks for bearing with me, I'm pretty clueless about TileManager. I managed
to move this to LTHI by adding a client callback when DidFinishRunningTasks
early-outs because it is done everything.

[danakj, 2014-10-31 15:39:55]

PTAL

[vmpstr, 2014-10-31 15:46:30]

https://codereview.chromium.org/671653005/diff/330001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/330001/cc/resources/tile_manag...
cc/resources/tile_manager.h:63: // Called when all tiles from the previous
BuildRasterQueue have been
drive by nit: This isn't entirely true... It would be called when all tiles that
need to be processed under the currently memory policy have been processed. The
raster queue might still have tiles that are EVENTUALLY or something when we
have "prepaint only" policy. (to be clear, I think the code is correct it's just
the comment omits the fact that *handwave*memory policy*handwave*)

[danakj, 2014-10-31 15:47:30]

https://codereview.chromium.org/671653005/diff/330001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/330001/cc/resources/tile_manag...
cc/resources/tile_manager.h:63: // Called when all tiles from the previous
BuildRasterQueue have been
On 2014/10/31 15:46:30, vmpstr wrote:
> drive by nit: This isn't entirely true... It would be called when all tiles
that
> need to be processed under the currently memory policy have been processed.
The
> raster queue might still have tiles that are EVENTUALLY or something when we
> have "prepaint only" policy. (to be clear, I think the code is correct it's
just
> the comment omits the fact that *handwave*memory policy*handwave*)

OK thanks I wasn't really sure what to write here, I tried :)

[danakj, 2014-10-31 15:49:04]

Changed it to

  // Called when all tiles from the previous BuildRasterQueue that will be      
                                                                         
  // processed under the current memory policy have been processed. Each        
                                                                         
  // BuildRasterQueue call is followed by either another BuildRasterQueue call, 
                                                                         
  // when some tiles were processed and more will be in the future, or a        
                                                                         
  // FinishedRasterQueue() call.                                                
                                                                         
  virtual void FinishedRasterQueue() = 0;

[reveman, 2014-10-31 16:08:50]

+ernstm

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

https://codereview.chromium.org/671653005/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: SetNeedsRedraw();
On 2014/10/31 15:39:46, danakj wrote:
> On 2014/10/31 00:44:01, reveman wrote:
> > On 2014/10/30 19:49:07, danakj wrote:
> > > On 2014/10/30 19:37:42, reveman wrote:
> > > > So this is just speculative. Would be nice if we had a better way to
> inform
> > > the
> > > > scheduler that we might need to redraw than calling SetNeedsRedraw and
> > > > pretending that we really need to redraw.
> > > 
> > > I could add something, but it wouldn't be that different in practice. When
> we
> > > need to draw we go to PrepareToDraw, do UpdateVisibleTiles, see if there
is
> > any
> > > damage, and early out. I think that's better than adding a
CheckVisibleTiles
> > > step, then check some state, then start a draw and have some state to
> prevent
> > > check visible tiles again (which is more like what we had before). It grew
> to
> > be
> > > complicated and we can just try draw and early out it's much simpler.
> > 
> > Ok, SetNeedsRedraw really means something like SetNeedsRedrawIfDamageExists.
> You
> > can keep this as SetNeedsRedraw for now but we might want to follow up with
> some
> > naming changes if the redraw is conditional on damage to make the intent
here
> a
> > bit more clear.
> 
> We could, I lean slightly toward leaving it how it is. It's an old name and we
> always draw "what is damaged". And it does try to redraw in the future
> regardless of damage existing now or not, IfDamageExists kinda sounds like the
> Set part early outs rather than the Draw part.

Yes, SetMayNeedRedraw? :) Let's not worry about this now though. SetNeedsRedraw
is fine for now. It just looked wrong when reading the code but I understand now
that it's not as bad as it looks.

https://codereview.chromium.org/671653005/diff/350001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/350001/cc/resources/tile_manag...
cc/resources/tile_manager.h:67: virtual void FinishedRasterQueue() = 0;
If we're adding another notification then we should make it consistent with
NotifyReadyToActivate(), hence something like NotifyAllTilesReadyToDraw(). I
don't think we need this though. Manfred's ReadyToDraw notification is really
what we want here: https://codereview.chromium.org/672283003/

Can we either rebase this on that or just reset
LTHI::active_visible_tile_is_top_of_raster_queue_ when being notified that some
visible tile as been initialized and improve it after landing Manfred's patch?

[danakj, 2014-10-31 16:34:39]

https://codereview.chromium.org/671653005/diff/350001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/350001/cc/resources/tile_manag...
cc/resources/tile_manager.h:67: virtual void FinishedRasterQueue() = 0;
On 2014/10/31 16:08:50, reveman wrote:
> If we're adding another notification then we should make it consistent with
> NotifyReadyToActivate(), hence something like NotifyAllTilesReadyToDraw(). I
> don't think we need this though. Manfred's ReadyToDraw notification is really
> what we want here: https://codereview.chromium.org/672283003/
> 
> Can we either rebase this on that or just reset
> LTHI::active_visible_tile_is_top_of_raster_queue_ when being notified that
some
> visible tile as been initialized and improve it after landing Manfred's patch?

Without this callback my pinch zoom test fails to make forward progress because
there's nothing to call UpdateVisibleTiles, so theres nothing to call
NotifyTileStateChange in the case where DidFinishRunningTasks early outs.

[danakj, 2014-10-31 16:43:42]

On Fri, Oct 31, 2014 at 12:34 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/671653005/diff/350001/cc/
> resources/tile_manager.h
> File cc/resources/tile_manager.h (right):
>
> https://codereview.chromium.org/671653005/diff/350001/cc/
> resources/tile_manager.h#newcode67
> cc/resources/tile_manager.h:67: virtual void FinishedRasterQueue() = 0;
> On 2014/10/31 16:08:50, reveman wrote:
>
>> If we're adding another notification then we should make it consistent
>>
> with
>
>> NotifyReadyToActivate(), hence something like
>>
> NotifyAllTilesReadyToDraw(). I
>
>> don't think we need this though. Manfred's ReadyToDraw notification is
>>
> really
>
>> what we want here: https://codereview.chromium.org/672283003/
>
>
I applied Manfred's patch and moved the code from LTHI::FinishedRasterQueue
to LTHI::NotifyReadyToDraw(), and my tests continue to pass. So it seems
like that's sufficient, though I'm not really sure how this patch works atm.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-31 19:47:38]

Here's my patch rebased on manfred's required-to-draw patch.

[danakj, 2014-10-31 20:02:01]

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:328: rasterizer_->CheckForCompletedTasks();
On 2014/10/28 01:35:34, vmpstr wrote:
> This would normally check completed tasks, but not if we scheduled everything
> successfully last time (early out above; so like a page with < 32 tiles total)

Right, I ended up not moving this anymore. Now instead I used manfred's
NotifyReadyToDraw() call to LTHI to tell it when all tiles are done, and his
CheckIfReadyToDraw() does this CheckForCompletedTasks().

Further, once we go to draw as a result of the NotifyReadyToDraw(), we'll
UpdateVisibleTiles there, so this should work out?

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:437: rasterizer_->CheckForCompletedTasks();
On 2014/10/28 18:09:21, enne wrote:
> On 2014/10/28 at 01:35:34, vmpstr wrote:
> > We still need something to check for completed tasks before drawing,
otherwise
> we won't actually issue completion callbacks (and not initialize tiles).
Maybe?
> 
> Yeah.  If we're using async uploads, we need to check for completed tasks or
no
> tile will be ready even if it is.

Right, no longer changing this method.

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/671653005/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:55: virtual void NotifyTileStateChanged(const Tile*
tile, bool visible_tile) = 0;
On 2014/10/28 18:09:21, enne wrote:
> This seems redundant to pass along visible_tile.  You can get this information
> from the tile itself?

Yes, using manfred's tile->required_to_draw() now instead.

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1349: if (visible_tile &&
!client_->IsInsideDraw())
On 2014/10/28 18:47:33, enne wrote:
> On 2014/10/28 18:24:48, danakj wrote:
> > On 2014/10/28 18:09:22, enne wrote:
> > > I don't think we want to redraw if a tile gets removed, since that could
> mean
> > > that the new frame is strictly worse.
> > 
> > Ya.. though that's like flaky at best if you scroll it'd disappear anyway,
for
> > example. Maybe I can pass that through with the Tile* though, not sure.
> 
> No, it's true that it's flaky at best.  But, it seems like we shouldn't do
work
> to put up a worse frame unless we have to.  :P

Actually, I don't think I agree. If we're evicting a tile in the renderer, we
need to draw and swap to tell the browser to stop using the tile in order to
actually free that memory. We could special case this in the browser but that
does not seem worth the complexity at all given the browser will rarely if ever
checkerboard anything.

https://codereview.chromium.org/671653005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3001: SetNeedsRedraw();
On 2014/10/28 18:47:33, enne wrote:
> On 2014/10/28 18:24:48, danakj wrote:
> > On 2014/10/28 18:09:21, enne wrote:
> > > Do you need the set needs redraw as well? Is this redundant with the
> > > NotifyTileStateChanged one?
> > 
> > We need this in order to trigger an UpdateDrawProperties and make the new
> raster
> > scale.
> 
> As I said elsewhere, the redraw part of that seems racy with respect to tiles
> being done.  I guess we'll early out of drawing if nothing has changed,
> but...it's kind of weird vs. just doing an UDP.

I think we agreed this is not racey. The Draw will do UpdateVisibleTiles() which
will damage any tiles that have completed so far, and then draw them. Or early
out if there's nothing damaged on screen.

[reveman, 2014-10-31 20:28:39]

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

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1295: }
How about using conditional assignment here instead of if-else?

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5491:
TestContext()->set_query_result_available_ext(1);
I don't think this is a reliable way to block tiles from completing. What if
ZeroCopy/OneCopy/Bitmap/GpuRasterWorkerPool is used? OneCopy might happen to
work but that's just luck. If one of its internal constants change
(kMaxPendingCopyOperations) this might start failing.

Could we instead instrument the raster source so it would be able to block for
testing purposes? That would work with all RasterWorkerPool implementations.

[danakj, 2014-10-31 20:43:02]

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

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1295: }
On 2014/10/31 20:28:39, reveman wrote:
> How about using conditional assignment here instead of if-else?

It's a similar number of lines of code with the line wrapping, and I thot this
was easier to read.

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5491:
TestContext()->set_query_result_available_ext(1);
On 2014/10/31 20:28:39, reveman wrote:
> I don't think this is a reliable way to block tiles from completing. What if
> ZeroCopy/OneCopy/Bitmap/GpuRasterWorkerPool is used? OneCopy might happen to
> work but that's just luck. If one of its internal constants change
> (kMaxPendingCopyOperations) this might start failing.

It doesn't work for those, and turning them on by default in unit tests would
causes these tests to fail. It is reliable with the test harness as it is today.

> Could we instead instrument the raster source so it would be able to block for
> testing purposes? That would work with all RasterWorkerPool implementations.

Raster is a synchronous task, so I'm not sure how that would work. Whereas
uploads are an asynchronous task which lets this work. Can you give me something
a bit more concrete? I don't think I'm seeing what you're seeing.

[reveman, 2014-10-31 21:38:13]

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

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1295: }
On 2014/10/31 20:43:01, danakj wrote:
> On 2014/10/31 20:28:39, reveman wrote:
> > How about using conditional assignment here instead of if-else?
> 
> It's a similar number of lines of code with the line wrapping, and I thot this
> was easier to read.

Acknowledged.

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5491:
TestContext()->set_query_result_available_ext(1);
On 2014/10/31 20:43:02, danakj wrote:
> On 2014/10/31 20:28:39, reveman wrote:
> > I don't think this is a reliable way to block tiles from completing. What if
> > ZeroCopy/OneCopy/Bitmap/GpuRasterWorkerPool is used? OneCopy might happen to
> > work but that's just luck. If one of its internal constants change
> > (kMaxPendingCopyOperations) this might start failing.
> 
> It doesn't work for those, and turning them on by default in unit tests would
> causes these tests to fail. It is reliable with the test harness as it is
today.
> 
> > Could we instead instrument the raster source so it would be able to block
for
> > testing purposes? That would work with all RasterWorkerPool implementations.
> 
> Raster is a synchronous task, so I'm not sure how that would work. Whereas
> uploads are an asynchronous task which lets this work.

Raster is asynchronous. You might have trouble getting this working with gpu
raster as it performs raster asynchronously on the compositor thread but it
should be possible in all other cases.

> Can you give me something
> a bit more concrete? I don't think I'm seeing what you're seeing.

Can you have picture playback block on a base::WaitableEvent that you can Signal
from here?

[brianderson, 2014-10-31 22:42:54]

https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la...
File cc/test/fake_content_layer_client.cc (left):

https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la...
cc/test/fake_content_layer_client.cc:47: draw_rect.Inset(draw_rect.width() /
4.0f, draw_rect.height() / 4.0f);
Why is this change needed? Seems unrelated.

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1325: client_->NotifyReadyToDraw();
Was this accidentally removed? The scheduler doesn't use NotifyReadyToDraw yet,
but it will soon.

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

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1116: // as part of the current frame being
drawn.g
typo: ".g"

[danakj, 2014-11-03 16:26:04]

https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la...
File cc/test/fake_content_layer_client.cc (left):

https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la...
cc/test/fake_content_layer_client.cc:47: draw_rect.Inset(draw_rect.width() /
4.0f, draw_rect.height() / 4.0f);
On 2014/10/31 22:42:53, brianderson wrote:
> Why is this change needed? Seems unrelated.

To make every tile not solid color, ie so blocking raster/upload does something.

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1325: client_->NotifyReadyToDraw();
On 2014/10/31 22:42:54, brianderson wrote:
> Was this accidentally removed? The scheduler doesn't use NotifyReadyToDraw
yet,
> but it will soon.

I assumed it could go away since we already SetNeedsRedraw() for each tile that
gets completed. From your comment elsewhere it sounds like not.

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

https://codereview.chromium.org/671653005/diff/410001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1116: // as part of the current frame being
drawn.g
On 2014/10/31 22:42:53, brianderson wrote:
> typo: ".g"

I blame emacs. Thanks :)

[danakj, 2014-11-05 16:06:48]

On 2014/10/31 21:38:13, reveman wrote:
> Can you have picture playback block on a base::WaitableEvent that you can
Signal
> from here?

I'd like to make this work but I can't test 1-copy to see if the test passes
there anyhow due to crashing:
https://code.google.com/p/chromium/issues/detail?id=428071

This kinda worries me since we've enabled it now, this DCHECK seems legit.

[danakj, 2014-11-05 19:40:02]

On 2014/11/05 16:06:48, danakj wrote:
> On 2014/10/31 21:38:13, reveman wrote:
> > Can you have picture playback block on a base::WaitableEvent that you can
> Signal
> > from here?
> 
> I'd like to make this work but I can't test 1-copy to see if the test passes
> there anyhow due to crashing:
> https://code.google.com/p/chromium/issues/detail?id=428071
> 
> This kinda worries me since we've enabled it now, this DCHECK seems legit.

OK it turns out this is from my use of the TestContext3d, making it return
inconsistent things from TestWebGraphicsContext3D::getQueryObjectuivEXT. For the
tests to work with 1-copy we should use a FakeRasterSource that can delay
raster, but this isn't possible until vmpstr@ does some more refactoring. Let's
not block this CL on all of that? I've added some TODOs.

[danakj, 2014-11-07 22:07:49]

On 2014/11/05 19:40:02, danakj wrote:
> On 2014/11/05 16:06:48, danakj wrote:
> > On 2014/10/31 21:38:13, reveman wrote:
> > > Can you have picture playback block on a base::WaitableEvent that you can
> > Signal
> > > from here?
> > 
> > I'd like to make this work but I can't test 1-copy to see if the test passes
> > there anyhow due to crashing:
> > https://code.google.com/p/chromium/issues/detail?id=428071
> > 
> > This kinda worries me since we've enabled it now, this DCHECK seems legit.
> 
> OK it turns out this is from my use of the TestContext3d, making it return
> inconsistent things from TestWebGraphicsContext3D::getQueryObjectuivEXT. For
the
> tests to work with 1-copy we should use a FakeRasterSource that can delay
> raster, but this isn't possible until vmpstr@ does some more refactoring.
Let's
> not block this CL on all of that? I've added some TODOs.

Weeee, ok I got these tests working with delaying raster instead of poking the
TestWGC3d. It uses the new DelayedRasterPicturePileImpl and
DelayedRasterTrigger.

[danakj, 2014-11-07 22:09:27]

Patchset #24 (id:490001) has been deleted

[danakj, 2014-11-10 17:45:01]

I think this CL does everything everyone has asked from it. Would love to get
some final reviews please :)

[brianderson, 2014-11-10 22:12:31]

lgtm

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:292: virtual bool SwapBuffers(const FrameData&
frame);
Does this still need to return a bool?

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5492: EndTestAfterDelayMs(16 * 4);
If there is a failure, this could be flaky, but I guess the delay is long enough
that it would be very likely to fail.

Still, would be nice to end this test ASAP rather than after a delay. Once
https://codereview.chromium.org/619843002 lands, we could use the
FakeExternalBeginFrameSource to end the test once NeedsBeginFrames goes false.
TODO?

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5497: EXPECT_TRUE(false);
ADD_FAILURE() << "No draws should happen once we have a complete frame.";

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_damage.cc (left):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_damage.cc:532: class
LayerTreeHostDamageTestVisibleTilesStillTriggerDraws
Does LayerTreeHostTestContinuousDrawWhenCreatingVisibleTiles effectively replace
this test?

[vmpstr, 2014-11-10 22:47:05]

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
File cc/test/delayed_raster_trigger.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:20: class DelayedRasterTrigger {
nit: This is just a DelayedTrigger right? (I'm not sure that Raster in the name
is helpful). 

Also, can you add a brief comment about how to use this class?

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

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1348: if (active_tree_ &&
!client_->IsInsideDraw() && tile->required_for_draw()) {
Is there any way to push this check into picture layer impl?

I'm a bit hesitant about tile->required_for_draw() call here, since it can be
out of date. There's a PLT::IsTileRequiredForDraw (or there should be?) that
would be a better alternative. 

Wdyt?

[vmpstr, 2014-11-10 22:51:27]

On 2014/11/10 22:47:05, vmpstr wrote:
>
https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
> File cc/test/delayed_raster_trigger.h (right):
> 
>
https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
> cc/test/delayed_raster_trigger.h:20: class DelayedRasterTrigger {
> nit: This is just a DelayedTrigger right? (I'm not sure that Raster in the
name
> is helpful). 
> 
> Also, can you add a brief comment about how to use this class?
> 
>
https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:1348: if (active_tree_ &&
> !client_->IsInsideDraw() && tile->required_for_draw()) {
> Is there any way to push this check into picture layer impl?
> 
> I'm a bit hesitant about tile->required_for_draw() call here, since it can be
> out of date. There's a PLT::IsTileRequiredForDraw (or there should be?) that
> would be a better alternative. 
> 
> Wdyt?

I guess there was some discussion about that in the first patch set...

[reveman, 2014-11-10 23:43:02]

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
File cc/test/delayed_raster_picture_pile_impl.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_picture_pile_impl.h:14: class
DelayedRasterPicturePileImpl : public PicturePileImpl {
Did you consider adding this functionality to FakePicturePileImpl instead of
introducing a new class?

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_picture_pile_impl.h:30: virtual
~DelayedRasterPicturePileImpl();
~DelayedRasterPicturePileImpl() override;

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
File cc/test/delayed_raster_trigger.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:15: class SingleThreadTaskRunner;
what are these for?

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:25: void Delay();
The name of this confused me. How about Wait() instead? That seems to better
describe what it does.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:32: std::vector<base::WaitableEvent*>
delayed_raster_tasks_;
hm, wouldn't it be enough with one WaitableEvent that multiple threads can wait
on? why the vector?

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer.cc (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
cc/test/fake_picture_layer.cc:51: GetPicturePileForTesting(),
delayed_raster_trigger_);
note: if you end up reusing FakePicturePileImpl then maybe a bit cleaner to
create a picturepileimpl of that type here unconditionally and instead handle
the case where delayed_raster_trigger_ is null inside the pile implementation
instead.

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
cc/test/fake_picture_layer.h:58: DelayedRasterTrigger* delayed_raster_trigger_;
Could this simply be a "base::WaitableEvent*
picture_pile_playback_is_allowed_to_complete_"? Or is the idea that
DelayedRasterTrigger would be able to do more like synthetic delays?

[enne (OOO), 2014-11-11 18:09:31]

lgtm % other reviewers.  I think all my previous high level concerns got
addressed by making this depend on the ReadyToDraw signal.

[danakj, 2014-11-12 20:19:40]

PTAL

The continuous draws test fails now after rebase, I need to look into what
changed, probably in the NotifyDraw stuff, since I last rebased onto that.

But everything else should be addressed.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
File cc/test/delayed_raster_picture_pile_impl.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_picture_pile_impl.h:14: class
DelayedRasterPicturePileImpl : public PicturePileImpl {
On 2014/11/10 23:43:01, reveman wrote:
> Did you consider adding this functionality to FakePicturePileImpl instead of
> introducing a new class?

Done.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
File cc/test/delayed_raster_trigger.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:15: class SingleThreadTaskRunner;
On 2014/11/10 23:43:02, reveman wrote:
> what are these for?

Woops, from previous attempts to maket his go.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:20: class DelayedRasterTrigger {
On 2014/11/10 22:47:04, vmpstr wrote:
> nit: This is just a DelayedTrigger right? (I'm not sure that Raster in the
name
> is helpful). 
> 
> Also, can you add a brief comment about how to use this class?

Done. Renamed the class to ThreadBlocker, and renamed the Begin/End methods to
Block/Unblock.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:25: void Delay();
On 2014/11/10 23:43:02, reveman wrote:
> The name of this confused me. How about Wait() instead? That seems to better
> describe what it does.

Done.

https://codereview.chromium.org/671653005/diff/550001/cc/test/delayed_raster_...
cc/test/delayed_raster_trigger.h:32: std::vector<base::WaitableEvent*>
delayed_raster_tasks_;
On 2014/11/10 23:43:02, reveman wrote:
> hm, wouldn't it be enough with one WaitableEvent that multiple threads can
wait
> on? why the vector?

Right, thanks! Done.

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer.cc (right):

https://codereview.chromium.org/671653005/diff/550001/cc/test/fake_picture_la...
cc/test/fake_picture_layer.cc:51: GetPicturePileForTesting(),
delayed_raster_trigger_);
On 2014/11/10 23:43:02, reveman wrote:
> note: if you end up reusing FakePicturePileImpl then maybe a bit cleaner to
> create a picturepileimpl of that type here unconditionally and instead handle
> the case where delayed_raster_trigger_ is null inside the pile implementation
> instead.

I added a constructor to FakePictureLayer to override the pile, since
PictureLayer now sets up the pile in its constructor.

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:292: virtual bool SwapBuffers(const FrameData&
frame);
On 2014/11/10 22:12:31, brianderson wrote:
> Does this still need to return a bool?

Ya it's used by some tests and it seems useful for them.

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5492: EndTestAfterDelayMs(16 * 4);
On 2014/11/10 22:12:31, brianderson wrote:
> If there is a failure, this could be flaky, but I guess the delay is long
enough
> that it would be very likely to fail.
> 
> Still, would be nice to end this test ASAP rather than after a delay. Once
> https://codereview.chromium.org/619843002 lands, we could use the
> FakeExternalBeginFrameSource to end the test once NeedsBeginFrames goes false.
> TODO?

Done.

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5492: EndTestAfterDelayMs(16 * 4);
On 2014/11/10 22:12:31, brianderson wrote:
> If there is a failure, this could be flaky, but I guess the delay is long
enough
> that it would be very likely to fail.
> 
> Still, would be nice to end this test ASAP rather than after a delay. Once
> https://codereview.chromium.org/619843002 lands, we could use the
> FakeExternalBeginFrameSource to end the test once NeedsBeginFrames goes false.
> TODO?

Done.

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5497: EXPECT_TRUE(false);
On 2014/11/10 22:12:31, brianderson wrote:
> ADD_FAILURE() << "No draws should happen once we have a complete frame.";

Done.

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_damage.cc (left):

https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_damage.cc:532: class
LayerTreeHostDamageTestVisibleTilesStillTriggerDraws
On 2014/11/10 22:12:31, brianderson wrote:
> Does LayerTreeHostTestContinuousDrawWhenCreatingVisibleTiles effectively
replace
> this test?

I think that it does, yes. It verifies that we continue to try drawing until we
get a complete frame, and then stop after.

Is there something you think we're losing coverage of?

[danakj, 2014-11-12 20:50:35]

https://codereview.chromium.org/671653005/diff/610001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/671653005/diff/610001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:272: LayerTreeHostImpl::NotifyReadyToDraw();
Here's the fix for the unit test. The LayerTreeTest harness was re-implementing
LTHI::NotifyReadyTo* instead of calling up to the base class.

[brianderson, 2014-11-12 21:15:35]

On 2014/11/12 20:19:40, danakj wrote:

>
https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_unittest_damage.cc (left):
> 
>
https://codereview.chromium.org/671653005/diff/550001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_unittest_damage.cc:532: class
> LayerTreeHostDamageTestVisibleTilesStillTriggerDraws
> On 2014/11/10 22:12:31, brianderson wrote:
> > Does LayerTreeHostTestContinuousDrawWhenCreatingVisibleTiles effectively
> replace
> > this test?
> 
> I think that it does, yes. It verifies that we continue to try drawing until
we
> get a complete frame, and then stop after.
> 
> Is there something you think we're losing coverage of?

Nope, just a sanity check.

[danakj, 2014-11-12 22:02:08]

Updated the description to what this CL finally ended up doing.

[danakj, 2014-11-12 22:06:31]

Patchset #30 (id:590002) has been deleted

[reveman, 2014-11-13 20:10:04]

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker.h
File cc/test/thread_blocker.h (right):

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker....
cc/test/thread_blocker.h:17: class ThreadBlocker {
What's the difference between an instance of this class and a
base::WaitableEvent instance that is initially in signaled state?

[vmpstr, 2014-11-13 20:13:49]

Everything looks good, I just have a question about
LTHI::required_for_draw_tile_is_top_of_raster_queue_ below.

https://codereview.chromium.org/671653005/diff/640001/cc/resources/picture_pi...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/671653005/diff/640001/cc/resources/picture_pi...
cc/resources/picture_pile_impl.h:20: #include
"third_party/skia/include/core/SkPicture.h"
nit: Either this isn't needed or the forward declare isn't needed.

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

https://codereview.chromium.org/671653005/diff/640001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1203:
required_for_draw_tile_is_top_of_raster_queue_ =
Just so I understand this a bit better... Would just setting this to true work
equally well? For the cases where the top of the queue _is_ required to draw,
this is equivalent. For other cases (there are no tasks that are required for
draw at the top of the queue), then we should get the NotifyReadyToDraw signal
_fairly_ quickly.

Or would this cause us to possibly draw unnecessarily?

[danakj, 2014-11-13 20:14:17]

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker.h
File cc/test/thread_blocker.h (right):

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker....
cc/test/thread_blocker.h:17: class ThreadBlocker {
On 2014/11/13 20:10:04, reveman wrote:
> What's the difference between an instance of this class and a
> base::WaitableEvent instance that is initially in signaled state?

A nicer API around should_delay_ I think for this use case.

[danakj, 2014-11-13 20:20:18]

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker.h
File cc/test/thread_blocker.h (right):

https://codereview.chromium.org/671653005/diff/640001/cc/test/thread_blocker....
cc/test/thread_blocker.h:17: class ThreadBlocker {
On 2014/11/13 20:14:17, danakj wrote:
> On 2014/11/13 20:10:04, reveman wrote:
> > What's the difference between an instance of this class and a
> > base::WaitableEvent instance that is initially in signaled state?
> 
> A nicer API around should_delay_ I think for this use case.

Mmm, I guess the test could Reset() instead of BlockWait() and Signal() instead
of UnblockWait(). I could at least get rid of the should_delay_ bool and the
lock I guess... and then it looks just like WaitableEvent, heh. Guess I should
just use that.

[danakj, 2014-11-13 20:27:54]

https://codereview.chromium.org/671653005/diff/640001/cc/resources/picture_pi...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/671653005/diff/640001/cc/resources/picture_pi...
cc/resources/picture_pile_impl.h:20: #include
"third_party/skia/include/core/SkPicture.h"
On 2014/11/13 20:13:49, vmpstr wrote:
> nit: Either this isn't needed or the forward declare isn't needed.

Removed the header.

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

https://codereview.chromium.org/671653005/diff/640001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1203:
required_for_draw_tile_is_top_of_raster_queue_ =
On 2014/11/13 20:13:49, vmpstr wrote:
> Just so I understand this a bit better... Would just setting this to true work
> equally well? For the cases where the top of the queue _is_ required to draw,
> this is equivalent. For other cases (there are no tasks that are required for
> draw at the top of the queue), then we should get the NotifyReadyToDraw signal
> _fairly_ quickly.
> 
> Or would this cause us to possibly draw unnecessarily?

It would make us try to draw every begin impl frame because we're expecting to
be able to draw something new very soon.

[danakj, 2014-11-13 20:28:16]

Removed the ThreadBlocker, using a base::WaitableEvent directly.

[vmpstr, 2014-11-13 20:37:41]

lgtm

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

https://codereview.chromium.org/671653005/diff/640001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1203:
required_for_draw_tile_is_top_of_raster_queue_ =
On 2014/11/13 20:27:54, danakj wrote:
> On 2014/11/13 20:13:49, vmpstr wrote:
> > Just so I understand this a bit better... Would just setting this to true
work
> > equally well? For the cases where the top of the queue _is_ required to
draw,
> > this is equivalent. For other cases (there are no tasks that are required
for
> > draw at the top of the queue), then we should get the NotifyReadyToDraw
signal
> > _fairly_ quickly.
> > 
> > Or would this cause us to possibly draw unnecessarily?
> 
> It would make us try to draw every begin impl frame because we're expecting to
> be able to draw something new very soon.

Per offline discussion, please leave a comment here about Top being a heuristic
not a definite statement that we have a tile ready to draw in the queue.

[danakj, 2014-11-13 20:39:41]

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

https://codereview.chromium.org/671653005/diff/640001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1203:
required_for_draw_tile_is_top_of_raster_queue_ =
On 2014/11/13 20:37:41, vmpstr wrote:
> On 2014/11/13 20:27:54, danakj wrote:
> > On 2014/11/13 20:13:49, vmpstr wrote:
> > > Just so I understand this a bit better... Would just setting this to true
> work
> > > equally well? For the cases where the top of the queue _is_ required to
> draw,
> > > this is equivalent. For other cases (there are no tasks that are required
> for
> > > draw at the top of the queue), then we should get the NotifyReadyToDraw
> signal
> > > _fairly_ quickly.
> > > 
> > > Or would this cause us to possibly draw unnecessarily?
> > 
> > It would make us try to draw every begin impl frame because we're expecting
to
> > be able to draw something new very soon.
> 
> Per offline discussion, please leave a comment here about Top being a
heuristic
> not a definite statement that we have a tile ready to draw in the queue.

Done.

[reveman, 2014-11-13 20:50:59]

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:64: base::WaitableEvent* raster_thread_blocker_;
I don't think this code should be aware of raster threads or make the assumption
that playback might happen on a raster thread. Can you rename this and related
code to something like "playback_allowed_event_?

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile_impl.cc (right):

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile_impl.cc:100: PicturePileImpl::PlaybackToCanvas(canvas,
canvas_rect, contents_scale);
Move the Wait() call above this line if you use my |playback_allowed_event_|
name suggestion. I'm assuming this doesn't matter for your use-case. Not sure
what to name it if keep the Wait() call below instead.
|allow_playback_to_finish_event_| maybe..

[danakj, 2014-11-13 21:24:40]

PTAL

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:64: base::WaitableEvent* raster_thread_blocker_;
On 2014/11/13 20:50:58, reveman wrote:
> I don't think this code should be aware of raster threads or make the
assumption
> that playback might happen on a raster thread. Can you rename this and related
> code to something like "playback_allowed_event_?

Hm, heh. I named it this thinking "it blocks the thread that does raster", which
is what it's doing, not so much about the concept of "raster threads". And the
playback /has/ to happen on a different thread or this test would deadlock when
it called Wait(). This is test code, is it really that important? I'll just
rename this to unblock me, but I don't see the point of this change I guess.

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile_impl.cc (right):

https://codereview.chromium.org/671653005/diff/660001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile_impl.cc:100: PicturePileImpl::PlaybackToCanvas(canvas,
canvas_rect, contents_scale);
On 2014/11/13 20:50:58, reveman wrote:
> Move the Wait() call above this line if you use my |playback_allowed_event_|
> name suggestion. I'm assuming this doesn't matter for your use-case. Not sure
> what to name it if keep the Wait() call below instead.
> |allow_playback_to_finish_event_| maybe..

Right it doesn't matter, it's just blocking this function returning. Done.

[reveman, 2014-11-13 21:52:48]

lgtm

https://codereview.chromium.org/671653005/diff/720001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile_impl.h (right):

https://codereview.chromium.org/671653005/diff/720001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile_impl.h:32: base::WaitableEvent* blocker);
nit: s/blocker/playback_allowed_event/

https://codereview.chromium.org/671653005/diff/720001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/720001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5511: //
TestContext()->set_query_result_available_ext(1);
nit: is this comment debug code? please remove it.

[danakj, 2014-11-13 21:54:11]

https://codereview.chromium.org/671653005/diff/720001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/671653005/diff/720001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5511: //
TestContext()->set_query_result_available_ext(1);
On 2014/11/13 21:52:48, reveman wrote:
> nit: is this comment debug code? please remove it.

Oh that's left from before, yah. Thanks!

[danakj, 2014-11-13 23:16:33]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-11-13 23:19:10]

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

[commit-bot: I haz the power, 2014-11-14 02:05:14]

Committed patchset #35 (id:740001)

[commit-bot: I haz the power, 2014-11-14 02:06:12]

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