cc: Use BeginFrameArgs

cc: Use BeginFrameArgs

This patch adds logic to the Scheduler to actually use the BeginFrame 
and deadline.

SchedulerStateMachine::begin_frame_state has been added and can be in one
of 4 states:  Idle, InsideBeginFrame, DeadlinePending, or InsideDeadline.

Notable restrictions of the states are:
- We start a commit as soon after InsideBeginFrame as we can, since the
    BeginFrame will be coordinated with user input.
- We do not start a commit while Idle, in order to wait for the next
    batch of user input.
- Draw and swap only occurs during the InsideDeadline state.

The deadlines of the Browser and Renderer compositor can be staggered in
order to have a total draw latency of < 1 frame when starting off. If we can't
hit 1 frame of latency consistently, we will fallback to a higher latency
mode to increase throughput.

Making these changes exposed some bugs and necessitated some other changes
that are hard to commit separately. Notable changes include:

1) The Scheduler listens to the ReadyToActivate signal and makes the final
    call regarding when to activate. This avoids activating commit 2 before
    commit 1 has been drawn. It also helps reduce the number of calls to
    ManageTiles because we don't call ActivatePendingTreeIfNeeded every frame.

2) An OutputSurfaceState to make sure we do not draw after output surface
    initialization until after the first commit.

3) A ForcedRedrawOnTimeoutState that forces a draw and swap if we've skipped
    too many draws because of checkerboarding. This is synchronized with the
    normal display pipeline and respects the BeginFrame and deadline.

4) A SynchronousReadback state to track readback commits and their
    "replacement commits" to make sure we don't draw before activation
    of the readback commit and don't swap before activation of the
    replacement commit. This executes as fast as possible and does not
    respect the BeginFrame/deadline timing semantics. Back to back
    readbacks before the replacement commit arrive are supported.

5) A number of unit test updates and fixes.

BUG=243461
BUG=269272

[brianderson, 2013-06-14 04:08:58]

First draft. PTAL. It still needs a lot of characterization and fine tuning.

Depends on: https://codereview.chromium.org/16863005

[brianderson, 2013-06-14 04:16:29]

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:209: do {
Changing to a do-while loop allows UpdateState to consider ACTION_NONE.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:184: int current_frame_attempt_;
current_frame_attempt_ allows us to reset the last_frame_attempt* variables
because we want to check some things multiple times per frame (begin frame
start, pending tree activation, and deadline).

[ajuma, 2013-06-14 15:22:32]

I really like the overall direction here.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:114: bool
SchedulerStateMachine::HasAttemptedTreeActivationThisFrame() const {
Calling this HasAttemptedTreeActivationThisFrameAttempt would make it clear that
we're trying multiple times per frame.

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1050: base::TimeDelta delta = deadline -
base::TimeTicks::Now();
To get a sufficiently precise delta on Windows, we'll need HighResNow() (also
when calculating the deadline itself).

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1059: deadline - base::TimeTicks::Now());
We can reuse |delta| here.

[Sami, 2013-06-14 15:59:20]

Nice, it's all coming together :)

https://codereview.chromium.org/16871016/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16871016/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:26: BeginFrameArgs
CreateBeginFrameWithAdjustedDeadline(base::TimeDelta delta);
Nit: I think calling this Create[...] is a little confusing because it's not a
constructor but a closer to something like Clone(). Maybe it would be more
straightforward to add a setter for the deadline.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:140:
client_->SetNeedsBeginFrameOnImplThread(needs_begin_frame);
nit: This function might be a little easier to read if you moved the repeated
calls to SetNeedsBeginFrameOnImplThread and last_set_needs_begin_frame_ to the
end.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:209: do {
On 2013/06/14 04:16:29, Brian Anderson wrote:
> Changing to a do-while loop allows UpdateState to consider ACTION_NONE.

Advancing the begin frame state on ACTION_NONE seems a little fishy, but I guess
there's no straightforward way to use explicit actions instead is there?

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:33: active_tree_has_been_drawn_(true),
Should this be false?

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:397: begin_frame_state_ =
BEGIN_FRAME_DRAWING;
Let me see if I got this right:

1. BeginFrame => BEGIN_FRAME_STARTING
2. Main thread processing starts => BEGIN_FRAME_WAITING
3. Deadline hit => BEGIN_FRAME_DRAWING
4. Main thread done, commit and activate tree => still in BEGIN_FRAME_DRAWING

Does this mean that we'll draw as soon as the tree activates even though we
passed the deadline? I think that's great for latency but maybe not what the
parent compositor is expecting :)

It also seems at odds with what happens if the tree activation happens right
before the deadline task runs, in which case we end up in BEGIN_FRAME_IDLE and
don't draw until the next BeginFrame.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:89: void StartBeginFrame(BeginFrameArgs
args);
bikeshed: StartBeginFrame sounds weird :) How about just OnBeginFrame or
DidBeginFrame?

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1056: Proxy::ImplThread()->PostDelayedTask(
Can/should we cancel this task if we drew before the deadline?

[brianderson, 2013-06-14 18:42:35]

https://codereview.chromium.org/16871016/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16871016/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:26: BeginFrameArgs
CreateBeginFrameWithAdjustedDeadline(base::TimeDelta delta);
On 2013/06/14 15:59:20, Sami wrote:
> Nit: I think calling this Create[...] is a little confusing because it's not a
> constructor but a closer to something like Clone(). Maybe it would be more
> straightforward to add a setter for the deadline.

True. I will change this to a setter for the deadline.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:140:
client_->SetNeedsBeginFrameOnImplThread(needs_begin_frame);
On 2013/06/14 15:59:20, Sami wrote:
> nit: This function might be a little easier to read if you moved the repeated
> calls to SetNeedsBeginFrameOnImplThread and last_set_needs_begin_frame_ to the
> end.

Ok. This could definitely be simplified.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:209: do {
On 2013/06/14 15:59:20, Sami wrote:
> On 2013/06/14 04:16:29, Brian Anderson wrote:
> > Changing to a do-while loop allows UpdateState to consider ACTION_NONE.
> 
> Advancing the begin frame state on ACTION_NONE seems a little fishy, but I
guess
> there's no straightforward way to use explicit actions instead is there?

It is a bit fishy, but I couldn't think of a simpler solution. It would be
pretty round about to create an explicit action just to advance the BeginFrame
state. If we could anticipate that the *next* action would be ACTION_NONE, we
could advance the begin_frame_state ahead of time, but I think that would be
hard to do since the current action might trigger further actions depending on
the current action's outcome.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:33: active_tree_has_been_drawn_(true),
On 2013/06/14 15:59:20, Sami wrote:
> Should this be false?

If the active tree has been drawn, then this patch will wait for another tree
activation after a BeginFrame before drawing again.
If the active tree has not been drawn (because it was activated while we were in
BEGIN_FRAME_IDLE), then we will draw immediately on a BeginFrame.

I initialized it to true so we wait for tree activation for the first frame.

I will change the variable name to begin_frame_should_draw_immediately so the
name reflects the effect rather than the cause.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:111: return current_frame_attempt_ ==
last_frame_attempt_draw_was_called_;
I should change back to use current_frame_number since we should only draw once
per frame.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:114: bool
SchedulerStateMachine::HasAttemptedTreeActivationThisFrame() const {
On 2013/06/14 15:22:32, ajuma wrote:
> Calling this HasAttemptedTreeActivationThisFrameAttempt would make it clear
that
> we're trying multiple times per frame.

Will do.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:397: begin_frame_state_ =
BEGIN_FRAME_DRAWING;
On 2013/06/14 15:59:20, Sami wrote:
> Let me see if I got this right:
> 
> 1. BeginFrame => BEGIN_FRAME_STARTING
> 2. Main thread processing starts => BEGIN_FRAME_WAITING

Almost:
2.a Main thread processing starts => BEGIN_FRAME_WAITING if active tree has been
drawn.
2.b Main thread processing starts => BEGIN_FRAME_DRAWING if active tree has not
been drawn.

> 3. Deadline hit => BEGIN_FRAME_DRAWING
> 4. Main thread done, commit and activate tree => still in BEGIN_FRAME_DRAWING
> 
> Does this mean that we'll draw as soon as the tree activates even though we
> passed the deadline? I think that's great for latency but maybe not what the
> parent compositor is expecting :)

The transition to BEGIN_FRAME_DRAWING will only occur for 3 and 4 if we are in
BEGIN_FRAME_WAITING. BEGIN_FRAME_DRAWING always transitions to BEGIN_FRAME_IDLE
(as part of UpdateState(ACTION_NONE), regardless of whether a draw actually
happened) which prevents us from going back to BEGIN_FRAME_DRAWING twice for the
same BeginFrame.

> 
> It also seems at odds with what happens if the tree activation happens right
> before the deadline task runs, in which case we end up in BEGIN_FRAME_IDLE and
> don't draw until the next BeginFrame.

The logic should draw if activation occurs before the deadline.

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:89: void StartBeginFrame(BeginFrameArgs
args);
On 2013/06/14 15:59:20, Sami wrote:
> bikeshed: StartBeginFrame sounds weird :) How about just OnBeginFrame or
> DidBeginFrame?

OnBeginFrame sounds good.

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1050: base::TimeDelta delta = deadline -
base::TimeTicks::Now();
On 2013/06/14 15:22:32, ajuma wrote:
> To get a sufficiently precise delta on Windows, we'll need HighResNow() (also
> when calculating the deadline itself).

Ok. We'll have to be very careful about comparing HighResNow() and Now().
@simonjam mentioned that base::TimeTicks makes an attempt to keep them
comparable, but I'm not sure we can trust it.

Also, HighResNow doesn't work very well on some old Windows+CPU+driver
combinations - the result isn't monotonically increasing if called on different
cores.

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1056: Proxy::ImplThread()->PostDelayedTask(
On 2013/06/14 15:59:20, Sami wrote:
> Can/should we cancel this task if we drew before the deadline?

It's kind of "canceled" manually based on the begin_frame_state_. Is there a way
to cancel it so that it doesn't even wake up the thread?

https://codereview.chromium.org/16871016/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1059: deadline - base::TimeTicks::Now());
On 2013/06/14 15:22:32, ajuma wrote:
> We can reuse |delta| here.

oops!

[brianderson, 2013-06-21 01:02:52]

PTAL.

Joth, Bo, can you guys test this patch locally to see if anything needs to
change for the WebView?

I still need to fix tests, but this version of the patch can get a CrOS build
(ThreadProxy+!impl_side_painting Browser compositor,
ThreadProxy+impl_side_painting Renderer compositor) down to 1 frame of latency
from 4 frames. Just make sure to set BeginFrameArgs::DefaultDeadlineAdjustment
to something sane.

Most of the comments have been addressed.

I'm still sticking with TimeTicks::Now() because that is what's used by the
DelayBasedTimeSource. If we want to switch to HighResNow, we should do it in a
separate patch.

Now to test this on Android and hook it up to Ali's estimation patch (and fix
tests.)

[nduca, 2013-06-21 01:14:18]

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:50: virtual void PostBeginFrameDeadline(const
base::Closure& closure,
i think this should be done by the output surface, which should tell us via the
client interface when the deadline fires...

[nduca, 2013-06-21 01:21:40]

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:227: "state", state_machine_.ToString());
medium term, might as well make StateMachine have an AsValue, and make
StateMachine issue a trace_event_snapshot. Once you do that, yo'ull get dots in
the UI for the state machine state dumps. See LayerTreeHostImpl::Draw and
AsValue for examples.

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:435: current_frame_number_++;
i think probably this needs to bump at the deadline or when we draw, not here.

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:438: begin_frame_state_ =
BEGIN_FRAME_STARTING;
This factoring seems a little wierd. Why not

begin_frame_state = INSIDE_BEGIN_FRAME, BEGIN_FRAME_DEADLINE_PENDING,
INSIDE_DEADLINE, and IDLE?

THat preserves the inside/outside notion that we usually have

[brianderson, 2013-06-21 01:27:20]

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:34: enum BeginFrameState {
Note: This version of the patch enforces the rule that the BeginFrameState will
always cycle through all the states in order, whether or not it actually waits
or draws. It will at least *try* to wait or *try* to draw in those states.

I found it easier to reason about than the branching state sequence of the first
patch.

I will add a comment about this.

[brianderson, 2013-06-21 01:42:47]

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:227: "state", state_machine_.ToString());
On 2013/06/21 01:21:40, nduca wrote:
> medium term, might as well make StateMachine have an AsValue, and make
> StateMachine issue a trace_event_snapshot. Once you do that, yo'ull get dots
in
> the UI for the state machine state dumps. See LayerTreeHostImpl::Draw and
> AsValue for examples.

Ooh, neat. I wasn't planing to commit the TRACE_EVENT itself but, if I implement
AsValue, I'll commit the AsValue method.

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:50: virtual void PostBeginFrameDeadline(const
base::Closure& closure,
On 2013/06/21 01:14:18, nduca wrote:
> i think this should be done by the output surface, which should tell us via
the
> client interface when the deadline fires...

I was originally going to implement it that way, but found out that whoever
posts the deadline task also needs to know about the expected draw time. 

Posting the task for the Browser's deadline will come too late. We need to post
a task for the Renderer's draw deadline, which I think can be better handled by
the Scheduler and ThreadProxy. Otherwise, we'd need to communicate the expected
draw time to the OutputSurface, which I'm not sure we'd want to do.

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:435: current_frame_number_++;
On 2013/06/21 01:21:40, nduca wrote:
> i think probably this needs to bump at the deadline or when we draw, not here.

There was a reason I put that here, but it may not apply anymore. I'll see if I
can move it to the deadline since that makes more sense.

https://codereview.chromium.org/16871016/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:438: begin_frame_state_ =
BEGIN_FRAME_STARTING;
On 2013/06/21 01:21:40, nduca wrote:
> This factoring seems a little wierd. Why not
> 
> begin_frame_state = INSIDE_BEGIN_FRAME, BEGIN_FRAME_DEADLINE_PENDING,
> INSIDE_DEADLINE, and IDLE?
> 
> THat preserves the inside/outside notion that we usually have

That sounds good. I'm not very attached to my generic naming.

[brianderson, 2013-06-21 02:08:35]

Addressed comments in patch set 2.

[nduca, 2013-06-21 02:45:00]

Thanks for clarifying.

> I was originally going to implement it that way, but found out that whoever
> posts the deadline task also needs to know about the expected draw time.
> 
> Posting the task for the Browser's deadline will come too late. We need to
post
> a task for the Renderer's draw deadline, which I think can be better handled
by
> the Scheduler and ThreadProxy. Otherwise, we'd need to communicate the
expected
> draw time to the OutputSurface, which I'm not sure we'd want to do.

Actually I think thats exactly what we should be doing. We should be giving
willDraw, didDraw estimates to the output surface and it can be computing draw
time.

My point of view here is that the scheduler isn't *all* scheduling. Rather, its
just the scheduling state that is about main-thread and impl-thread
coordination. In contrast, the output surface is supposed to deal with
impl-thread to parent coordination. Hence my recommendation that it have the
estimation and deadline logic.

[boliu, 2013-06-21 03:35:09]

Thanks for looping us in. I applied Patch set 2 (btw it needs a rebase) and
nothing appears to regress downstream, so good to go from webview's end with one
comment.

I got very excited reading this patch. So if I understand correctly, draw
happens in OnBeginFrameDeadline now. And if ShouldTriggerBeginFrameDeadlineEarly
returns false, then BeginFrame is only a signal for tree activation?

Then if Deadline could be exposed to output surface and
ShouldTriggerBeginFrameDeadlineEarly always returns false, it would fit very
well with android framework's draw behavior. There is a clear signal to begin
the next frame, and another callback to make all the gl calls to do the draw.
Right now we essentially do nothing in the first signal, and do a massive begin
frame task on the second callback. It would be great if we could split this big
task into two separate tasks. And it would avoid changing scroll offset (ie tree
activation) during draw, another problem we are struggling with.

Unfortuantely this doesn't fit very well into the deadline model because the the
timing between these two calls depends on the embedding app, draw mode etc that
are out of our control.

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:183: OnBeginFrameDeadline();
For now though, android webview needs to always fall into this case since it
needs to draw before begin frame returns.

[Sami, 2013-06-21 10:27:42]

I think the new begin frame state machine looks a lot simpler that the previous
one -- great job!

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:17: current_frame_number_(0),
Nit: I think it's a little confusing to have two counters with similar names but
more or less uncorrelated values. Would it make sense to
s/current_frame_attempt_/current_draw_attempt_/ or maybe
current_activation_attempt_? I'm not too opposed to the current name either if
you think it's more consistent.

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:228: bool can_commit = visible_ ||
needs_forced_commit_;
Nit: can_commit could be moved above this switch() and reused in the
COMMIT_STATE_IDLE state above.

https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:265: if
(ShouldSendBeginFrameToMainThread())
Nit: braces for multi-line 'if'.

[brianderson, 2013-06-21 17:11:39]

On 2013/06/21 03:35:09, boliu wrote:
> Thanks for looping us in. I applied Patch set 2 (btw it needs a rebase) and
> nothing appears to regress downstream, so good to go from webview's end with
one
> comment.
> 
> I got very excited reading this patch. So if I understand correctly, draw
> happens in OnBeginFrameDeadline now.

For the most part. It will still try to draw outside of OnBeginFrameDeadline if
needs_forced_redraw_ is true.

> And if ShouldTriggerBeginFrameDeadlineEarly
> returns false, then BeginFrame is only a signal for tree activation?

It's a little more complicated. If ShouldTriggerBeginFrameDeadlineEarly returns
false and we haven't passed the deadline, BeginFrame will not draw. BeginFrame
will still be used to send a BeginFrame to the main thread if one isn't already
pending. Although this patch still attempts activation in BeginFrame and the
deadline, integration of this patch and
https://codereview.chromium.org/17351017/ will decouple activation so it can
occur at anytime.

> 
> Then if Deadline could be exposed to output surface and
> ShouldTriggerBeginFrameDeadlineEarly always returns false, it would fit very
> well with android framework's draw behavior. There is a clear signal to begin
> the next frame, and another callback to make all the gl calls to do the draw.
> Right now we essentially do nothing in the first signal, and do a massive
begin
> frame task on the second callback. It would be great if we could split this
big
> task into two separate tasks. And it would avoid changing scroll offset (ie
tree
> activation) during draw, another problem we are struggling with.
> 
> Unfortuantely this doesn't fit very well into the deadline model because the
the
> timing between these two calls depends on the embedding app, draw mode etc
that
> are out of our control.

It sounds like being able to have the OutputSurface trigger the deadline
callback (as Nat suggested) would be very beneficial for WebView. If WebView's
OutputSurface's deadline callback didn't necessarily depend on the expected draw
time of the Renderer and could be triggered by the embedding app, would that fit
well?

> 
> https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/16871016/diff/24001/cc/scheduler/scheduler.cc...
> cc/scheduler/scheduler.cc:183: OnBeginFrameDeadline();
> For now though, android webview needs to always fall into this case since it
> needs to draw before begin frame returns.

[ajuma, 2013-06-21 17:15:24]

On 2013/06/21 01:02:52, Brian Anderson wrote:
> I still need to fix tests, but this version of the patch can get a CrOS build
> (ThreadProxy+!impl_side_painting Browser compositor,
> ThreadProxy+impl_side_painting Renderer compositor) down to 1 frame of latency
> from 4 frames.

Awesome, that's super-exciting to hear!

> I'm still sticking with TimeTicks::Now() because that is what's used by the
> DelayBasedTimeSource. If we want to switch to HighResNow, we should do it in a
> separate patch.

Sounds reasonable. We should keep an eye out for weird behavior on Windows.

[boliu, 2013-06-21 18:13:34]

On 2013/06/21 17:11:39, Brian Anderson wrote:
> On 2013/06/21 03:35:09, boliu wrote:
> > I got very excited reading this patch. So if I understand correctly, draw
> > happens in OnBeginFrameDeadline now.
> 
> For the most part. It will still try to draw outside of OnBeginFrameDeadline
if
> needs_forced_redraw_ is true.

Yep, forced draw is a problem for webview. See crbug.com/250909

> It sounds like being able to have the OutputSurface trigger the deadline
> callback (as Nat suggested) would be very beneficial for WebView. If WebView's
> OutputSurface's deadline callback didn't necessarily depend on the expected
draw
> time of the Renderer and could be triggered by the embedding app, would that
fit
> well?

Sounds perfect

[brianderson, 2013-06-21 22:30:08]

Addressed some more comments. I would like to hold off on the change where the
OutputSurface triggers the deadline until after the branch point. I've opened a
follow up bug here: https://code.google.com/p/chromium/issues/detail?id=252933.

Working on fixing and adding tests now.

[nduca, 2013-06-21 22:51:26]

LGTM for when its needed. This is a good step forward.

[boliu, 2013-06-24 16:47:55]

https://codereview.chromium.org/16871016/diff/34002/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/34002/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:144:
!settings_.using_synchronous_renderer_compositor);
It seems webview broke without proactive_begin_frame_wanted_.
Scheduler::SetCanDraw is never called with true.

I guess could you just remove this workaround for webview for this patch, then
we can work out something later?

[brianderson, 2013-06-24 18:54:06]

https://codereview.chromium.org/16871016/diff/34002/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/34002/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:144:
!settings_.using_synchronous_renderer_compositor);
On 2013/06/24 16:47:55, boliu wrote:
> It seems webview broke without proactive_begin_frame_wanted_.
> Scheduler::SetCanDraw is never called with true.
> 
> I guess could you just remove this workaround for webview for this patch, then
> we can work out something later?

Thanks for the heads up. I'll fix it in the next upload.

[brianderson, 2013-06-24 19:08:30]

Just wanted to give an update on this issue, since the branch point is supposed
to be today at 5pm.

I suspect I wont be able to hit the deadline and will have to get this patch in
after the branch point. Although I'm still trying to hit the deadline, I don't
want to rush this patch.

The effect of this patch on the tests is larger than I expected. The scheduling
changes affect a number of tests (even outside of the scheduler unit tests) that
made fragile assumptions about how the scheduler works and when frames would be
drawn. I am going through the failures one by one to make sure there isn't
actually a bug in the scheduler.

[Sami, 2013-07-02 10:28:05]

https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:236: TRACE_EVENT0("cc", str.c_str());
Can we trace strings like this nowadays? I remember some trouble earlier because
the string got free'd before the trace event was written.

https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:238: //TRACE_EVENT1("cc", "SchedulerStateMachine",
We could also uncomment this and put it to the disabled-by-default category as
we do with tree dumps.

[danakj, 2013-07-02 13:24:24]

https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:236: TRACE_EVENT0("cc", str.c_str());
On 2013/07/02 10:28:06, Sami wrote:
> Can we trace strings like this nowadays? I remember some trouble earlier
because
> the string got free'd before the trace event was written.

I believe there are COPY versions of the macros to save the string.

[brianderson, 2013-07-09 01:39:28]

>
https://codereview.chromium.org/16871016/diff/46001/cc/scheduler/scheduler.cc...
> cc/scheduler/scheduler.cc:236: TRACE_EVENT0("cc", str.c_str());
> Can we trace strings like this nowadays? I remember some trouble earlier
because
> the string got free'd before the trace event was written.

I did run into issues with this method and have removed it.

[brianderson, 2013-07-09 01:50:48]

PTAL.

Still need to fix the scheduler unit tests.
All other unit tests were fixed, but rebasing today broke two more tests and I'm
still trying to figure those out.

Uploading the latest for feedback, particularly on the parts to which I've added
comments.

https://codereview.chromium.org/16871016/diff/52001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/52001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:294: // TODO(brianderson): How to handle
OUTPUT_SURFACE_LOST.
I really need to understand how to handle OUTPUT_SURFACE_LOST if we are waiting
for activation. I'm not very familiar with the output surface creation flow.
Thoughts?

https://codereview.chromium.org/16871016/diff/52001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/52001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:410: scheduler_on_impl_thread_->SetHasTrees(
The meaning of the two arguments to SetHasTrees (has_pending_tree and
active_tree_is_null) are subtly different. Just thought I'd point that out in
case anyone has an opinion here.

https://codereview.chromium.org/16871016/diff/52001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:827: // We defer forced commits while there is a
pending tree here.
I added deferred_start_commit_on_impl_thread_ in order to allow tests with
readbacks / forced commits to work with the new scheduler.

I'm not sure if this is the right thing to do however. Is this good enough for
now? Should I be discarding the pending or active tree so we don't block the
readback as long? Thoughts?

[boliu, 2013-07-09 23:33:26]

A couple more requests for webview: https://codereview.chromium.org/18939003/

Basically need to ensure the deadline is called synchronously and only from
begin frame. Hopefully this will be cleaned after the deadline function is
exposed to output surface.

https://codereview.chromium.org/16871016/diff/52001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/52001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:294: // TODO(brianderson): How to handle
OUTPUT_SURFACE_LOST.
On 2013/07/09 01:50:48, Brian Anderson wrote:
> I really need to understand how to handle OUTPUT_SURFACE_LOST if we are
waiting
> for activation. I'm not very familiar with the output surface creation flow.
> Thoughts?

ACTION_BEGIN_OUTPUT_SURFACE_CREATION post a task to to main thread. When it
runs, it recreates the output surface on main thread, then posts blocks on impl
thread waiting to the clean up the last output surface and the new one is
initialized. It calls Scheduler::DidCreateAndInitializeOutputSurface if it
succeeds.

That's the basic structure. There are additional logic for retrying on failure
and supporting CompositeAndReadback. The old output surface is still present
while waiting for the new one to be initialized.

I think we should return ACTION_BEGIN_OUTPUT_SURFACE_CREATION asap, but I don't
fully understand how everything else is tied together in this patch to really
say.

[brianderson, 2013-07-10 21:58:03]

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:180: if
(settings_.using_synchronous_renderer_compositor)
Bo, does this method fix WebView so it always draws without posting?

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:175: output_surface_state_ !=
OUTPUT_SURFACE_ACTIVE ||
This is how I plan to handle lost output surfaces in case we are waiting for
activation: Always try to activate if the output surface isn't active. Once we
activate, that will unblock the main thread and the commit state will become
COMMIT_STATE_IDLE.  Then we will be able to handle
ACTION_BEGIN_OUTPUT_SURFACE_CREATION.

https://codereview.chromium.org/16871016/diff/66001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1274: if (frame.has_no_damage ||
output_surface_lost_)
I avoid swapping if we don't have an output surface now. Tests were failing
because they would pretend to swap to a lost output surface and get an ack.

https://codereview.chromium.org/16871016/diff/66001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:394: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
This makes sure to update animation state when we can't draw.

[boliu, 2013-07-10 23:39:52]

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:180: if
(settings_.using_synchronous_renderer_compositor)
On 2013/07/10 21:58:04, Brian Anderson wrote:
> Bo, does this method fix WebView so it always draws without posting?

Yep I think so.

So just to confirm I understand this correctly: for webview,
ShouldTriggerBeginFrameDeadlineEarly will never be true in SetHasTrees and
FinishCommit above because we directly call OnBeginFrameDeadline here so we are
skipping over DEADLINE_PENDING state.

[brianderson, 2013-07-11 00:25:42]

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/66001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:180: if
(settings_.using_synchronous_renderer_compositor)
On 2013/07/10 23:39:52, boliu wrote:
> On 2013/07/10 21:58:04, Brian Anderson wrote:
> > Bo, does this method fix WebView so it always draws without posting?
> 
> Yep I think so.
> 
> So just to confirm I understand this correctly: for webview,
> ShouldTriggerBeginFrameDeadlineEarly will never be true in SetHasTrees and
> FinishCommit above because we directly call OnBeginFrameDeadline here so we
are
> skipping over DEADLINE_PENDING state.

Yes. BeginFrame will never exit in the DEADLINE_PENDING state for WebView.

[brianderson, 2013-07-12 02:06:05]

In working on the scheduler unit tests, and had a few questions reagarding
readbacks:

For my education: What are all the ways readbacks can be initiated from Blink?
Is glReadPixels one of those ways?

For the purposes of this patch: Do we have to prevent the tree committed by
ThreadProxy::CompositeAndReadback from ever being displayed on screen? If so,
where do we currently do that? If not, would it be acceptable for the scheduler
to block the readback until the next BeginFrame deadline to prevent the readback
from increasing latency?

[piman, 2013-07-12 02:41:09]

On Thu, Jul 11, 2013 at 7:06 PM, <brianderson@chromium.org> wrote:

> In working on the scheduler unit tests, and had a few questions reagarding
> readbacks:
>
> For my education: What are all the ways readbacks can be initiated from
> Blink?
> Is glReadPixels one of those ways?
>

https://code.google.com/p/chromium/issues/detail?id=251936 (and its
dependencies) tracks all the places that call (indirectly)
CompositeAndReadback.

For the purposes of this patch: Do we have to prevent the tree committed by
> ThreadProxy::**CompositeAndReadback from ever being displayed on screen?


Some of the paths try to grab a snapshot at a given size, and what they do
is resize the compositor (the LTH), CompositeAndReadback (which doesn't
swap), and resize it back. See RenderWidget::OnPaintAtSize. We obviously
don't want to flash the "wrong" size.


> If so,
> where do we currently do that?


In ThreadProxy::ScheduledActionDrawAndSwapInternal we check if it's a
readback request, and if it is we don't swap.


> If not, would it be acceptable for the scheduler
> to block the readback until the next BeginFrame deadline to prevent the
> readback
> from increasing latency?


Note: CompositeAndReadback can be called even if the LTH is invisible and
we don't tick.

>
>
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[danakj, 2013-07-12 15:33:33]

On Thu, Jul 11, 2013 at 7:06 PM, <brianderson@chromium.org> wrote:

> In working on the scheduler unit tests, and had a few questions reagarding
> readbacks:
>
> For my education: What are all the ways readbacks can be initiated from
> Blink?
> Is glReadPixels one of those ways?
>

Blink does not have access to the compositor context, so it can't call
glReadPixels on anything directly other than webgl stuff.

Compositor has two ways to get pixels, the new way and the deprecated way.

1) CopyOutputRequest is the new way. This is an asynchronous copy request
performed during the next draw to the screen.

2) CopmpositeAndReadback is the old way. It is a synchronous call from the
main thread that forces the compositor to immediate commit, damage the
readback rect, draw, and read pixels out the readback rect. This path does
not do a swap buffers. ThreadProxy does not initiate a swap if the
ScheduledDrawAndSwap is determined to be for a readback.


>
> For the purposes of this patch: Do we have to prevent the tree committed by
> ThreadProxy::**CompositeAndReadback from ever being displayed on screen?
> If so,
> where do we currently do that? If not, would it be acceptable for the
> scheduler
> to block the readback until the next BeginFrame deadline to prevent the
> readback
> from increasing latency?
>

CompositeAndReadback is synchronous, and must have pixels in the buffer
when it returns on the main thread. It's the hugest spaghetti to make it
work and greatly complicates the scheduler and everything else. So the
number of bugs caused by it shouldn't be too surprising. I think you'll
have to be incredibly careful to change it and not break things.

However, there is good news:
https://code.google.com/p/chromium/issues/detail?id=251936 We want to
remove it entirely and we have bugs tracking each of the places that uses
it, to replace it with CopyOutputRequest instead.


>
>
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[brianderson, 2013-07-12 18:15:34]

Thanks for the background @piman and @danakj!

> Blink does not have access to the compositor context, so it can't call
glReadPixels on anything directly other than webgl stuff.

Does this mean there will never be an "incomplete" commit? My concern is that
someone might render half the frame, request a readback, render the rest, and
then want to swap; in which case, we would end up with bad commit temporarily on
the active tree. Although we do not swap on a readback request, it seems like an
animation or fast touch path could cause us to swap with the incomplete tree on
the next BeginFrame.

[danakj, 2013-07-12 18:19:16]

On Fri, Jul 12, 2013 at 11:15 AM, <brianderson@chromium.org> wrote:

> Thanks for the background @piman and @danakj!
>
>
>  Blink does not have access to the compositor context, so it can't call
>>
> glReadPixels on anything directly other than webgl stuff.
>
> Does this mean there will never be an "incomplete" commit? My concern is
> that
> someone might render half the frame, request a readback, render the rest,
> and
> then want to swap; in which case, we would end up with bad commit
> temporarily on
> the active tree. Although we do not swap on a readback request, it seems
> like an
> animation or fast touch path could cause us to swap with the incomplete
> tree on
> the next BeginFrame.
>

CompositeAndReadback() path does a BeginFrameAndCommit which does
UpdateLayers, Commit, PushProperties, Activate, Draw.

I'm not sure what you mean by "render half the frame" here. Who is the
someone? Compositor will do paint/raster/upload/draw as required for the
frame.

On the CopyOutputRequest path, similarly, the compositor will only draw
when its ready to draw, and then it'll just make a copy of what it drew
afterward.



>
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[piman, 2013-07-12 18:29:57]

On Fri, Jul 12, 2013 at 11:15 AM, <brianderson@chromium.org> wrote:

> Thanks for the background @piman and @danakj!
>
>
>  Blink does not have access to the compositor context, so it can't call
>>
> glReadPixels on anything directly other than webgl stuff.
>
> Does this mean there will never be an "incomplete" commit? My concern is
> that
> someone might render half the frame, request a readback, render the rest,
> and
> then want to swap; in which case, we would end up with bad commit
> temporarily on
> the active tree. Although we do not swap on a readback request, it seems
> like an
> animation or fast touch path could cause us to swap with the incomplete
> tree on
> the next BeginFrame.
>

In case that's the source of your confusion: for readback, the compositor
does a full draw and then reads back (readPixels), it's not the client that
issues the readPixels at arbitrary times.

Antoine


>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[brianderson, 2013-07-17 02:31:00]

The existing SchedulerStateMachine unit tests are passing now.

https://codereview.chromium.org/16871016/diff/90001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/90001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:60: enum SynchronousReadbackState {
Would like feedback on new readback states. It was the only way I could get
things to work without deferring commits from the main thread.

[brianderson, 2013-07-18 02:02:48]

PTAL. All existing tests have been fixed. I still need to add additional unit
tests to improve coverage though.

https://codereview.chromium.org/16871016/diff/111001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16871016/diff/111001/cc/output/output_surface...
cc/output/output_surface.cc:221: base::TimeTicks
OutputSurface::RetroactiveBeginFrameDeadline() {
Using this RetroactiveBeginFrameDeadline instead of RetroactiveBeginFramePeriod
made testing easier.

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.cc:104: timebase = base::TimeTicks();
This is a hack to keep the phase from changing every few frames. I will remove
it before committing. See
https://code.google.com/p/chromium/issues/detail?id=252087

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (left):

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:84: void SetNeedsForcedRedraw();
I got rid of this method because the state machine internally decides when to
trigger a forced draw as the readback state advances.

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_settings.h (right):

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_settings.h:21: bool
use_begin_frame_workaround_for_crbug_249806;
I added this to avoid the workaround being enabled by default for the unit
tests. I will make sure to add tests before committing that does test the
workaround.

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:60: enum SynchronousReadbackState {
This state is used to make sure:
1) We do not perform the readback until the readback commit has been activated.
2) We do not swap the commit associated with the readback to screen.
3) We know when it is safe to swap again. (The readback commit has been replaced
by a new commit on the active tree.)

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:96: ACTION_DRAW_AND_SWAP_FORCED,
We still keep the forced draw for when we have skipped too many frames because
of checkerboarding. The concept has been mostly decoupled from the forced draw
associated with readback.

https://codereview.chromium.org/16871016/diff/111001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:97: ACTION_DRAW_AND_READBACK,
The readback no longer swaps.

https://codereview.chromium.org/16871016/diff/111001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (left):

https://codereview.chromium.org/16871016/diff/111001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:143:
base::Bind(&ThreadProxy::RequestReadbackOnImplThread,
This method has been made part of ForceCommitForReadbackOnImplThread to avoid
race conditions where we might commit and draw for readback before the Impl
thread has the readback request.

https://codereview.chromium.org/16871016/diff/111001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/16871016/diff/111001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:829: // We defer forced commits while there is a
pending tree here.
This code is no longer needed to make composite and readback work, but it could
help us overlap our recording and playback SkPictures as part of the phase 2
items.

[brianderson, 2013-07-24 00:06:31]

Cleanup patch. Needs a rebase though.

New tests still pending. One area that is definitely missing coverage is
interaction between simultaneous Readback + ForcedRedraw.

https://codereview.chromium.org/16871016/diff/125001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/16871016/diff/125001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:56: COMMIT_STATE_WAITING_FOR_FIRST_DRAW,
The SynchronousReadbackState and ForcedRedrawOnTimeoutState makes
COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW redundant. Removing it makes the code
cleaner.

https://codereview.chromium.org/16871016/diff/125001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:96: ACTION_DRAW_AND_SWAP_ABORT,
I've added an explicit action to abort the draw and swap. There are a couple
cases (output surface lost, draw suspended until commit) where we need to go
through the motions of drawing to unblock the main thread.

Previously, this would be done without necessarily advancing the state machine. 
Adding an explicit DRAW_AND_SWAP_ABORT allows us to always advance to
COMMIT_STATE_IDLE and restrict more actions to only occur in the idle state.

https://codereview.chromium.org/16871016/diff/125001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:230: bool active_tree_has_been_drawn_;
I've merged the functionality of commit_tree_has_been_drawn_ into
active_tree_has_been_drawn_.

[nduca, 2013-07-25 00:10:14]

LGTM to land when you're ready, though if you can give a quick thought to
simplifying the readback states, that'd be cool. Lets file a followup bug to
fuse the scheduler and scheduler state machines, they're causing us more pain
than saving.

[brianderson, 2013-07-25 17:57:23]

cc+=Jared

[brianderson, 2013-08-02 02:29:50]

I'm fixing several things not related to this patch, but doing them separately
will be hard to do without putting the tree in a bad state temporarily...

The latest patch has the scheduler completely controlling activation now to
avoid a second activation before the active tree has been drawn. Fixing this
exposed a couple bugs requiring the following additional fixes, which are also
in the latest patch:

* OutputSurface initialization no longer triggers a draw until after activation
if we are impl-side painting.
* The page-scroll delta is now synchronized properly when there is a pending
tree.

[enne (OOO), 2013-08-02 19:41:24]

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:843: virtual void DidCommit() OVERRIDE {
This test seems like it depends on PrepareToDrawOnThread resetting used
textures, and changing this function to DidCommit might make it flaky.

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:966: virtual void DidCommit() OVERRIDE {
There's test logic in PrepareToDrawOnThread that seems like it could be skipped
if you make this change?

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:274: } while (action !=
SchedulerStateMachine::ACTION_NONE);
This is a much-needed scheduler change.

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:260: // In these cases, we want to
activate even if the pending tree isn't ready
This looks really good.  This is what I was trying to get at with the TODO(enne)
in this file and am kind of surprised it hasn't bitten us sooner.

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:272: 
This comment doesn't really add much, and I don't understand why you wouldn't
want to activate here.  Can you explain this more?

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:277: // We do not want to activate a
second tree before drawing the first one.
How is that possible? Maybe I'm misreading other parts of this patch, but
previous we wouldn't send a new begin frame message to the main thread to start
a commit until we'd drawn from the previous commit.  And you can't activate
until you've started another commit.

It seems like that's still the case given that you check for COMMIT_STATE_IDLE
in SchedulerStateMachine::ShouldSendBeginFrameToMainThread().  Am I missing
something?

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:278: return active_tree_has_been_drawn_
|| active_tree_is_null_;
There are other reasons that the active tree might not have drawn (e.g.
everything in CanDraw).  It seems like maybe active_tree_is_null_ in this check
should be replaced by !can_draw_?

[brianderson, 2013-08-02 23:04:30]

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:843: virtual void DidCommit() OVERRIDE {
On 2013/08/02 19:41:25, enne wrote:
> This test seems like it depends on PrepareToDrawOnThread resetting used
> textures, and changing this function to DidCommit might make it flaky.

I changed a number of DidCommitAndDrawFrame's to DidCommit's because delaying
the first draw after OutputSurfaceInitialization till after activation
introduced race conditions in existing tests that depended on the
source_frame_number on the Main thread. I got a little too trigger happy here.

https://codereview.chromium.org/16871016/diff/140001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:966: virtual void DidCommit() OVERRIDE {
On 2013/08/02 19:41:25, enne wrote:
> There's test logic in PrepareToDrawOnThread that seems like it could be
skipped
> if you make this change?
Trigger happy again. Fixed.

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:272: 
On 2013/08/02 19:41:25, enne wrote:
> This comment doesn't really add much, and I don't understand why you wouldn't
> want to activate here.  Can you explain this more?

Good catch! This needlessly delays activation. I probably would have caught this
myself if I tried writing a better comment in the first place. =)

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:277: // We do not want to activate a
second tree before drawing the first one.
On 2013/08/02 19:41:25, enne wrote:
> How is that possible? Maybe I'm misreading other parts of this patch, but
> previous we wouldn't send a new begin frame message to the main thread to
start
> a commit until we'd drawn from the previous commit.  And you can't activate
> until you've started another commit.
> 
> It seems like that's still the case given that you check for COMMIT_STATE_IDLE
> in SchedulerStateMachine::ShouldSendBeginFrameToMainThread().  Am I missing
> something?

In this patch, the draw no longer has to occur before we send the begin frame to
the main thread.  When impl-side-painting, there is a new
COMMIT_STATE_WAITING_FOR_ACTIVATION that transitions to COMMIT_STATE_IDLE on
activation rather than to COMMIT_STATE_WAITING_FOR_FIRST_DRAW. So it is now
possible for us to start (and finish) another commit before we've drawn the
previous one.

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:278: return active_tree_has_been_drawn_
|| active_tree_is_null_;
On 2013/08/02 19:41:25, enne wrote:
> There are other reasons that the active tree might not have drawn (e.g.
> everything in CanDraw).  It seems like maybe active_tree_is_null_ in this
check
> should be replaced by !can_draw_?

In cases where can_draw_ is false, ACTION_DRAW_AND_SWAP_ABORT will set
active_tree_has_been_drawn_ to true, which will in turn unblock activation. I
will add a comment to explain.

At one point I was using can_draw_ to unblock activation, but that resulted in
checkerboarding when switching to a new tab.

[enne (OOO), 2013-08-05 19:37:42]

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:278: return active_tree_has_been_drawn_
|| active_tree_is_null_;
On 2013/08/02 23:04:31, Brian Anderson wrote:
> On 2013/08/02 19:41:25, enne wrote:
> > There are other reasons that the active tree might not have drawn (e.g.
> > everything in CanDraw).  It seems like maybe active_tree_is_null_ in this
> check
> > should be replaced by !can_draw_?
> 
> In cases where can_draw_ is false, ACTION_DRAW_AND_SWAP_ABORT will set
> active_tree_has_been_drawn_ to true, which will in turn unblock activation. I
> will add a comment to explain.
> 
> At one point I was using can_draw_ to unblock activation, but that resulted in
> checkerboarding when switching to a new tab.

In that case, shouldn't that logic to set active_tree_has_been_drawn to true
when it can't be drawn mean that you don't need the active_tree_is_null clause
here?

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1516: virtual void DidCommitAndDrawFrame()
OVERRIDE {
I want to look through these test changes a little bit more closely, but I
wonder if DidCommitAndDrawFrame should just get removed.  It seems like a hazard
that all of these tests are checking the main thread here, expecting that no
other commit has happened.

[brianderson, 2013-08-05 21:56:18]

A note about the latest patch: The TabCaptureApiTest.ActiveTabPermission test
somehow triggers back to back readbacks before the replacement commit comes in,
which was not anticipated in the previous patch. Conveniently, the code already
could handle back to back readbacks and I just needed to relax a DCHECK.

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/140001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:278: return active_tree_has_been_drawn_
|| active_tree_is_null_;
On 2013/08/05 19:37:43, enne wrote:
> On 2013/08/02 23:04:31, Brian Anderson wrote:
> > On 2013/08/02 19:41:25, enne wrote:
> > > There are other reasons that the active tree might not have drawn (e.g.
> > > everything in CanDraw).  It seems like maybe active_tree_is_null_ in this
> > check
> > > should be replaced by !can_draw_?
> > 
> > In cases where can_draw_ is false, ACTION_DRAW_AND_SWAP_ABORT will set
> > active_tree_has_been_drawn_ to true, which will in turn unblock activation.
I
> > will add a comment to explain.
> > 
> > At one point I was using can_draw_ to unblock activation, but that resulted
in
> > checkerboarding when switching to a new tab.
> 
> In that case, shouldn't that logic to set active_tree_has_been_drawn to true
> when it can't be drawn mean that you don't need the active_tree_is_null clause
> here?

As it is currently coded, ShouldDraw() will not return true when
active_tree_is_null_, which is why the active_tree_is_null_ clause is needed
here.

I just tried removing the checks for active_tree_is_null_ in ShouldDraw() and
ShouldActivatePendingTree(). It appears to work and results in less code.
Conceptually though, it seems strange to try to draw when there is no active
tree. The intention of the ACTION_DRAW_AND_SWAP_ABORT was to unblock a draw that
was initiated but could never happen.

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1516: virtual void DidCommitAndDrawFrame()
OVERRIDE {
On 2013/08/05 19:37:43, enne wrote:
> I want to look through these test changes a little bit more closely, but I
> wonder if DidCommitAndDrawFrame should just get removed.  It seems like a
hazard
> that all of these tests are checking the main thread here, expecting that no
> other commit has happened.

DidCommitAndDrawFrame is definitely race prone when we are impl-side-painting. I
found that most tests actually just want to wait for their commit to finish.

If it is possible to transform all the tests that actually need to trigger
actions after draw to perform their actions on the Impl thread when the draw
occurs rather than on the Main thread, that would be awesome.

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:225: int index_impl_;
I need to revert my changes to this test.

https://codereview.chromium.org/16871016/diff/172001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:857: case 4:
I will remove case 3 and 4 here. They were an experiment I forgot to remove.

[enne (OOO), 2013-08-06 23:35:15]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
It seems like this is not testing what it should be.  The test seems to be
waiting for the TextureLayerImpl to be deleted first as a result of the first
commit.  However, this won't be the case right after the second commit; it'll
only happen after the second activation?

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:227: !active_tree_is_null_));
I think my confusion about check for the active_tree_is_null vs. can_draw in
ShouldActivatePendingTree comes from this line.

It seems like !active_tree_is_null should be !can_draw?

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:276: return active_tree_has_been_drawn_
|| active_tree_is_null_;
CanDraw already includes a null root layer check
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...).
 It seems like for the null root layer (like in other examples where can_draw is
false), the scheduler state machine will get a DRAW_AND_SWAP_ABORT and go
through this path.

I just still don't understand why active_tree_is_null has to get called out here
specially but other can_draw triggers don't.  It seems like any of the other
can_draw triggers will also cause ShouldDraw to return false because ShouldDraw
checks DrawSuspendedUntilCommit which checks can_draw.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:255: if (!ManageTiles())
It seems like TileManager should be able to activate here via
TileManagerClient::NotifyReadyToActivate without additional logic.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
This false seems a little sketchy.  If you call ManageTiles and then immediately
call ManageTiles again, it will return false, but that doesn't mean the pending
tree can be activated.

I wonder if this would be conceptually cleaner if ManageTiles didn't return
anything.  The one caller that checks the return value is also guarded by an
impl-side painting setting which makes the tile_manager_ check redundant.  The
one caller is also right after an UpdateDrawProperties call which will always
indirectly call WillModifyTilePriorities.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2205:
pending_tree_->set_sent_page_scale_delta(scroll_info->page_scale_delta);
Sent page scale delta should always be 1 on the pending tree as it has not sent
anything.  I think this will especially not work with your changes to pass the
sent page scale delta from pending to active tree during activaiton.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (left):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:200: pending_tree->SetPageScaleDelta(
This is what keeps the pending tree's total page scale factor up to date during
a pinch gesture.

You've added it to AnimatePinchGesture, but not all pinches are animated. 
There's also this one:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Is there a reason to change this?

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:115:
target_tree->set_sent_page_scale_delta(sent_page_scale_delta());
If you're going to change this code, can you DCHECK that sent page scale delta
here is 1?

[brianderson, 2013-08-07 01:55:50]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/06 23:35:16, enne wrote:
> It seems like this is not testing what it should be.  The test seems to be
> waiting for the TextureLayerImpl to be deleted first as a result of the first
> commit.  However, this won't be the case right after the second commit; it'll
> only happen after the second activation?

I changed this so that DidActivateTreeOnThread posts a task to the main thread
to delete the TextureLayer. Should that work?

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:227: !active_tree_is_null_));
On 2013/08/06 23:35:16, enne wrote:
> I think my confusion about check for the active_tree_is_null vs. can_draw in
> ShouldActivatePendingTree comes from this line.
> 
> It seems like !active_tree_is_null should be !can_draw?

DrawSuspendedUntilCommit takes into account !can_draw for all cases. Only during
output surface initialization do we avoid aborting draws if the part of the
reason we cant draw is that the active tree is null.

Removing the active_tree_is_null check in that case results in aborted draws
before our first commit/activation, which looks funny and results in weird test
expectations.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:255: if (!ManageTiles())
On 2013/08/06 23:35:16, enne wrote:
> It seems like TileManager should be able to activate here via
> TileManagerClient::NotifyReadyToActivate without additional logic.

In this patch, I avoid back to back activations by having the Scheduler
explicitly initiate activation. This code eventually notifies the Scheduler
which keeps track of whether or not the current active tree has been drawn.  If
the current active tree hasn't been drawn, it will not activate the pending tree
until after the draw.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
On 2013/08/06 23:35:16, enne wrote:
> This false seems a little sketchy.  If you call ManageTiles and then
immediately
> call ManageTiles again, it will return false, but that doesn't mean the
pending
> tree can be activated.
> 
> I wonder if this would be conceptually cleaner if ManageTiles didn't return
> anything.  The one caller that checks the return value is also guarded by an
> impl-side painting setting which makes the tile_manager_ check redundant.  The
> one caller is also right after an UpdateDrawProperties call which will always
> indirectly call WillModifyTilePriorities.

Ok. It seems like I need to fix some of the corner cases here.

If I do not return a value from this function to notify the Scheduler that we
are ready to activate immediately, then there are cases where
manage_tiles_needed_ is still false even after the call to UpdateDrawProperties.
 I think this is because of layers that aren't tiled (such as texture layers)
and when we are rasterizing on demand.

Is there a better way to check for those cases where we are not expecting a
NotifyReadyToActivate from the TileManager and can activate immediately.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2205:
pending_tree_->set_sent_page_scale_delta(scroll_info->page_scale_delta);
On 2013/08/06 23:35:16, enne wrote:
> Sent page scale delta should always be 1 on the pending tree as it has not
sent
> anything.  I think this will especially not work with your changes to pass the
> sent page scale delta from pending to active tree during activaiton.

What prompted the page scale delta changes was that my changes (either back to
back activates or delaying the first draw after a new output surface until after
activation) were causing LayerTreeHostTestStartPageScaleAnimation to fail. I
think the best course of action will be to undo my changes regarding the page
scale and debug with Enne what's going wrong and the best way to fix it.

[brianderson, 2013-08-07 03:05:11]

I undid the page scale animation changes and the
LayerTreeHostTestStartPageScaleAnimation test still passes.

[reveman, 2013-08-07 03:32:28]

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1424: void
LayerTreeHostImpl::ActivatePendingTreeIfNeeded() {
can this function be removed?

[enne (OOO), 2013-08-07 15:44:19]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/07 01:55:51, Brian Anderson wrote:
> On 2013/08/06 23:35:16, enne wrote:
> > It seems like this is not testing what it should be.  The test seems to be
> > waiting for the TextureLayerImpl to be deleted first as a result of the
first
> > commit.  However, this won't be the case right after the second commit;
it'll
> > only happen after the second activation?
> 
> I changed this so that DidActivateTreeOnThread posts a task to the main thread
> to delete the TextureLayer. Should that work?
> 

Yeah, that seems like the right approach.

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:227: !active_tree_is_null_));
On 2013/08/07 01:55:51, Brian Anderson wrote:
> On 2013/08/06 23:35:16, enne wrote:
> > I think my confusion about check for the active_tree_is_null vs. can_draw in
> > ShouldActivatePendingTree comes from this line.
> > 
> > It seems like !active_tree_is_null should be !can_draw?
> 
> DrawSuspendedUntilCommit takes into account !can_draw for all cases. Only
during
> output surface initialization do we avoid aborting draws if the part of the
> reason we cant draw is that the active tree is null.
> 
> Removing the active_tree_is_null check in that case results in aborted draws
> before our first commit/activation, which looks funny and results in weird
test
> expectations.

Thanks for all the explanation.  If it's not necessary, but it makes traces less
confusing, that seems like the right thing to do.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
On 2013/08/07 01:55:51, Brian Anderson wrote:
> On 2013/08/06 23:35:16, enne wrote:
> > This false seems a little sketchy.  If you call ManageTiles and then
> immediately
> > call ManageTiles again, it will return false, but that doesn't mean the
> pending
> > tree can be activated.
> > 
> > I wonder if this would be conceptually cleaner if ManageTiles didn't return
> > anything.  The one caller that checks the return value is also guarded by an
> > impl-side painting setting which makes the tile_manager_ check redundant. 
The
> > one caller is also right after an UpdateDrawProperties call which will
always
> > indirectly call WillModifyTilePriorities.
> 
> Ok. It seems like I need to fix some of the corner cases here.
> 
> If I do not return a value from this function to notify the Scheduler that we
> are ready to activate immediately, then there are cases where
> manage_tiles_needed_ is still false even after the call to
UpdateDrawProperties.
>  I think this is because of layers that aren't tiled (such as texture layers)
> and when we are rasterizing on demand.

Maybe the problem here is just the comment.  ManageTiles returning false does
not mean that you can activate.  That's true during commit, but is not true in
general.  Returning true from ManageTiles here just means that the TileManager
is responsible for notifying about activation.

One option would be to change the comment on ManageTiles.  Alternatively, the
caller of ManageTiles during commit should just check manage_tiles_needed_
directly prior to calling ManageTiles to catch this case, rather than returning
a boolean value any time ManageTiles is called.

[brianderson, 2013-08-07 18:41:56]

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
On 2013/08/07 15:44:19, enne wrote:
> On 2013/08/07 01:55:51, Brian Anderson wrote:
> > On 2013/08/06 23:35:16, enne wrote:
> > > This false seems a little sketchy.  If you call ManageTiles and then
> > immediately
> > > call ManageTiles again, it will return false, but that doesn't mean the
> > pending
> > > tree can be activated.
> > > 
> > > I wonder if this would be conceptually cleaner if ManageTiles didn't
return
> > > anything.  The one caller that checks the return value is also guarded by
an
> > > impl-side painting setting which makes the tile_manager_ check redundant. 
> The
> > > one caller is also right after an UpdateDrawProperties call which will
> always
> > > indirectly call WillModifyTilePriorities.
> > 
> > Ok. It seems like I need to fix some of the corner cases here.
> > 
> > If I do not return a value from this function to notify the Scheduler that
we
> > are ready to activate immediately, then there are cases where
> > manage_tiles_needed_ is still false even after the call to
> UpdateDrawProperties.
> >  I think this is because of layers that aren't tiled (such as texture
layers)
> > and when we are rasterizing on demand.
> 
> Maybe the problem here is just the comment.  ManageTiles returning false does
> not mean that you can activate.  That's true during commit, but is not true in
> general.  Returning true from ManageTiles here just means that the TileManager
> is responsible for notifying about activation.
> 
> One option would be to change the comment on ManageTiles.  Alternatively, the
> caller of ManageTiles during commit should just check manage_tiles_needed_
> directly prior to calling ManageTiles to catch this case, rather than
returning
> a boolean value any time ManageTiles is called.

Thanks for the background. I would like to avoid calling ManageTiles if we don't
need to because of the overhead
(https://code.google.com/p/chromium/issues/detail?id=269272).

Would another option be to revert changes to LayerTreeHostImpl::ManageTiles and
have pending_tree_->UpdateDrawProperties() somehow trigger the ReadyToActivate
if it doesn't set manage_tiles_needed_ to true?

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1424: void
LayerTreeHostImpl::ActivatePendingTreeIfNeeded() {
On 2013/08/07 03:32:28, David Reveman wrote:
> can this function be removed?

It's very close to being removable. LayerTreeHostImplTimeSourceAdapter still
needs it when we we are not visible. Do you mind if we remove it as a separate
patch?

[reveman, 2013-08-07 19:09:38]

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/216001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1424: void
LayerTreeHostImpl::ActivatePendingTreeIfNeeded() {
On 2013/08/07 18:41:57, Brian Anderson wrote:
> On 2013/08/07 03:32:28, David Reveman wrote:
> > can this function be removed?
> 
> It's very close to being removable. LayerTreeHostImplTimeSourceAdapter still
> needs it when we we are not visible. Do you mind if we remove it as a separate
> patch?

That's perfectly fine. I just wanted to make sure we didn't leave dead code
behind.

[piman, 2013-08-07 19:39:52]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/07 15:44:19, enne wrote:
> On 2013/08/07 01:55:51, Brian Anderson wrote:
> > On 2013/08/06 23:35:16, enne wrote:
> > > It seems like this is not testing what it should be.  The test seems to be
> > > waiting for the TextureLayerImpl to be deleted first as a result of the
> first
> > > commit.  However, this won't be the case right after the second commit;
> it'll
> > > only happen after the second activation?
> > 
> > I changed this so that DidActivateTreeOnThread posts a task to the main
thread
> > to delete the TextureLayer. Should that work?
> > 
> 
> Yeah, that seems like the right approach.

I would prefer that we wait for a frame to be drawn. We only create the resource
in WillDraw, and this is really meant to exercise this behavior (if we delete
the layer after the resource was created).

Why can't we keep DidCommitAndDrawFrame ?

[enne (OOO), 2013-08-07 19:48:48]

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
On 2013/08/07 18:41:57, Brian Anderson wrote:
> Thanks for the background. I would like to avoid calling ManageTiles if we
don't
> need to because of the overhead
> (https://code.google.com/p/chromium/issues/detail?id=269272).
> 
> Would another option be to revert changes to LayerTreeHostImpl::ManageTiles
and
> have pending_tree_->UpdateDrawProperties() somehow trigger the ReadyToActivate
> if it doesn't set manage_tiles_needed_ to true?

I wasn't suggesting to move the early out, just to add an additional check in
the commit function.

I think what you suggest could work for now, but I have been thinking about
making UpdateDrawProperties not always update tile priorities which would cause
that check to misfire.

I think it might be clearer to do it in the commit function, since not needing
to manage tiles is sort of a property of the tree, and can only be true once,
and so will always be true in commit when we have to add pending priorities for
all tiles on the pending tree.

[brianderson, 2013-08-07 20:12:28]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/07 19:39:52, piman wrote:
> On 2013/08/07 15:44:19, enne wrote:
> > On 2013/08/07 01:55:51, Brian Anderson wrote:
> > > On 2013/08/06 23:35:16, enne wrote:
> > > > It seems like this is not testing what it should be.  The test seems to
be
> > > > waiting for the TextureLayerImpl to be deleted first as a result of the
> > first
> > > > commit.  However, this won't be the case right after the second commit;
> > it'll
> > > > only happen after the second activation?
> > > 
> > > I changed this so that DidActivateTreeOnThread posts a task to the main
> thread
> > > to delete the TextureLayer. Should that work?
> > > 
> > 
> > Yeah, that seems like the right approach.
> 
> I would prefer that we wait for a frame to be drawn. We only create the
resource
> in WillDraw, and this is really meant to exercise this behavior (if we delete
> the layer after the resource was created).
> 
> Why can't we keep DidCommitAndDrawFrame ?

It's possible that DidCommitAndDrawFrame doesn't get called for the first time
until after two commits have occurred, so we end up not seeing all values of
source_frame_number() in DidCommitAndDrawFrame sometimes.

If we want to verify that a particular source_frame_number was activated or
drawn, we have to check that on the Impl thread.

Maybe it would help if DidCommitAndDrawFrame took the source_frame_number as an
argument that the Impl thread could communicate to the main thread?

For the purposes of this patch, however, my current plan is to post the task to
the main thread to delete the Texture layer from SwapBuffersCompleteOnThread
instead of DidActivateTreeOnThread as originally planned.

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:324: return false;
On 2013/08/07 19:48:48, enne wrote:
> On 2013/08/07 18:41:57, Brian Anderson wrote:
> > Thanks for the background. I would like to avoid calling ManageTiles if we
> don't
> > need to because of the overhead
> > (https://code.google.com/p/chromium/issues/detail?id=269272).
> > 
> > Would another option be to revert changes to LayerTreeHostImpl::ManageTiles
> and
> > have pending_tree_->UpdateDrawProperties() somehow trigger the
ReadyToActivate
> > if it doesn't set manage_tiles_needed_ to true?
> 
> I wasn't suggesting to move the early out, just to add an additional check in
> the commit function.
> 
> I think what you suggest could work for now, but I have been thinking about
> making UpdateDrawProperties not always update tile priorities which would
cause
> that check to misfire.
> 
> I think it might be clearer to do it in the commit function, since not needing
> to manage tiles is sort of a property of the tree, and can only be true once,
> and so will always be true in commit when we have to add pending priorities
for
> all tiles on the pending tree.

Ah, I understand your original comment now and it makes a lot of sense. Checking
in the commit works very well.

[piman, 2013-08-07 20:27:45]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/07 20:12:29, Brian Anderson wrote:
> On 2013/08/07 19:39:52, piman wrote:
> > On 2013/08/07 15:44:19, enne wrote:
> > > On 2013/08/07 01:55:51, Brian Anderson wrote:
> > > > On 2013/08/06 23:35:16, enne wrote:
> > > > > It seems like this is not testing what it should be.  The test seems
to
> be
> > > > > waiting for the TextureLayerImpl to be deleted first as a result of
the
> > > first
> > > > > commit.  However, this won't be the case right after the second
commit;
> > > it'll
> > > > > only happen after the second activation?
> > > > 
> > > > I changed this so that DidActivateTreeOnThread posts a task to the main
> > thread
> > > > to delete the TextureLayer. Should that work?
> > > > 
> > > 
> > > Yeah, that seems like the right approach.
> > 
> > I would prefer that we wait for a frame to be drawn. We only create the
> resource
> > in WillDraw, and this is really meant to exercise this behavior (if we
delete
> > the layer after the resource was created).
> > 
> > Why can't we keep DidCommitAndDrawFrame ?
> 
> It's possible that DidCommitAndDrawFrame doesn't get called for the first time
> until after two commits have occurred, so we end up not seeing all values of
> source_frame_number() in DidCommitAndDrawFrame sometimes.
> 
> If we want to verify that a particular source_frame_number was activated or
> drawn, we have to check that on the Impl thread.
> 
> Maybe it would help if DidCommitAndDrawFrame took the source_frame_number as
an
> argument that the Impl thread could communicate to the main thread?
> 
> For the purposes of this patch, however, my current plan is to post the task
to
> the main thread to delete the Texture layer from SwapBuffersCompleteOnThread
> instead of DidActivateTreeOnThread as originally planned.

You want to make sure you wait for the commit *and* the frame that corresponds
to that commit (i.e. not one before that).

I'm surprised we now see DidCommitAndDrawFrame being called after the next
commit (that increases the source frame number) happened. I suspect some amount
of synchronization depends on the current behavior.

Doesn't that mean that we have 2 commits in flight now? Isn't that a problem
(increasing memory load)?

[brianderson, 2013-08-07 20:33:00]

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:1093: virtual void DidCommit() OVERRIDE {
On 2013/08/07 20:27:45, piman wrote:
> On 2013/08/07 20:12:29, Brian Anderson wrote:
> > On 2013/08/07 19:39:52, piman wrote:
> > > On 2013/08/07 15:44:19, enne wrote:
> > > > On 2013/08/07 01:55:51, Brian Anderson wrote:
> > > > > On 2013/08/06 23:35:16, enne wrote:
> > > > > > It seems like this is not testing what it should be.  The test seems
> to
> > be
> > > > > > waiting for the TextureLayerImpl to be deleted first as a result of
> the
> > > > first
> > > > > > commit.  However, this won't be the case right after the second
> commit;
> > > > it'll
> > > > > > only happen after the second activation?
> > > > > 
> > > > > I changed this so that DidActivateTreeOnThread posts a task to the
main
> > > thread
> > > > > to delete the TextureLayer. Should that work?
> > > > > 
> > > > 
> > > > Yeah, that seems like the right approach.
> > > 
> > > I would prefer that we wait for a frame to be drawn. We only create the
> > resource
> > > in WillDraw, and this is really meant to exercise this behavior (if we
> delete
> > > the layer after the resource was created).
> > > 
> > > Why can't we keep DidCommitAndDrawFrame ?
> > 
> > It's possible that DidCommitAndDrawFrame doesn't get called for the first
time
> > until after two commits have occurred, so we end up not seeing all values of
> > source_frame_number() in DidCommitAndDrawFrame sometimes.
> > 
> > If we want to verify that a particular source_frame_number was activated or
> > drawn, we have to check that on the Impl thread.
> > 
> > Maybe it would help if DidCommitAndDrawFrame took the source_frame_number as
> an
> > argument that the Impl thread could communicate to the main thread?
> > 
> > For the purposes of this patch, however, my current plan is to post the task
> to
> > the main thread to delete the Texture layer from SwapBuffersCompleteOnThread
> > instead of DidActivateTreeOnThread as originally planned.
> 
> You want to make sure you wait for the commit *and* the frame that corresponds
> to that commit (i.e. not one before that).
> 
> I'm surprised we now see DidCommitAndDrawFrame being called after the next
> commit (that increases the source frame number) happened. I suspect some
amount
> of synchronization depends on the current behavior.
> 
> Doesn't that mean that we have 2 commits in flight now? Isn't that a problem
> (increasing memory load)?

This patch doesn't allow 2 commits in flight, but it does allow a second commit
to start before the previous commit has drawn.  Previously, the commit was held
until after the draw.

[piman, 2013-08-07 20:41:51]

On Wed, Aug 7, 2013 at 1:33 PM, <brianderson@chromium.org> wrote:

>
> https://codereview.chromium.**org/16871016/diff/188001/cc/**
>
layers/texture_layer_unittest.**cc<https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_unittest.cc>
> File cc/layers/texture_layer_**unittest.cc (right):
>
> https://codereview.chromium.**org/16871016/diff/188001/cc/**
>
layers/texture_layer_unittest.**cc#newcode1093<https://codereview.chromium.org/16871016/diff/188001/cc/layers/texture_layer_unittest.cc#newcode1093>
> cc/layers/texture_layer_**unittest.cc:1093: virtual void DidCommit()
> OVERRIDE {
> On 2013/08/07 20:27:45, piman wrote:
>
>> On 2013/08/07 20:12:29, Brian Anderson wrote:
>> > On 2013/08/07 19:39:52, piman wrote:
>> > > On 2013/08/07 15:44:19, enne wrote:
>> > > > On 2013/08/07 01:55:51, Brian Anderson wrote:
>> > > > > On 2013/08/06 23:35:16, enne wrote:
>> > > > > > It seems like this is not testing what it should be.  The
>>
> test seems
>
>> to
>> > be
>> > > > > > waiting for the TextureLayerImpl to be deleted first as a
>>
> result of
>
>> the
>> > > > first
>> > > > > > commit.  However, this won't be the case right after the
>>
> second
>
>> commit;
>> > > > it'll
>> > > > > > only happen after the second activation?
>> > > > >
>> > > > > I changed this so that DidActivateTreeOnThread posts a task to
>>
> the main
>
>> > > thread
>> > > > > to delete the TextureLayer. Should that work?
>> > > > >
>> > > >
>> > > > Yeah, that seems like the right approach.
>> > >
>> > > I would prefer that we wait for a frame to be drawn. We only
>>
> create the
>
>> > resource
>> > > in WillDraw, and this is really meant to exercise this behavior
>>
> (if we
>
>> delete
>> > > the layer after the resource was created).
>> > >
>> > > Why can't we keep DidCommitAndDrawFrame ?
>> >
>> > It's possible that DidCommitAndDrawFrame doesn't get called for the
>>
> first time
>
>> > until after two commits have occurred, so we end up not seeing all
>>
> values of
>
>> > source_frame_number() in DidCommitAndDrawFrame sometimes.
>> >
>> > If we want to verify that a particular source_frame_number was
>>
> activated or
>
>> > drawn, we have to check that on the Impl thread.
>> >
>> > Maybe it would help if DidCommitAndDrawFrame took the
>>
> source_frame_number as
>
>> an
>> > argument that the Impl thread could communicate to the main thread?
>> >
>> > For the purposes of this patch, however, my current plan is to post
>>
> the task
>
>> to
>> > the main thread to delete the Texture layer from
>>
> SwapBuffersCompleteOnThread
>
>> > instead of DidActivateTreeOnThread as originally planned.
>>
>
>  You want to make sure you wait for the commit *and* the frame that
>>
> corresponds
>
>> to that commit (i.e. not one before that).
>>
>
>  I'm surprised we now see DidCommitAndDrawFrame being called after the
>>
> next
>
>> commit (that increases the source frame number) happened. I suspect
>>
> some amount
>
>> of synchronization depends on the current behavior.
>>
>
>  Doesn't that mean that we have 2 commits in flight now? Isn't that a
>>
> problem
>
>> (increasing memory load)?
>>
>
> This patch doesn't allow 2 commits in flight, but it does allow a second
> commit to start before the previous commit has drawn.  Previously, the
> commit was held until after the draw.
>

Right, that's 2 commits in flight.
You need to keep around resources for commit 1 (that hasn't drawn yet) and
commit 2 (that hasn't drawn either).


>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[brianderson, 2013-08-07 21:34:23]

> >
> >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
> >>
> > problem
> >
> >> (increasing memory load)?
> >>
> >
> > This patch doesn't allow 2 commits in flight, but it does allow a second
> > commit to start before the previous commit has drawn.  Previously, the
> > commit was held until after the draw.
> >
> 
> Right, that's 2 commits in flight.
> You need to keep around resources for commit 1 (that hasn't drawn yet) and
> commit 2 (that hasn't drawn either).

I did some more digging and it looks like, if there is a TextureLayer, commit 2
will still be blocked by commit 1's draw in this patch because of
ThreadProxy::texture_acquisition_completion_event_on_impl_thread_. In which
case, there is no increased memory pressure and the changes to this test
shouldn't be required.

So my comment that commit 2 can start before commit 1 has drawn only applies to
certain cases. PictureLayers would see increased memory pressure, but in a more
spiky manner than TextureLayers would have (we should see spikes when the
content between the active and pending trees are vastly different).

Is memory pressure from PictureLayers something we should be concerned about?

To be conservative for this patch, if we are impl-side-painting, I could send
the BeginFrame to the main thread for commit 2 before we draw commit 1, but
block the actual commit on the Impl thread until commit 2. That would allow us
to record the Picture earlier but still reduce memory pressure for PictureLayers
because rasterization would be deferred. If not impl-side-painting though, I
would have to delay the BeginFrame to the main thread as well because the main
thread does the rasterization.

[danakj, 2013-08-07 21:38:02]

On Wed, Aug 7, 2013 at 5:34 PM, <brianderson@chromium.org> wrote:

> >
>> >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
>> >>
>> > problem
>> >
>> >> (increasing memory load)?
>> >>
>> >
>> > This patch doesn't allow 2 commits in flight, but it does allow a second
>> > commit to start before the previous commit has drawn.  Previously, the
>> > commit was held until after the draw.
>> >
>>
>
>  Right, that's 2 commits in flight.
>> You need to keep around resources for commit 1 (that hasn't drawn yet) and
>> commit 2 (that hasn't drawn either).
>>
>
> I did some more digging and it looks like, if there is a TextureLayer,
> commit 2
> will still be blocked by commit 1's draw in this patch because of
> ThreadProxy::texture_**acquisition_completion_event_**on_impl_thread_. In
> which
> case, there is no increased memory pressure and the changes to this test
> shouldn't be required.
>
> So my comment that commit 2 can start before commit 1 has drawn only
> applies to
> certain cases. PictureLayers would see increased memory pressure, but in a
> more
> spiky manner than TextureLayers would have (we should see spikes when the
> content between the active and pending trees are vastly different).
>
> Is memory pressure from PictureLayers something we should be concerned
> about?
>
> To be conservative for this patch, if we are impl-side-painting, I could
> send
> the BeginFrame to the main thread for commit 2 before we draw commit 1, but
> block the actual commit on the Impl thread until commit 2. That would
> allow us
> to record the Picture earlier but still reduce memory pressure for
> PictureLayers
> because rasterization would be deferred. If not impl-side-painting though,
> I
> would have to delay the BeginFrame to the main thread as well because the
> main
> thread does the rasterization.
>

This increases latency for that frame though, as it has to be blocked,
maybe it would have produced something different if you executed it closer
to the vsync? </devilsadvocate>

Also re: DidCommitAndDrawFrame needing to worry about 2 commits in flight,
we should only see that in a test if the test schedules a second commit
before the draw happens, because we don't just magically commit for no
reason.


>
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[brianderson, 2013-08-07 21:44:30]

On 2013/08/07 21:34:23, Brian Anderson wrote:
> > >
> > >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
> > >>
> > > problem
> > >
> > >> (increasing memory load)?
> > >>
> > >
> > > This patch doesn't allow 2 commits in flight, but it does allow a second
> > > commit to start before the previous commit has drawn.  Previously, the
> > > commit was held until after the draw.
> > >
> > 
> > Right, that's 2 commits in flight.
> > You need to keep around resources for commit 1 (that hasn't drawn yet) and
> > commit 2 (that hasn't drawn either).
> 
> I did some more digging and it looks like, if there is a TextureLayer, commit
2
> will still be blocked by commit 1's draw in this patch because of
> ThreadProxy::texture_acquisition_completion_event_on_impl_thread_. In which
> case, there is no increased memory pressure and the changes to this test
> shouldn't be required.
> 
> So my comment that commit 2 can start before commit 1 has drawn only applies
to
> certain cases. PictureLayers would see increased memory pressure, but in a
more
> spiky manner than TextureLayers would have (we should see spikes when the
> content between the active and pending trees are vastly different).
> 
> Is memory pressure from PictureLayers something we should be concerned about?
> 
> To be conservative for this patch, if we are impl-side-painting, I could send
> the BeginFrame to the main thread for commit 2 before we draw commit 1, but
> block the actual commit on the Impl thread until commit 2. That would allow us
> to record the Picture earlier but still reduce memory pressure for
PictureLayers
> because rasterization would be deferred. If not impl-side-painting though, I
> would have to delay the BeginFrame to the main thread as well because the main
> thread does the rasterization.

Actually, for PictureLayers the active tree's contents can't be deallocated
until it's been replaced, so memory pressure wouldn't be a reason to defer the
commits until after the draw.

Are there other layer types with special behaviors that I should be considering?

[danakj, 2013-08-07 21:47:49]

On Wed, Aug 7, 2013 at 5:44 PM, <brianderson@chromium.org> wrote:

> On 2013/08/07 21:34:23, Brian Anderson wrote:
>
>> > >
>> > >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
>> > >>
>> > > problem
>> > >
>> > >> (increasing memory load)?
>> > >>
>> > >
>> > > This patch doesn't allow 2 commits in flight, but it does allow a
>> second
>> > > commit to start before the previous commit has drawn.  Previously, the
>> > > commit was held until after the draw.
>> > >
>> >
>> > Right, that's 2 commits in flight.
>> > You need to keep around resources for commit 1 (that hasn't drawn yet)
>> and
>> > commit 2 (that hasn't drawn either).
>>
>
>  I did some more digging and it looks like, if there is a TextureLayer,
>> commit
>>
> 2
>
>> will still be blocked by commit 1's draw in this patch because of
>> ThreadProxy::texture_**acquisition_completion_event_**on_impl_thread_.
>> In which
>> case, there is no increased memory pressure and the changes to this test
>> shouldn't be required.
>>
>
>  So my comment that commit 2 can start before commit 1 has drawn only
>> applies
>>
> to
>
>> certain cases. PictureLayers would see increased memory pressure, but in a
>>
> more
>
>> spiky manner than TextureLayers would have (we should see spikes when the
>> content between the active and pending trees are vastly different).
>>
>
>  Is memory pressure from PictureLayers something we should be concerned
>> about?
>>
>
>  To be conservative for this patch, if we are impl-side-painting, I could
>> send
>> the BeginFrame to the main thread for commit 2 before we draw commit 1,
>> but
>> block the actual commit on the Impl thread until commit 2. That would
>> allow us
>> to record the Picture earlier but still reduce memory pressure for
>>
> PictureLayers
>
>> because rasterization would be deferred. If not impl-side-painting
>> though, I
>> would have to delay the BeginFrame to the main thread as well because the
>> main
>> thread does the rasterization.
>>
>
> Actually, for PictureLayers the active tree's contents can't be deallocated
> until it's been replaced, so memory pressure wouldn't be a reason to defer
> the
> commits until after the draw.
>
> Are there other layer types with special behaviors that I should be
> considering?
>

DelegatedRendererLayer? It's like texture layer but with an arbitrary
number of resources.


>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[piman, 2013-08-07 21:49:04]

On Wed, Aug 7, 2013 at 2:34 PM, <brianderson@chromium.org> wrote:

> >
>> >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
>> >>
>> > problem
>> >
>> >> (increasing memory load)?
>> >>
>> >
>> > This patch doesn't allow 2 commits in flight, but it does allow a second
>> > commit to start before the previous commit has drawn.  Previously, the
>> > commit was held until after the draw.
>> >
>>
>
>  Right, that's 2 commits in flight.
>> You need to keep around resources for commit 1 (that hasn't drawn yet) and
>> commit 2 (that hasn't drawn either).
>>
>
> I did some more digging and it looks like, if there is a TextureLayer,
> commit 2
> will still be blocked by commit 1's draw in this patch because of
> ThreadProxy::texture_**acquisition_completion_event_**on_impl_thread_. In
> which
> case, there is no increased memory pressure and the changes to this test
> shouldn't be required.
>

Well... while the AcquireLayerTextures logic may make this test still work
with your change, that's certainly not a desired side effect. We want to
remove that logic eventually when we have fully switched to mailboxes and
fixed a couple of other bugs.
More generally, outside of this test, there are many places (e.g. Aura,
Android UI) that assume that once a commit has happened with a new texture,
the old one is not in use any more. It sounds like you're breaking that. It
makes me very uncomfortable.


> So my comment that commit 2 can start before commit 1 has drawn only
> applies to
> certain cases.


What are they?


> PictureLayers would see increased memory pressure, but in a more
> spiky manner than TextureLayers would have (we should see spikes when the
> content between the active and pending trees are vastly different).
>
> Is memory pressure from PictureLayers something we should be concerned
> about?


> To be conservative for this patch, if we are impl-side-painting, I could
> send
> the BeginFrame to the main thread for commit 2 before we draw commit 1, but
> block the actual commit on the Impl thread until commit 2. That would
> allow us
> to record the Picture earlier but still reduce memory pressure for
> PictureLayers
> because rasterization would be deferred. If not impl-side-painting though,
> I
> would have to delay the BeginFrame to the main thread as well because the
> main
> thread does the rasterization.
>
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[piman, 2013-08-07 21:51:03]

On Wed, Aug 7, 2013 at 2:47 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Aug 7, 2013 at 5:44 PM, <brianderson@chromium.org> wrote:
>
>> On 2013/08/07 21:34:23, Brian Anderson wrote:
>>
>>> > >
>>> > >  Doesn't that mean that we have 2 commits in flight now? Isn't that a
>>> > >>
>>> > > problem
>>> > >
>>> > >> (increasing memory load)?
>>> > >>
>>> > >
>>> > > This patch doesn't allow 2 commits in flight, but it does allow a
>>> second
>>> > > commit to start before the previous commit has drawn.  Previously,
>>> the
>>> > > commit was held until after the draw.
>>> > >
>>> >
>>> > Right, that's 2 commits in flight.
>>> > You need to keep around resources for commit 1 (that hasn't drawn yet)
>>> and
>>> > commit 2 (that hasn't drawn either).
>>>
>>
>>  I did some more digging and it looks like, if there is a TextureLayer,
>>> commit
>>>
>> 2
>>
>>> will still be blocked by commit 1's draw in this patch because of
>>> ThreadProxy::texture_**acquisition_completion_event_**on_impl_thread_.
>>> In which
>>> case, there is no increased memory pressure and the changes to this test
>>> shouldn't be required.
>>>
>>
>>  So my comment that commit 2 can start before commit 1 has drawn only
>>> applies
>>>
>> to
>>
>>> certain cases. PictureLayers would see increased memory pressure, but in
>>> a
>>>
>> more
>>
>>> spiky manner than TextureLayers would have (we should see spikes when the
>>> content between the active and pending trees are vastly different).
>>>
>>
>>  Is memory pressure from PictureLayers something we should be concerned
>>> about?
>>>
>>
>>  To be conservative for this patch, if we are impl-side-painting, I could
>>> send
>>> the BeginFrame to the main thread for commit 2 before we draw commit 1,
>>> but
>>> block the actual commit on the Impl thread until commit 2. That would
>>> allow us
>>> to record the Picture earlier but still reduce memory pressure for
>>>
>> PictureLayers
>>
>>> because rasterization would be deferred. If not impl-side-painting
>>> though, I
>>> would have to delay the BeginFrame to the main thread as well because
>>> the main
>>> thread does the rasterization.
>>>
>>
>> Actually, for PictureLayers the active tree's contents can't be
>> deallocated
>> until it's been replaced, so memory pressure wouldn't be a reason to
>> defer the
>> commits until after the draw.
>>
>> Are there other layer types with special behaviors that I should be
>> considering?
>>
>
> DelegatedRendererLayer? It's like texture layer but with an arbitrary
> number of resources.
>

Also IOSurfaceLayer on mac is like a texture layer, but using a system
resource. I won't claim to know what semantics are expected wrt
synchronization though.



>
>>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>>
>
>

[enne (OOO), 2013-08-07 21:51:19]

Whoa whoa whoa.  How does hit patch change memory behavior?

Previously we had this:

commit A => activate A => draw A[0] => commit B => activate B[1] => draw B

[0] Commit B not allowed until this draw happens
[1] Draw A continues happening prior to this step as needed

This patch now allows this:

commit A => activate A => commit B => draw A[2] => activate B[3] => draw B

[2] Now a second commit can happen prior to drawing the active tree.  However,
activate B is prevented until draw A to throttle updates from the main thread to
make sure we're not activating more frequently than we can draw.
[3] Draw A continues happening prior to this step as needed

In both cases there are still "only" three trees: main thread, pending, active.

[danakj, 2013-08-07 21:59:40]

On Wed, Aug 7, 2013 at 5:51 PM, <enne@chromium.org> wrote:

> Whoa whoa whoa.  How does hit patch change memory behavior?
>
> Previously we had this:
>
> commit A => activate A => draw A[0] => commit B => activate B[1] => draw B
>
> [0] Commit B not allowed until this draw happens
> [1] Draw A continues happening prior to this step as needed
>
> This patch now allows this:
>
> commit A => activate A => commit B => draw A[2] => activate B[3] => draw B
>

Yeh, commit can't happen between [2] and [3] because you can't commit while
there is a pending tree. So I agree, memory shouldn't be changed overall.

Also, piman@ when the main thread commits, it still has to wait for the
activate before it can reuse old resources now. But we have a commit blocks
activation flag for these layers. Without that, they'd need to wait for a
notification that activation happened. Is this somehow changed by this
patch? If you can't finish commit until activate, then commit B won't be
"finished" until activate B which means A won't be drawn again at that
time. I'm not sure anything changes wrt that either..



>
> [2] Now a second commit can happen prior to drawing the active tree.
>  However,
> activate B is prevented until draw A to throttle updates from the main
> thread to
> make sure we're not activating more frequently than we can draw.
> [3] Draw A continues happening prior to this step as needed
>
> In both cases there are still "only" three trees: main thread, pending,
> active.
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[piman, 2013-08-07 22:15:01]

On Wed, Aug 7, 2013 at 2:59 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Aug 7, 2013 at 5:51 PM, <enne@chromium.org> wrote:
>
>> Whoa whoa whoa.  How does hit patch change memory behavior?
>>
>> Previously we had this:
>>
>> commit A => activate A => draw A[0] => commit B => activate B[1] => draw B
>>
>> [0] Commit B not allowed until this draw happens
>> [1] Draw A continues happening prior to this step as needed
>>
>> This patch now allows this:
>>
>> commit A => activate A => commit B => draw A[2] => activate B[3] => draw B
>>
>
> Yeh, commit can't happen between [2] and [3] because you can't commit
> while there is a pending tree. So I agree, memory shouldn't be changed
> overall.
>
> Also, piman@ when the main thread commits, it still has to wait for the
> activate before it can reuse old resources now. But we have a commit blocks
> activation flag for these layers. Without that, they'd need to wait for a
> notification that activation happened. Is this somehow changed by this
> patch? If you can't finish commit until activate, then commit B won't be
> "finished" until activate B which means A won't be drawn again at that
> time. I'm not sure anything changes wrt that either..
>

So, you're saying, maybe that's the piece of info I'm missing, we can only
have a second commit before the draw if we're on impl-side painting?

>
>
>
>>
>> [2] Now a second commit can happen prior to drawing the active tree.
>>  However,
>> activate B is prevented until draw A to throttle updates from the main
>> thread to
>> make sure we're not activating more frequently than we can draw.
>> [3] Draw A continues happening prior to this step as needed
>>
>> In both cases there are still "only" three trees: main thread, pending,
>> active.
>>
>>
>>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>>
>
>

[piman, 2013-08-07 22:59:02]

On Wed, Aug 7, 2013 at 2:51 PM, <enne@chromium.org> wrote:

> Whoa whoa whoa.  How does hit patch change memory behavior?
>
> Previously we had this:
>
> commit A => activate A => draw A[0] => commit B => activate B[1] => draw B
>
> [0] Commit B not allowed until this draw happens
> [1] Draw A continues happening prior to this step as needed
>
> This patch now allows this:
>
> commit A => activate A => commit B => draw A[2] => activate B[3] => draw B
>

So to me what this means is that we will increase commit latency for frame
B if the limitation is the draw frequency.
If there's 16ms between draw A and draw B, then previously there's <16ms
between commit B and draw B, whereas after there's >16ms.
It does allow better overlap, so it can increase throughput - though so can
increasing the number of frames in flights (which would be simpler if that
was the desired outcome).



>
> [2] Now a second commit can happen prior to drawing the active tree.
>  However,
> activate B is prevented until draw A to throttle updates from the main
> thread to
> make sure we're not activating more frequently than we can draw.
> [3] Draw A continues happening prior to this step as needed
>
> In both cases there are still "only" three trees: main thread, pending,
> active.
>
>
>
https://codereview.chromium.**org/16871016/<https://codereview.chromium.org/1...
>

[brianderson, 2013-08-08 00:11:26]

On 2013/08/07 22:15:01, piman wrote:
> On Wed, Aug 7, 2013 at 2:59 PM, Dana Jansens <mailto:danakj@chromium.org>
wrote:
> 
> > On Wed, Aug 7, 2013 at 5:51 PM, <mailto:enne@chromium.org> wrote:
> >
> >> Whoa whoa whoa.  How does hit patch change memory behavior?
> >>
> >> Previously we had this:
> >>
> >> commit A => activate A => draw A[0] => commit B => activate B[1] => draw B
> >>
> >> [0] Commit B not allowed until this draw happens
> >> [1] Draw A continues happening prior to this step as needed
> >>
> >> This patch now allows this:
> >>
> >> commit A => activate A => commit B => draw A[2] => activate B[3] => draw B
> >>
> >
> > Yeh, commit can't happen between [2] and [3] because you can't commit
> > while there is a pending tree. So I agree, memory shouldn't be changed
> > overall.
> >
> > Also, piman@ when the main thread commits, it still has to wait for the
> > activate before it can reuse old resources now. But we have a commit blocks
> > activation flag for these layers. Without that, they'd need to wait for a
> > notification that activation happened. Is this somehow changed by this
> > patch? If you can't finish commit until activate, then commit B won't be
> > "finished" until activate B which means A won't be drawn again at that
> > time. I'm not sure anything changes wrt that either..
> >
> 
> So, you're saying, maybe that's the piece of info I'm missing, we can only
> have a second commit before the draw if we're on impl-side painting?

If we are not impl-side-painting, this patch still waits for the first draw to
occur for commit 1 before sending the BeginFrame to the main thread for commit
2.

I have not changed the logic for
ThreadProxy::completion_event_for_commit_held_on_tree_activation_ (used for
double buffered layers) or
ThreadProxy::texture_acquisition_completion_event_on_impl_thread_ (used for
single buffered TextureLayers), so any layer that depended on them before will
behave the same way after this patch.

With impl-side-painting, we've always had to support having both the pending and
active tree with separate contents, so we should use the same number of
resources as before regardless of whether we've drawn the active tree or not.

Additionally, this patch does not yet send BeginFrame to them main thread for
commit 3 until commit 2 has been activated. I want to do that eventually in a
follow up patch since I've seen it improve rAF performance in local tests, but
it looks like I'll need to make sure resource usage doesn't go up too much for
certain layer types (and that we don't try to do too much on single/dual core
devices).


> 
> >
> >
> >
> >>
> >> [2] Now a second commit can happen prior to drawing the active tree.
> >>  However,
> >> activate B is prevented until draw A to throttle updates from the main
> >> thread to
> >> make sure we're not activating more frequently than we can draw.
> >> [3] Draw A continues happening prior to this step as needed
> >>
> >> In both cases there are still "only" three trees: main thread, pending,
> >> active.
> >>
> >>
> >>
>
https://codereview.chromium.**org/16871016/%3Chttps://codereview.chromium.org...>
> >>
> >
> >

[brianderson, 2013-08-08 00:24:36]

On 2013/08/07 22:59:02, piman wrote:
> On Wed, Aug 7, 2013 at 2:51 PM, <mailto:enne@chromium.org> wrote:
> 
> > Whoa whoa whoa.  How does hit patch change memory behavior?
> >
> > Previously we had this:
> >
> > commit A => activate A => draw A[0] => commit B => activate B[1] => draw B
> >
> > [0] Commit B not allowed until this draw happens
> > [1] Draw A continues happening prior to this step as needed
> >
> > This patch now allows this:
> >
> > commit A => activate A => commit B => draw A[2] => activate B[3] => draw B
> >
> 
> So to me what this means is that we will increase commit latency for frame
> B if the limitation is the draw frequency.
> If there's 16ms between draw A and draw B, then previously there's <16ms
> between commit B and draw B, whereas after there's >16ms.
> It does allow better overlap, so it can increase throughput - though so can
> increasing the number of frames in flights (which would be simpler if that
> was the desired outcome).
> 
> 

This patch does a few things that affect latency and throughput:
1) It starts off trying to do everything in 1 frame of latency, all the way from
Renderer Main to Browser Impl and GPU thread.
2) If we can't hit 1 frame of latency consistently, it falls back to a higher
latency mode with more frames in flight.
3) No attempt at recovering latency is made yet once we fall behind.

By decoupling the sending of the BeginFrame message to the main thread from the
deadline/drawing, we can handle jank on the main thread much better when a
single frame takes more than 16ms (up to the next deadline).

This patch actually couples the sending of BeginFrame to the main thread with
the BeginFrame it gets from OutputSurface. Eventually this will be coordinated
with user input as well (it already is on Android), which is the most important
aspect of latency. After we send user input on the main thread, we don't want to
delay the commit longer than we need to.

> 
> >
> > [2] Now a second commit can happen prior to drawing the active tree.
> >  However,
> > activate B is prevented until draw A to throttle updates from the main
> > thread to
> > make sure we're not activating more frequently than we can draw.
> > [3] Draw A continues happening prior to this step as needed
> >
> > In both cases there are still "only" three trees: main thread, pending,
> > active.
> >
> >
> >
>
https://codereview.chromium.**org/16871016/%3Chttps://codereview.chromium.org...>
> >

[brianderson, 2013-08-08 02:45:04]

https://codereview.chromium.org/16871016/diff/232001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/16871016/diff/232001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:68: deadline = base::TimeTicks();
@piman: I know you had reservations previously about using a hard-coded deadline
on ChromeOS, but after playing around with a hard coded deadline localy, I don't
see many negatives of using a deadline - even if it's not very accurate. If we
make the deadlines, we get awesome latency. If we don't, then the scheduler
falls back to the old behavior.

The only negative I can think of is that, in theory, sometimes we might start in
a low latency mode and jank to a higher latency mode. In that case, it may have
been better to target a higher latency in the first place.  In practice though,
I find that fast pages are fast and slow pages are janky anyway.

Do you mind if I remove this ifdef?

[enne (OOO), 2013-08-15 19:47:59]

I took a look through all the test changes.  Most of the changes seem fine, but
I have some questions on a few.

I don't really have more comments on this patch at this point (other than
piman's latency question), although that's partially because I don't know that I
can review the scheduler / scheduler state machine changes.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:89: EXPECT_GE(num_commits_, 1);
I think this isn't what this test is looking for.  Given the comment above the
class, I'd expect this to be:

    EXPECT_EQ(1, num_commits_);
    EXPECT_GE(num_draws_, 1);

...although I don't see where a second draw would come from and it seems like
that should be EXPECT_EQ as well.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:705: : frame_count_with_pending_tree_(0) {}
style nit: indentation.  Consider 'git cl format'-ing your change.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:721: if (frame_count_with_pending_tree_ >
1) {
Why do you need two begin frames before the first draw? I only see one
PostSetNeedsCommit; where does the second begin frame come from?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1396: class
LayerTreeHostTestFinishAllRendering : public LayerTreeHostTest {
Since you're updating this, can you add a class-level comment for what this test
is trying to test?

It seems like it's trying to test that if the scheduler is asked to draw but
layer textures are acquired, that draws are still prevented?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1494: if
(host_impl->active_tree()->source_frame_number() == 0) {
It looks like you're changing the memory policy during activation of source
frame 0 but before drawing frame 0, whereas before it was between draw of frame
0 and draw of frame 1.  How is this still working?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1834: virtual void DidCommit() OVERRIDE {
I think this change here is right, but this is a bizarre test.  It seems like
CommitCompleteOnThread should EXPECT_EQ(num_draw_layers, num_commit_complete++),
if it's actually trying to make sure that we don't commit without drawing?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:2232:
&LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled::EndTest,
style nit: indentation.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (left):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:779: EXPECT_EQ(2,
added_animations_);
Just as a sanity check, move this to AfterTest rather than removing it?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:273: if
(!host_impl->active_tree()->root_layer())
I'm a little confused why animation is happening with this patch.  It looks like
in LTHI that animation happens right before drawing or during a background tick.
 Is drawing happening without a root layer for this test?

That said, what do you think about moving this early out (and other similar
ones) to LayerTreeHostImplForTesting::AnimateLayers or even up to the callers of
LayerTreeHostImpl::Animate?

The real LayerTreeHostImpl bails when there's not an active tree and I don't see
a reason to call animate without an active tree.  Also, all the unit tests still
pass with that change.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:164: } else if
(did_reset_child_id_) {
I'm not sure I understand the changes to this test.  How can you get a fourth
activation prior to initializing the renderer? It seems like you'd have to
initialize the renderer and draw the tree from the third activation first.

[brianderson, 2013-08-15 23:05:09]

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:89: EXPECT_GE(num_commits_, 1);
On 2013/08/15 19:47:59, enne wrote:
> I think this isn't what this test is looking for.  Given the comment above the
> class, I'd expect this to be:
> 
>     EXPECT_EQ(1, num_commits_);
>     EXPECT_GE(num_draws_, 1);
> 
> ...although I don't see where a second draw would come from and it seems like
> that should be EXPECT_EQ as well.

I tried to make the code match the comments here ("at least 1 commit and at
least 1 draw"), but looking at the code I would expect exactly 1 of each as
well.  I made this change pretty early on, when my changes were likely
misbehaving, so I will change it to EXPECT_EQ and see how it goes.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:705: : frame_count_with_pending_tree_(0) {}
On 2013/08/15 19:47:59, enne wrote:
> style nit: indentation.  Consider 'git cl format'-ing your change.

Magic! Thanks for the pointer.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:721: if (frame_count_with_pending_tree_ >
1) {
On 2013/08/15 19:47:59, enne wrote:
> Why do you need two begin frames before the first draw? I only see one
> PostSetNeedsCommit; where does the second begin frame come from?

I really need to clarify the uses of "BeginFrame" in the code. 
WillBeginFrameOnThread refers to the iml thread's BeginFrame not the commit's
BeginFrame.

In this test, the first BeginFrame on the impl thread does not activate or draw,
then the second BeginFrame activates and draws.

I should actually remove CanActivatePendingTreeIfNeeded from this test all
together since it should no longer be called after the activation changes in
more recent versions of this patch.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1396: class
LayerTreeHostTestFinishAllRendering : public LayerTreeHostTest {
On 2013/08/15 19:47:59, enne wrote:
> Since you're updating this, can you add a class-level comment for what this
test
> is trying to test?
> 
> It seems like it's trying to test that if the scheduler is asked to draw but
> layer textures are acquired, that draws are still prevented?

I will add a comment.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1494: if
(host_impl->active_tree()->source_frame_number() == 0) {
On 2013/08/15 19:47:59, enne wrote:
> It looks like you're changing the memory policy during activation of source
> frame 0 but before drawing frame 0, whereas before it was between draw of
frame
> 0 and draw of frame 1.  How is this still working?

My understanding of the memory policy comes from the existing comment below. The
comment or my understanding of it may be wrong: I took the comment to mean that
new resources for tiles would not be allocated after changing the memory policy
here.

I believe this test was flaky after my changes because the second commit could
finish and start rasterizing before the first commit was drawn. If the memory
policy wasn't changed until the first draw, then the second commit could get
resources allocated to it.

So I changed the memory policy after the first activation, which definitely
occurs before the second commit.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1834: virtual void DidCommit() OVERRIDE {
On 2013/08/15 19:47:59, enne wrote:
> I think this change here is right, but this is a bizarre test.  It seems like
> CommitCompleteOnThread should EXPECT_EQ(num_draw_layers,
num_commit_complete++),
> if it's actually trying to make sure that we don't commit without drawing?

I will add that check since it would check all commits rather than just the
commits between the first and second draw.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (left):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:779: EXPECT_EQ(2,
added_animations_);
On 2013/08/15 19:47:59, enne wrote:
> Just as a sanity check, move this to AfterTest rather than removing it?

Ok.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:273: if
(!host_impl->active_tree()->root_layer())
On 2013/08/15 19:47:59, enne wrote:
> I'm a little confused why animation is happening with this patch.  It looks
like
> in LTHI that animation happens right before drawing or during a background
tick.
>  Is drawing happening without a root layer for this test?
> 
> That said, what do you think about moving this early out (and other similar
> ones) to LayerTreeHostImplForTesting::AnimateLayers or even up to the callers
of
> LayerTreeHostImpl::Animate?
> 
> The real LayerTreeHostImpl bails when there's not an active tree and I don't
see
> a reason to call animate without an active tree.  Also, all the unit tests
still
> pass with that change.

I would be fine with moving the early out up to the callers of
LayerTreeHostImpl::Animate. Is it okay that we do not animate the pending tree
when there's no active tree?

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:164: } else if
(did_reset_child_id_) {
On 2013/08/15 19:47:59, enne wrote:
> I'm not sure I understand the changes to this test.  How can you get a fourth
> activation prior to initializing the renderer? It seems like you'd have to
> initialize the renderer and draw the tree from the third activation first.

I made changes to the Scheduler's behavior when we lose the output surface such
that it clears the pipeline by immediately activating the pending tree and
aborting the draw of that activation.  (Then, after output surface
initialization, the Scheduler waits for the first commit to activate before
drawing.)

The way I clear the pipeline ruined the assumptions of this test.

I could have cleared the pipeline as a special function, but I found it easier
to reason about the behavior of the scheduler when it goes through the motions
in corner cases like this rather than adding another branch into it's behavior.

[brianderson, 2013-08-15 23:30:23]

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1494: if
(host_impl->active_tree()->source_frame_number() == 0) {
On 2013/08/15 23:05:10, Brian Anderson wrote:
> On 2013/08/15 19:47:59, enne wrote:
> > It looks like you're changing the memory policy during activation of source
> > frame 0 but before drawing frame 0, whereas before it was between draw of
> frame
> > 0 and draw of frame 1.  How is this still working?
> 
> My understanding of the memory policy comes from the existing comment below.
The
> comment or my understanding of it may be wrong: I took the comment to mean
that
> new resources for tiles would not be allocated after changing the memory
policy
> here.
> 
> I believe this test was flaky after my changes because the second commit could
> finish and start rasterizing before the first commit was drawn. If the memory
> policy wasn't changed until the first draw, then the second commit could get
> resources allocated to it.
> 
> So I changed the memory policy after the first activation, which definitely
> occurs before the second commit.

@reveman or @vmpstr, can changing the memory policy after activation affect the
subsequent draw?

[brianderson, 2013-08-15 23:45:10]

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:273: if
(!host_impl->active_tree()->root_layer())
>  Is drawing happening without a root layer for this test?

Animation used to be coupled with drawing, but this patch moves drawing to the
deadline and keeps animation in the impl thread's BeginFrame when using
ThreadProxy.

This should allow us to better prioritize tiles and make more informed
activation decisions before the deadline.

[reveman, 2013-08-15 23:46:35]

On 2013/08/15 23:30:23, Brian Anderson wrote:
>
https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_unittest.cc:1494: if
> (host_impl->active_tree()->source_frame_number() == 0) {
> On 2013/08/15 23:05:10, Brian Anderson wrote:
> > On 2013/08/15 19:47:59, enne wrote:
> > > It looks like you're changing the memory policy during activation of
source
> > > frame 0 but before drawing frame 0, whereas before it was between draw of
> > frame
> > > 0 and draw of frame 1.  How is this still working?
> > 
> > My understanding of the memory policy comes from the existing comment below.
> The
> > comment or my understanding of it may be wrong: I took the comment to mean
> that
> > new resources for tiles would not be allocated after changing the memory
> policy
> > here.
> > 
> > I believe this test was flaky after my changes because the second commit
could
> > finish and start rasterizing before the first commit was drawn. If the
memory
> > policy wasn't changed until the first draw, then the second commit could get
> > resources allocated to it.
> > 
> > So I changed the memory policy after the first activation, which definitely
> > occurs before the second commit.
> 
> @reveman or @vmpstr, can changing the memory policy after activation affect
the
> subsequent draw?

No, it will not affect the subsequent draw unless you call ManageTiles(). All
bets are off if you call ManageTiles().

[enne (OOO), 2013-08-16 00:53:21]

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:721: if (frame_count_with_pending_tree_ >
1) {
On 2013/08/15 23:05:10, Brian Anderson wrote:
> On 2013/08/15 19:47:59, enne wrote:
> > Why do you need two begin frames before the first draw? I only see one
> > PostSetNeedsCommit; where does the second begin frame come from?
> 
> I really need to clarify the uses of "BeginFrame" in the code. 
> WillBeginFrameOnThread refers to the iml thread's BeginFrame not the commit's
> BeginFrame.
> 
> In this test, the first BeginFrame on the impl thread does not activate or
draw,
> then the second BeginFrame activates and draws.
> 
> I should actually remove CanActivatePendingTreeIfNeeded from this test all
> together since it should no longer be called after the activation changes in
> more recent versions of this patch.

OH.  One confusing begin frame name to rule them all.  :(

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1494: if
(host_impl->active_tree()->source_frame_number() == 0) {
On 2013/08/15 23:30:25, Brian Anderson wrote:
> On 2013/08/15 23:05:10, Brian Anderson wrote:
> > On 2013/08/15 19:47:59, enne wrote:
> > > It looks like you're changing the memory policy during activation of
source
> > > frame 0 but before drawing frame 0, whereas before it was between draw of
> > frame
> > > 0 and draw of frame 1.  How is this still working?
> > 
> > My understanding of the memory policy comes from the existing comment below.
> The
> > comment or my understanding of it may be wrong: I took the comment to mean
> that
> > new resources for tiles would not be allocated after changing the memory
> policy
> > here.
> > 
> > I believe this test was flaky after my changes because the second commit
could
> > finish and start rasterizing before the first commit was drawn. If the
memory
> > policy wasn't changed until the first draw, then the second commit could get
> > resources allocated to it.
> > 
> > So I changed the memory policy after the first activation, which definitely
> > occurs before the second commit.
> 
> @reveman or @vmpstr, can changing the memory policy after activation affect
the
> subsequent draw?

Ok.  Maybe this could be more robust if when you activate source frame #1 you
change the policy *and* manage tiles.  Otherwise, moving around when we manage
tiles could break this test.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_animation.cc:273: if
(!host_impl->active_tree()->root_layer())
On 2013/08/15 23:05:10, Brian Anderson wrote:
> On 2013/08/15 19:47:59, enne wrote:
> > I'm a little confused why animation is happening with this patch.  It looks
> like
> > in LTHI that animation happens right before drawing or during a background
> tick.
> >  Is drawing happening without a root layer for this test?
> > 
> > That said, what do you think about moving this early out (and other similar
> > ones) to LayerTreeHostImplForTesting::AnimateLayers or even up to the
callers
> of
> > LayerTreeHostImpl::Animate?
> > 
> > The real LayerTreeHostImpl bails when there's not an active tree and I don't
> see
> > a reason to call animate without an active tree.  Also, all the unit tests
> still
> > pass with that change.
> 
> I would be fine with moving the early out up to the callers of
> LayerTreeHostImpl::Animate. Is it okay that we do not animate the pending tree
> when there's no active tree?

Yeah.  LayerTreeHostImpl::Animate does this check.  The only way there can be no
active tree is if there has never been a commit.  Animations aren't started
until they get to the active tree so that the full animation gets displayed to
the user.  Therefore, the only reason you need to animate the pending tree is if
there's an animation happening on the active tree that also affects the same
layer in the pending tree.

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:164: } else if
(did_reset_child_id_) {
On 2013/08/15 23:05:10, Brian Anderson wrote:
> On 2013/08/15 19:47:59, enne wrote:
> > I'm not sure I understand the changes to this test.  How can you get a
fourth
> > activation prior to initializing the renderer? It seems like you'd have to
> > initialize the renderer and draw the tree from the third activation first.
> 
> I made changes to the Scheduler's behavior when we lose the output surface
such
> that it clears the pipeline by immediately activating the pending tree and
> aborting the draw of that activation.  (Then, after output surface
> initialization, the Scheduler waits for the first commit to activate before
> drawing.)
> 
> The way I clear the pipeline ruined the assumptions of this test.
> 
> I could have cleared the pipeline as a special function, but I found it easier
> to reason about the behavior of the scheduler when it goes through the motions
> in corner cases like this rather than adding another branch into it's
behavior.

Ah, ok.  This is that new aborted draw case.  Thanks for all the patient
explanation.  :)

[brianderson, 2013-08-16 01:20:47]

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/16871016/diff/266001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:1494: if
(host_impl->active_tree()->source_frame_number() == 0) {
On 2013/08/16 00:53:22, enne wrote:
> On 2013/08/15 23:30:25, Brian Anderson wrote:
> > On 2013/08/15 23:05:10, Brian Anderson wrote:
> > > On 2013/08/15 19:47:59, enne wrote:
> > > > It looks like you're changing the memory policy during activation of
> source
> > > > frame 0 but before drawing frame 0, whereas before it was between draw
of
> > > frame
> > > > 0 and draw of frame 1.  How is this still working?
> > > 
> > > My understanding of the memory policy comes from the existing comment
below.
> > The
> > > comment or my understanding of it may be wrong: I took the comment to mean
> > that
> > > new resources for tiles would not be allocated after changing the memory
> > policy
> > > here.
> > > 
> > > I believe this test was flaky after my changes because the second commit
> could
> > > finish and start rasterizing before the first commit was drawn. If the
> memory
> > > policy wasn't changed until the first draw, then the second commit could
get
> > > resources allocated to it.
> > > 
> > > So I changed the memory policy after the first activation, which
definitely
> > > occurs before the second commit.
> > 
> > @reveman or @vmpstr, can changing the memory policy after activation affect
> the
> > subsequent draw?
> 
> Ok.  Maybe this could be more robust if when you activate source frame #1 you
> change the policy *and* manage tiles.  Otherwise, moving around when we manage
> tiles could break this test.

Hmmm. I tried calling ManageTiles after changing the memory policy for source
frame #0 and the first draw still succeeded. Is that expected?

I'm not sure if calling ManageTiles after the second activate will help make
this test more robust, since ManagegTiles might still be initiated after the
second commit but before the first draw.

I'm trying to think of a better way to write this test... maybe move the checks
for HaveCachedResourcesForRenderPassId for source frame #0 here.

[brianderson, 2013-08-16 02:26:15]

> I don't really have more comments on this patch at this point (other than
piman's latency question), although that's partially because I don't know that I
can review the scheduler / scheduler state machine changes.

Regarding the concern that sending the BeginFrame to the main thread before
drawing might increasing latency:

Although this patch will start the commit earlier, the BeginFrame is still only
sent to the main thread after we receive the BeginFrame from the Browser, which
will eventually indicate (once the BIC work is done) that all inputs for this
interval have been sent. So there will be no hit in terms of user input latency,
but we will get the benefit of overlapping the commit with the draw.

[enne (OOO), 2013-08-16 17:40:18]

On 2013/08/16 02:26:15, Brian Anderson wrote:
> Regarding the concern that sending the BeginFrame to the main thread before
> drawing might increasing latency:
> 
> Although this patch will start the commit earlier, the BeginFrame is still
only
> sent to the main thread after we receive the BeginFrame from the Browser,
which
> will eventually indicate (once the BIC work is done) that all inputs for this
> interval have been sent. So there will be no hit in terms of user input
latency,
> but we will get the benefit of overlapping the commit with the draw.

What about latency for rAF frames? Is rAF also tied to BeginFrame to the main
thread?

[brianderson, 2013-08-16 18:49:20]

On 2013/08/16 17:40:18, enne wrote:
> On 2013/08/16 02:26:15, Brian Anderson wrote:
> > Regarding the concern that sending the BeginFrame to the main thread before
> > drawing might increasing latency:
> > 
> > Although this patch will start the commit earlier, the BeginFrame is still
> only
> > sent to the main thread after we receive the BeginFrame from the Browser,
> which
> > will eventually indicate (once the BIC work is done) that all inputs for
this
> > interval have been sent. So there will be no hit in terms of user input
> latency,
> > but we will get the benefit of overlapping the commit with the draw.
> 
> What about latency for rAF frames? Is rAF also tied to BeginFrame to the main
> thread?

rAF is also tied to the BeginFrame to the main thread.

Now that I think about it, I would actually argue that starting the commit
before the draw can actually help latency if the commit otherwise would not have
finished in time for the next deadline. In that case, trying to save a
fractional frame of latency would actually cost us a whole frame of latency.
(Once the BIC is in place, we won't be able to save a fractional frame of
latency by committing later anyway. Also, Android throttles user input to the
vsync for us already.)

[brianderson, 2013-08-19 21:53:24]

A quick update on the state of this patch:

I am attempting to break it up into multiple parts. It is proving difficult so,
if it takes too much longer, I might try pushing this patch again as-is.

I was hoping to pull out just the activation changes, but the unit tests break
without the readback and output surface state changes, which are more
intertwined with the deadline changes.

[danakj, 2013-08-19 21:54:39]

Oh! okay, here's a couple tiny comments then.

I'd love a primer on what this retroactive begin frame stuff is all about..

https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:52: return
base::TimeDelta::FromMicroseconds(-16666 / 3);
can you comment explaining this constant?

https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:64: #if defined(OS_ANDROID)
ifdefs make unittests hard to write. i guess you have no unit tests that call
this method in any way then? is that missing test coverage?

it'd be nicer if this decision was made somewhere that can see
LayerTreeSettings, and given to this class in the constructor.

https://codereview.chromium.org/16871016/diff/288001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16871016/diff/288001/cc/output/output_surface...
cc/output/output_surface.cc:275: if (frame_rate_controller_)
This seems like it could be a separate CL?

[brianderson, 2013-08-20 01:50:53]

On 2013/08/19 21:54:39, danakj wrote:
> Oh! okay, here's a couple tiny comments then.
> 
> I'd love a primer on what this retroactive begin frame stuff is all about..

Retroactive BeginFrames are BeginFrames that start in the middle of the
BeginFrame+deadline interval. They wanted to start at the beginning, but were
blocked for one reason or another including:
1) SetNeedsBeginFrame was previously false. (This is very important for the
first frame's latency.)
2) We did not get the SwapAck back yet.
3) The previous BeginFrame ran long and ate into the next BeginFrame's interval.

Instead of skipping the frame all together, the retroactive BeginFrame attempts
to catch up.

> 
>
https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
> File cc/output/begin_frame_args.cc (right):
> 
>
https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
> cc/output/begin_frame_args.cc:52: return
> base::TimeDelta::FromMicroseconds(-16666 / 3);
> can you comment explaining this constant?
> 

Done.

>
https://codereview.chromium.org/16871016/diff/288001/cc/output/begin_frame_ar...
> cc/output/begin_frame_args.cc:64: #if defined(OS_ANDROID)
> ifdefs make unittests hard to write. i guess you have no unit tests that call
> this method in any way then? is that missing test coverage?
> 
> it'd be nicer if this decision was made somewhere that can see
> LayerTreeSettings, and given to this class in the constructor.

I went ahead and created an entry in LayerTreeSettings in the most recent patch.

> 
>
https://codereview.chromium.org/16871016/diff/288001/cc/output/output_surface.cc
> File cc/output/output_surface.cc (right):
> 
>
https://codereview.chromium.org/16871016/diff/288001/cc/output/output_surface...
> cc/output/output_surface.cc:275: if (frame_rate_controller_)
> This seems like it could be a separate CL?

I never ran into the bug this fixes with the old scheduler, but it could be a
separate patch, yes.

[brianderson, 2013-08-20 02:02:58]

Hopefully this is the last time I make this patch any bigger.

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:193: // We emulate the old non-deadline scheduler here
by posting the
I got rid of the AdjustDeadline function that had the ifdefs and use the new
flag here to decide for when to post the deadline task.

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:704: // behavior of blocking the next
commit until after the draw.
Since the feature of sending the BeginFrame to the main thread before the active
tree has been drawn was a major concern, I've hidden that behavior behind the
deadline_scheduling_enabled flag here.

https://codereview.chromium.org/16871016/diff/304002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/16871016/diff/304002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2940: if
(compositor->layer_tree_settings().deadline_scheduling_enabled) {
piman: Is this an acceptable hack until we add real BeginFrame messages for
Aura?

[danakj, 2013-08-20 02:27:20]

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/16871016/diff/304002/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:704: // behavior of blocking the next
commit until after the draw.
On 2013/08/20 02:03:01, Brian Anderson wrote:
> Since the feature of sending the BeginFrame to the main thread before the
active
> tree has been drawn was a major concern, I've hidden that behavior behind the
> deadline_scheduling_enabled flag here.

If you can imagine people wanting to toggle the two independently for testing
perf etc, it might be worth adding a separate setting for this. Or people could
just come in here and change the code as well.

[brianderson, 2013-09-04 18:35:29]

Closing this issue since it has been broken up into multiple patches.  Remaining
patches not in ToT yet can be found here:

https://codereview.chromium.org/23503003/
https://codereview.chromium.org/23796002/