cc: Implement deadine scheduling disabled by default

cc: Implement deadine scheduling disabled by default

This patch adds logic to the Scheduler to actually use the BeginFrame
and deadline, but is not enabled by default on any platform yet. This
will ensure emulation of old scheduler in the fallback path is sane.
Emulation of the old path is implemented using an immediate 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. (True on Android.
Soon to be true on other platforms.)
- 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 compositors can be nested 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.

BUG=243461

[brianderson, 2013-08-30 00:12:28]

This depends on https://codereview.chromium.org/23503003/.

I have not fixed any tests yet, but it works in normal operation and improves
latency. Looking for early feedback if anyone can take a look.

I explicitly did not include the change to start the next commit before the
previous commit has drawn to potentially improve throughput. I will do that as a
separate patch with a separate flag.

This patch primarily addresses latency, but has logic that could help with a
little with throughput (if there are no CSS animations or fast touch paths
enabled) by immediately triggering draws when there is a new active tree.

[Sami, 2013-08-30 13:49:06]

Great to see this happening :)

https://codereview.chromium.org/23796002/diff/1/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/23796002/diff/1/cc/base/switches.cc#newcode31
cc/base/switches.cc:31: // Overrides the kDisableDeadlineShceduling flag.
Typo: kDisableDeadlineScheduling

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

https://codereview.chromium.org/23796002/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:810: // Proactive BeginFrames are bad
for the synchronous compoisitor because we
Typo: compositor

https://codereview.chromium.org/23796002/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:845: // TODO(brianderson): This should
take into account multiple commit sources.
When could something like this happen? If there's both main thread animations
and input event handling going on? It would seem like being greedy like you've
done is a good default.

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

https://codereview.chromium.org/23796002/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine_unittest.cc:12: if (action ==
SchedulerStateMachine::ACTION_DRAW_AND_SWAP_IF_POSSIBLE || \
Nit: maybe separate this part into a normal function?

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

https://codereview.chromium.org/23796002/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1351: if (frame.has_no_damage ||
output_surface_lost_)
Could you explain why we'd need to avoid swapping a lost output surface?
Generally we only need to react to a lost surface if there is some GL state we
need to invalidate, but I don't think LTHI has anything like that.

https://codereview.chromium.org/23796002/diff/1/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/23796002/diff/1/ui/compositor/compositor.h#ne...
ui/compositor/compositor.h:307: // Retreives the current LayerTreeSettings
Typo: retrieves

[brianderson, 2013-09-03 22:51:46]

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

https://codereview.chromium.org/23796002/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:845: // TODO(brianderson): This should
take into account multiple commit sources.
On 2013/08/30 13:49:06, Sami wrote:
> When could something like this happen? If there's both main thread animations
> and input event handling going on? It would seem like being greedy like you've
> done is a good default.

This isn't so much an issue for Android as it is for ChromeOS, which can have
multiple child Renderers. If there are multiple child Renderers producing
output, this patch will go into a higher latency mode for the Renderer that lost
the commit race rather than waiting till the deadline for the slower Renderer to
commit.

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

https://codereview.chromium.org/23796002/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine_unittest.cc:12: if (action ==
SchedulerStateMachine::ACTION_DRAW_AND_SWAP_IF_POSSIBLE || \
On 2013/08/30 13:49:06, Sami wrote:
> Nit: maybe separate this part into a normal function?

I wanted to do that, but then we lose information regarding the line number that
fails.

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

https://codereview.chromium.org/23796002/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1351: if (frame.has_no_damage ||
output_surface_lost_)
On 2013/08/30 13:49:06, Sami wrote:
> Could you explain why we'd need to avoid swapping a lost output surface?
> Generally we only need to react to a lost surface if there is some GL state we
> need to invalidate, but I don't think LTHI has anything like that.

This was a change I pulled in from the big patch "just in case". I'll see if I
can remove it since this was added very early on and is likely no longer needed
since we have ACTION_DRAW_AND_SWAP_ABORT now.

[brianderson, 2013-09-06 03:28:42]

Tests have been fixed and Sami's comments addressed. ptal.

[enne (OOO), 2013-09-10 03:35:43]

https://codereview.chromium.org/23796002/diff/27001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/23796002/diff/27001/cc/base/switches.cc#newco...
cc/base/switches.cc:31: // Overrides the kDisableDeadlineScheduling flag.
typo? s/Disable/Enable/

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:184: // The synchronous renderer compositor has to
make it's GL calls
s/'//

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

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:178:
major_state->SetString("commit_state",
begin_frame_state

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:478: return false;
Can you rephrase this comment to refer to this "return false" and not to the
"return true" at the end of this function? In this case, it seems like you don't
send a begin frame to the main thread if you are already waiting on a begin
frame to the compositor thread, since you'll do it then.  (Or, am I
misunderstanding?)

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:595: draw_attempt_count_++;
What's the difference between "draw attempt" and "source frame"?  I have to
admit that I'm a bit ಠ_ಠ of how this generates the confusing function name
HasDrawnThisDrawAttempt.

It also seems to me like you shouldn't bump the draw attempt count in both
commit and activation (and begin frame too?).  In impl-side painting for
example, commit shouldn't affect drawing state.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:746: begin_frame_state_ =
BEGIN_FRAME_STATE_IDLE;
Just so I understand this correctly, this is the normal (only?) mechanism for
going from BEGIN_FRAME_STATE_INSIDE_DEADLINE to BEGIN_FRAME_STATE_IDLE?

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:850: DCHECK_EQ(begin_frame_state_,
BEGIN_FRAME_STATE_DEADLINE_PENDING);
What guarantees that this is true? It seems like you only get to
BEGIN_FRAME_STATE_DEADLINE_PENDING via AdvanceBeginFrameStateWhenNoActionsRemain
and that seems like it would race with this PostTask-initiated event.

[brianderson, 2013-09-10 21:16:55]

https://codereview.chromium.org/23796002/diff/27001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/23796002/diff/27001/cc/base/switches.cc#newco...
cc/base/switches.cc:31: // Overrides the kDisableDeadlineScheduling flag.
On 2013/09/10 03:35:44, enne wrote:
> typo? s/Disable/Enable/

Done.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:184: // The synchronous renderer compositor has to
make it's GL calls
On 2013/09/10 03:35:44, enne wrote:
> s/'//

Done.

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

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:178:
major_state->SetString("commit_state",
On 2013/09/10 03:35:44, enne wrote:
> begin_frame_state

Done.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:478: return false;
On 2013/09/10 03:35:44, enne wrote:
> Can you rephrase this comment to refer to this "return false" and not to the
> "return true" at the end of this function? In this case, it seems like you
don't
> send a begin frame to the main thread if you are already waiting on a begin
> frame to the compositor thread, since you'll do it then.  (Or, am I
> misunderstanding?)

I will change the comment to refer to the "return fals". Also, I don't think the
BeginFrameNeededByImplThread is needed here anymore since the conditions that
come beforehand allow us to make forward progress in all the cases that we need
it explicitly now.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:595: draw_attempt_count_++;
On 2013/09/10 03:35:44, enne wrote:
> What's the difference between "draw attempt" and "source frame"?  I have to
> admit that I'm a bit ಠ_ಠ of how this generates the confusing function name
> HasDrawnThisDrawAttempt.
> 
> It also seems to me like you shouldn't bump the draw attempt count in both
> commit and activation (and begin frame too?).  In impl-side painting for
> example, commit shouldn't affect drawing state.

Note: It is possible for a readback and a swap to occur within the same
BeginImplFrame interval, which was the initial reason to use two different
counters.

begin_frame_count_ is used to block anything that should only be done once per
BeginImplFrame, which include A) swaps and B) sending BeginMainFrames.

draw_attempt_count_ is used to block actions that shouldn't be called more than
once in a series of back-to-back actions within ProcessScheduledActions, which
include 1) readbacks, 2) updating visible tiles, and 3) activating the pending
tree.

With the new code base, however, I think I can get rid of draw_attempt_count,
because there are other mechanisms that will prevent us from doing 1 and 3
multiple times now: 1) The readback state will only draw in a single sate; 3)
Activation will always occur now, blocking subsequent activations until the
active tree has been drawn

For 2, I'm trying to see if I can get rid of
HasUpdatedVisibleTilesThisDrawAttempt. I think I can if I only call it when the
last swap used an imcomplete tile, in which case I can change the name to
HasUpdatedVisibleTilesThisFrame.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:746: begin_frame_state_ =
BEGIN_FRAME_STATE_IDLE;
On 2013/09/10 03:35:44, enne wrote:
> Just so I understand this correctly, this is the normal (only?) mechanism for
> going from BEGIN_FRAME_STATE_INSIDE_DEADLINE to BEGIN_FRAME_STATE_IDLE?

Yes, this is the only way.  We only transition out of
BEGIN_FRAME_STATE_INSIDE_DEADLINE when all actions that occur back-to-back in
response to entering BEGIN_FRAME_STATE_INSIDE_DEADLINE have completed. This is
important because actions after the draw, such as activation and subsequently
sending BeginFrame to the main thread, can occur inside
BEGIN_FRAME_STATE_INSIDE_DEADLINE.  (Sending the BeginFrame to the main thread
will not occur in BEGIN_FRAME_STATE_IDLE.)

I will add a comment.

https://codereview.chromium.org/23796002/diff/27001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:850: DCHECK_EQ(begin_frame_state_,
BEGIN_FRAME_STATE_DEADLINE_PENDING);
On 2013/09/10 03:35:44, enne wrote:
> What guarantees that this is true? It seems like you only get to
> BEGIN_FRAME_STATE_DEADLINE_PENDING via
AdvanceBeginFrameStateWhenNoActionsRemain
> and that seems like it would race with this PostTask-initiated event.

In Scheduler::BeginFrame, we will always transition to the
BEGIN_FRAME_STATE_DEADLINE_PENDING at the end of the call to
ProcessScheduledActions, which will alway happen before any posted tasks. I
suppose I could make that transition explicit rather than relying on
AdvanceBeginFrameStateWhenNoActionsRemain.

Now that I think about it, I should get rid of
AdvanceBeginFrameStateWhenNoActionsRemain altoghether by also transitioning to
BEGIN_FRAME_STATE_IDLE explicitly at the end of Scheduler::OnBeginFrameDeadline.

I really wanted to get rid of AdvanceBeginFrameStateWhenNoActionsRemain, but
couldn't figure out how in older versions of this patch.

[brianderson, 2013-09-11 01:44:23]

ptal. I have addressed enne's comments and fixed the resulting output surface
initialization race conditions that resulted since getting rid of
draw_attempt_count_ changed the timing and sped some things up a bit.

[brianderson, 2013-09-11 19:01:39]

There was one last race condition that was resulting in overlapping
BeginFrame+deadline intervals, which I've fixed by removing the
safe_to_expect_begin_frame_ workaround in
https://codereview.chromium.org/24019008.

I can run cc_unittests forever without hitting those DCHECKs now.

[enne (OOO), 2013-09-11 22:30:46]

Thanks for all the changes.  This CL is a lot more simple.

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:155: has_pending_begin_frame_ &&
!state_machine_.InsideBeginFrame()) ||
has_pending_begin_frame is a funny way of saying that a swap didn't happen.

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:207:
PostBeginFrameDeadline(last_begin_frame_args_.frame_time +
This case almost seems like you shouldn't have a deadline at all.  If there's no
need to draw, then why post a deadline at all?  Or, is this what this comment is
about?

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

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:793: void
SchedulerStateMachine::OnBeginFrameDeadlinePending() {
These functions make things a lot more clear!

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:819: bool
SchedulerStateMachine::InsideBeginFrame() const {
What does "inside begin frame" mean here if this function is also true for the
deadline pending case?

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

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:239: int
last_begin_frame_count_begin_frame_sent_to_main_thread_;
I like this a million times better than draw attempt (both in the naming and in
the only getting updated in one place), but two different begin frames in the
same variable name is a bit unfortunate.

What was wrong with current_frame_number? HasSentBeginFrameToMainThreadThisFrame
still uses that language.

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

https://codereview.chromium.org/23796002/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1500: DCHECK(!client_->IsInsideDraw())
I think this would be cleaner if you just checked client_->IsInsideDraw and only
notified the client if it wasn't.  Then you wouldn't need the notify_client
parameter or the DCHECK.

I think that's pretty reasonable as outside of draw, you need to know you need
another draw.  Inside of draw, you're just about to draw and so it'll get taken
care of.

Alternatively, maybe the client should check its own InsideDraw state and not
set needs redraw in DidInitializeVisibleTileOnImplThread.

[brianderson, 2013-09-12 00:12:33]

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:207:
PostBeginFrameDeadline(last_begin_frame_args_.frame_time +
On 2013/09/11 22:30:47, enne wrote:
> This case almost seems like you shouldn't have a deadline at all.  If there's
no
> need to draw, then why post a deadline at all?  Or, is this what this comment
is
> about?

We may not have to draw yet, but if a new commit comes in to trigger a draw,
that draw will have to wait until the next BeginFrame if we don't post the
deadline here. By posting the deadline task for the next anticipated frame time
when the impl thread doesn't have anything it wants to draw, the impl thread
will always draw as soon as there is a new active tree from the main thread.

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

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:819: bool
SchedulerStateMachine::InsideBeginFrame() const {
On 2013/09/11 22:30:47, enne wrote:
> What does "inside begin frame" mean here if this function is also true for the
> deadline pending case?

I'll need to fix the naming. "Inside BeginFrame" is clearly an overloaded term.

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

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:239: int
last_begin_frame_count_begin_frame_sent_to_main_thread_;
On 2013/09/11 22:30:47, enne wrote:
> I like this a million times better than draw attempt (both in the naming and
in
> the only getting updated in one place), but two different begin frames in the
> same variable name is a bit unfortunate.
> 
> What was wrong with current_frame_number?
HasSentBeginFrameToMainThreadThisFrame
> still uses that language.
> 

I'll change it back to current_frame_number now that we've gotten rid of the
draw_attemp_count_.

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

https://codereview.chromium.org/23796002/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1500: DCHECK(!client_->IsInsideDraw())
On 2013/09/11 22:30:47, enne wrote:
> I think this would be cleaner if you just checked client_->IsInsideDraw and
only
> notified the client if it wasn't.  Then you wouldn't need the notify_client
> parameter or the DCHECK.
> 
> I think that's pretty reasonable as outside of draw, you need to know you need
> another draw.  Inside of draw, you're just about to draw and so it'll get
taken
> care of.
> 
> Alternatively, maybe the client should check its own InsideDraw state and not
> set needs redraw in DidInitializeVisibleTileOnImplThread.

I like your first suggestion and will go with that.

[enne (OOO), 2013-09-12 00:26:45]

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/61001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:207:
PostBeginFrameDeadline(last_begin_frame_args_.frame_time +
On 2013/09/12 00:12:34, Brian Anderson wrote:
> On 2013/09/11 22:30:47, enne wrote:
> > This case almost seems like you shouldn't have a deadline at all.  If
there's
> no
> > need to draw, then why post a deadline at all?  Or, is this what this
comment
> is
> > about?
> 
> We may not have to draw yet, but if a new commit comes in to trigger a draw,
> that draw will have to wait until the next BeginFrame if we don't post the
> deadline here. By posting the deadline task for the next anticipated frame
time
> when the impl thread doesn't have anything it wants to draw, the impl thread
> will always draw as soon as there is a new active tree from the main thread.

Oh! Thanks for the explanation.

[brianderson, 2013-09-12 20:48:54]

ptal.

I've addressed enne's comments and rebased on top of:
https://codereview.chromium.org/23767011 - cc: Always use SetNeedsBeginFrame to
request the next BeginFrame

[enne (OOO), 2013-09-12 21:56:21]

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:144: // Only disable the BeginFrame after a BeginFrame
where we didn't swap,
Can you phrase this in the positive to match the fact that you're setting the
variable to true below? This is really saying that you only need another begin
frame if you swapped?  (And that's because when we don't swap, that's because
we're in a readback, and so we'll move forward and draw and swap again without a
begin frame?  Or, am I mistaken about why this is needed?)

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:148: settings_.using_synchronous_renderer_compositor ?
0 : 1;
This seems like it should really just be if
(!settings_.using_synchronous_renderer_compositor). 
begin_frame_did_not_swap_count is always >= 0, right?

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:149: if (begin_frame_did_not_swap_count_ <
negative_edge_glitch_threshold &&
bikeshed: Could you not put 'not' in a variable name? Maybe just
swap_count_since_last_begin_frame? Or, maybe even
has_swapped_since_last_begin_frame, since you're just checking against 1 here?

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

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:229:
minor_state->SetInteger("consecutive_failed_draws_",
This is trivial, but this whole section seems to have or not have trailing
underscores at random.

[brianderson, 2013-09-12 22:42:53]

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:144: // Only disable the BeginFrame after a BeginFrame
where we didn't swap,
On 2013/09/12 21:56:22, enne wrote:
> Can you phrase this in the positive to match the fact that you're setting the
> variable to true below? This is really saying that you only need another begin
> frame if you swapped?  (And that's because when we don't swap, that's because
> we're in a readback, and so we'll move forward and draw and swap again without
a
> begin frame?  Or, am I mistaken about why this is needed?)

I will change the comment.

It's trying to keep needs_begin_frame_ true if we just swapped to avoid toggling
SetNeedsBeginFrame. Every toggle requires a message to the Browser compositor on
Android (and soon for other platforms), and may cause the next BeginFrame to be
skipped if the BrowserCompositor samples the value of SetNeedsBeginFrame at an
unfortunate time.

We may not have swapped for other reasons: mostly in cases where
ProactiveBeginFrameWanted requested a BeginFrame, but new content wasn't
actually ready yet.

https://codereview.chromium.org/23796002/diff/72001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:149: if (begin_frame_did_not_swap_count_ <
negative_edge_glitch_threshold &&
On 2013/09/12 21:56:22, enne wrote:
> bikeshed: Could you not put 'not' in a variable name? Maybe just
> swap_count_since_last_begin_frame? Or, maybe even
> has_swapped_since_last_begin_frame, since you're just checking against 1 here?

That prevents us from using a larger threshold, but I guess we can change it
later if we need to. I think I can actually put this logic into
ProactiveBeginFrameNeeded, since the state machine should have all the state it
needs already.

[enne (OOO), 2013-09-13 22:28:50]

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
Maybe I just missed this in previous patches, but what is moving around all the
Animate/UpdateAnimationState/ResetCurrentFrameTimeForNextFrame solving here?

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1018: // The commit may have added animations,
requiring us to start
Can you mention that this is only true if impl-side painting is not turned on?

[brianderson, 2013-09-13 22:50:31]

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
On 2013/09/13 22:28:51, enne wrote:
> Maybe I just missed this in previous patches, but what is moving around all
the
> Animate/UpdateAnimationState/ResetCurrentFrameTimeForNextFrame solving here?

Since we have a BeginFrame+deadline interval (in which we can commit, activate,
draw, and swap within a single vsync) I wanted to update the animation time at
the start of the BeginFrame for a few reasons:

1) So that activation of a new commit depends on the animation time of the draw.
Otherwise, I was worried we could activate prematurely and needlessly
checkerboard when we trigger the deadline early.
2) This also allows the RAF frame time to match the draw time when we are in the
low-latency mode.
3) As a followup patch, I want to tie BeginFrameArgs::frame_time in
ThreadProxy::BeginFrameOnImplThread to the animation time, rather than relying
on the LTHI::CurrentFrameTimeTicks and LTHI::CurrrentFrameTime to sample
base::TimeTicks::Now() at a somewhat jittery phase.

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1018: // The commit may have added animations,
requiring us to start
Now that you've pointed that out, is this call even needed? cc_unittests pass
without it.

I forget why this was added. @ajuma, do you remember?

We only background tick if we are not visible. If we are not visible, commits
will be aborted by the main thread and we shouldn't expect any new animations
that would cause us to start background ticking.

Only a readback commit would go through while not visible, and that shouldn't
add animations.

[brianderson, 2013-09-13 22:52:58]

+ajuma for comment regarding the call to UpdateBackgroundAnimateTicking in
ThreadProxy::ScheduledActionCommit.

[enne (OOO), 2013-09-13 23:21:53]

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
With this patch, the animation time is the previous draw time, since you're
animating in BeginFrameOnImplThread and setting the time in
DidBeginFrameDeadlineOnImplThread, which happens afterwards.

It seems like ResetCurrentFrameTime should happen in BeginFrameOnImplThread,
which will then allow you to set the time to be in the future, rather than in
the past?

UpdateAnimationState also seems out of place, because you animate in
BeginFrameOnImplThread and only later in DidBeginFrameDeadline do you activate
animations from the previous draw.  If I'm reading that right, you'll have two
draws with the start frame of animation?  Can this just stay in
DrawSwapReadbackInternal, or at least move to DidBeginFrameOnImplThread prior to
animate (which is sort of where it was previously via
DidAnticipatedDrawTimeChange)?

Maybe ajuma or vollick can sanity check my animation thoughts here.

[brianderson, 2013-09-14 00:09:58]

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
I think the naming of the timing functions is a little confusing (especially
given it's previous usage) but, with this patch, the frame time should get
sampled in BeginFrameOnImplThread. So we will not use a frame time in the past
and we will not have two draws with the start frame of animation.

Reasoning:
LTHI::ResetCurrentFrameTimeForNextFrame() sets the frame time to zero internally
in LTHI, so that the next time LTHI::CurrentFrameTimeTicks() is called it will
sample a new frame time.  The next time LTHI::CurrentFrameTimeTicks() is called
will be inside of ThreadProxy::BeginFrameOnImplThread().

I also think putting UpdateAnimationState inside of
DidBeginFrameDeadlineOnImpleThread makes sense because it ensures we have
balanced calls to Animate and UpdateAnimationState as not every BeginFrame will
result in a draw.

I would also like feedback from @ajuma and @vollick though. The balanced calls
give me a warm fuzzy feeling, but maybe they don't need to be balanced?

[ajuma, 2013-09-14 00:19:39]

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
On 2013/09/14 00:09:59, Brian Anderson wrote:
> I also think putting UpdateAnimationState inside of
> DidBeginFrameDeadlineOnImpleThread makes sense because it ensures we have
> balanced calls to Animate and UpdateAnimationState as not every BeginFrame
will
> result in a draw.
> 
> I would also like feedback from @ajuma and @vollick though. The balanced calls
> give me a warm fuzzy feeling, but maybe they don't need to be balanced?

They should stay balanced. In the old code, we were relying on background
ticking in situations where we were visible but CanDraw was false. Since we're
now foreground ticking in those situations, we need to make sure we
UpdateAnimationState too (so putting the call in
DidBeginFrameDeadlineOnImplThread seems reasonable to me).

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
!layer_tree_host_impl_->CanDraw());
On 2013/09/13 23:21:54, enne wrote:
> UpdateAnimationState also seems out of place, because you animate in
> BeginFrameOnImplThread and only later in DidBeginFrameDeadline do you activate
> animations from the previous draw.  If I'm reading that right, you'll have two
> draws with the start frame of animation? 

The call to Animate moves animations into a Starting state (where they get
ticked at their starting frame). When UpdateAnimationState promotes animations
from started to running, it uses the time of the last tick as the animation's
start time.. So the next time that those animations get ticked, they'll be past
their start frame.

https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1018: // The commit may have added animations,
requiring us to start
On 2013/09/13 22:50:33, Brian Anderson wrote:
> Now that you've pointed that out, is this call even needed? cc_unittests pass
> without it.
> 
> I forget why this was added. @ajuma, do you remember?

This was added because a test was failing in a situation where CanDraw was false
but we were still getting commits; since we background tick only if both CanDraw
is false and there are animations, a commit that added an animation without
causing CanDraw to be true would require us to start background ticking. In the
current state of this CL, it looks like in this situation (where CanDraw is
false but we're visible), we'll continue to foreground tick (since ticking has
moved to BeginFrameOnImplThread), so we won't need to background tick.

[brianderson, 2013-09-14 00:40:39]

On 2013/09/14 00:19:39, ajuma wrote:
> https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
> cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
> !layer_tree_host_impl_->CanDraw());
> On 2013/09/14 00:09:59, Brian Anderson wrote:
> > I also think putting UpdateAnimationState inside of
> > DidBeginFrameDeadlineOnImpleThread makes sense because it ensures we have
> > balanced calls to Animate and UpdateAnimationState as not every BeginFrame
> will
> > result in a draw.
> > 
> > I would also like feedback from @ajuma and @vollick though. The balanced
calls
> > give me a warm fuzzy feeling, but maybe they don't need to be balanced?
> 
> They should stay balanced. In the old code, we were relying on background
> ticking in situations where we were visible but CanDraw was false. Since we're
> now foreground ticking in those situations, we need to make sure we
> UpdateAnimationState too (so putting the call in
> DidBeginFrameDeadlineOnImplThread seems reasonable to me).
> 
>
https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
> cc/trees/thread_proxy.cc:425: frame_did_draw_ ||
> !layer_tree_host_impl_->CanDraw());
> On 2013/09/13 23:21:54, enne wrote:
> > UpdateAnimationState also seems out of place, because you animate in
> > BeginFrameOnImplThread and only later in DidBeginFrameDeadline do you
activate
> > animations from the previous draw.  If I'm reading that right, you'll have
two
> > draws with the start frame of animation? 
> 
> The call to Animate moves animations into a Starting state (where they get
> ticked at their starting frame). When UpdateAnimationState promotes animations
> from started to running, it uses the time of the last tick as the animation's
> start time.. So the next time that those animations get ticked, they'll be
past
> their start frame.
> 
>
https://codereview.chromium.org/23796002/diff/105001/cc/trees/thread_proxy.cc...
> cc/trees/thread_proxy.cc:1018: // The commit may have added animations,
> requiring us to start
> On 2013/09/13 22:50:33, Brian Anderson wrote:
> > Now that you've pointed that out, is this call even needed? cc_unittests
pass
> > without it.
> > 
> > I forget why this was added. @ajuma, do you remember?
> 
> This was added because a test was failing in a situation where CanDraw was
false
> but we were still getting commits; since we background tick only if both
CanDraw
> is false and there are animations, a commit that added an animation without
> causing CanDraw to be true would require us to start background ticking. In
the
> current state of this CL, it looks like in this situation (where CanDraw is
> false but we're visible), we'll continue to foreground tick (since ticking has
> moved to BeginFrameOnImplThread), so we won't need to background tick.

@ajuma: Older versions of the patch were calling BeginFrame to background tick,
but this version of the patch does not. LayerTreeHostImplTimeSourceAdapter calls
Animate and UpdateAnimationState directly. Does that affect your comments at
all?

[ajuma, 2013-09-14 01:38:43]

> @ajuma: Older versions of the patch were calling BeginFrame to background
tick,
> but this version of the patch does not. LayerTreeHostImplTimeSourceAdapter
calls
> Animate and UpdateAnimationState directly. Does that affect your comments at
> all?

There are a few situations to consider:
1a) We're invisible but have no animations
1b) We're invisible and do have an animation
2a) We're visible but CanDraw is false and we have no animations
2b) We're visible but CanDraw is false and we do have an animation
3) We're visible and CanDraw is true

In ToT, we background tick for (1b) and (2b), and we foreground tick in (3). Is
this still the case with the latest version of this patch? If so, then a commit
can cause us to go from state (2a) to (2b), so we'd still need to update
background ticking.

Also, if we're only calling BeginFrameOnImplThread in situation (3), would there
ever be a situation where this wasn't followed by a DrawSwapReadbackInternal?
(If not, we could leave UpdateAnimationState where it is, and still have
balanced calls to Animate and UpdateAnimationState.)

[ajuma, 2013-09-14 01:50:12]

On 2013/09/14 01:38:43, ajuma wrote:
> > @ajuma: Older versions of the patch were calling BeginFrame to background
> tick,
> > but this version of the patch does not. LayerTreeHostImplTimeSourceAdapter
> calls
> > Animate and UpdateAnimationState directly. Does that affect your comments at
> > all?
> 
> There are a few situations to consider:
> 1a) We're invisible but have no animations
> 1b) We're invisible and do have an animation
> 2a) We're visible but CanDraw is false and we have no animations
> 2b) We're visible but CanDraw is false and we do have an animation
> 3) We're visible and CanDraw is true
> 
> In ToT, we background tick for (1b) and (2b), and we foreground tick in (3).
Is
> this still the case with the latest version of this patch? If so, then a
commit
> can cause us to go from state (2a) to (2b), so we'd still need to update
> background ticking.

But we already have a call to do that in ScheduledActionCommit. So, regardless,
we don't need an additional call :) (If I'm remembering right, that additional
call was added to a patch set that had removed the existing call.)

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

Thanks for the animation sanity check.

lgtm, modulo removing that UpdateBackgroundAnimations call that sounds
unnecessary.

[brianderson, 2013-09-17 01:35:52]

The latest patch enables this by default for all Aura platforms as well.

https://codereview.chromium.org/23796002/diff/125001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/23796002/diff/125001/cc/base/switches.cc#newc...
cc/base/switches.cc:202: #if defined(OS_ANDROID) || defined (USE_AURA)
piman: Is it ok to enable by default on Aura using this method?

[commit-bot: I haz the power, 2013-09-17 01:46:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23796002/125001

[commit-bot: I haz the power, 2013-09-17 01:59:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-17 02:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23796002/125001

[commit-bot: I haz the power, 2013-09-17 03:45:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23796002/45001

[commit-bot: I haz the power, 2013-09-17 05:50:09]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-17 06:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23796002/45001

[commit-bot: I haz the power, 2013-09-17 07:13:06]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[brianderson, 2013-09-17 17:13:48]

There was a lot of flakiness last night, so I didn't bother forcing this patch
through.

I'm not very worried about the win_rel flakiness, since it looks like they were
showing up elsewhere.

I am somewhat concerned about the linux_chromeos content_browsertests flakiness
however, since it doesn't look like the failures are showing up elsewhere and
they have to do with readbacks.

[danakj, 2013-09-17 20:26:17]

I made it to the end of the scheduler changes so far, these were my
thoughts/concerns, which we mostly also covered in 1:1 conversations (thanks for
all your explanations)

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/output/begin_fr...
File cc/output/begin_frame_args.cc (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/output/begin_fr...
cc/output/begin_frame_args.cc:55: // and the Renderer Main thread the first 1/3
of a frame to produce output.
nit: mention this assumes 60hz?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/output/output_s...
File cc/output/output_surface.cc (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/output/output_s...
cc/output/output_surface.cc:215: skipped_begin_frame_args_ = BeginFrameArgs();
Are these changes covered by a unit test?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/frame...
File cc/scheduler/frame_rate_controller.cc (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/frame...
cc/scheduler/frame_rate_controller.cc:81: args.deadline += deadline_adjustment_;
Is this covered by a unit test?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/frame...
cc/scheduler/frame_rate_controller.cc:106: void
FrameRateController::SetDeadlineAdjustment(base::TimeDelta delta) {
I find no callers of this function, is it just dead code? Trying to understand
what the deadline adjustment is.

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/frame...
cc/scheduler/frame_rate_controller.cc:125: args.deadline +=
deadline_adjustment_;
why adjust the deadline after instead of just passing in the adjusted deadline?
is this a functional change? if not, the old way reads easier IMO. (same thing
up above)

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
File cc/scheduler/scheduler.cc (left):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler.cc:170: DCHECK(!state_machine_.inside_begin_frame());
Can we dcheck against the begin_frame_state() instead?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
File cc/scheduler/scheduler.cc (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler.cc:145: (state_machine_.begin_frame_state() ==
bool at_end_of_deadline = "this thing" might help clarify things?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler.cc:157: if
(poll_for_draw_triggers_closure_.IsCancelled()) {
As discussed, DCHECK(!needs_begin_frame) with a comment that we should only poll
if BeingFrameNeededByImplThread() is false.

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
File cc/scheduler/scheduler_state_machine.cc (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.cc:408: // we check as late possible and
give the tiles more time to initialize.
as late as possible

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.cc:446: // We shouldn't normally accept
commits if there isn't an OutputSurface.
Only if there's a readback? "shouldn't normally" is kind of vague

How does this moving up here relate to deadline scheduling?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.cc:454: // We should not send the
BeginFrame to the main thread while we are in
Why is this above the forced redraw return true? In that case we've skipped
frames, do we not want to get one up ASAP?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.cc:753: return
!BeginFrameNeededByImplThread() &&
This is just to deal with synchronous compositor right? If the next line is true
then this will always be true except on synchronous compositor? Can this be more
explicit?

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
File cc/scheduler/scheduler_state_machine.h (left):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.h:86: commit_state_ ==
COMMIT_STATE_READY_TO_COMMIT;
This is going to affect the unit tests that do not want to exit until there's no
commit pending:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...

I'm worried this will add flakiness.

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
File cc/scheduler/scheduler_state_machine.h (right):

https://chromiumcodereview.appspot.com/23796002/diff/45001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.h:55: BEGIN_FRAME_STATE_DEADLINE_PENDING,
I am anti-"pending" in these names as they are not clear to me if we're waiting
on something to start, or something that was already started.

Can INSIDE_BEGIN_FRAME be STARTING_BEGIN_FRAME, and the pending one be
INSIDE_BEGIN_FRAME?

Or can we merge into just INSIDE_BEGIN_FRAME?

[brianderson, 2013-09-17 21:08:23]

https://codereview.chromium.org/23796002/diff/45001/cc/output/begin_frame_arg...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/23796002/diff/45001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.cc:55: // and the Renderer Main thread the first 1/3
of a frame to produce output.
On 2013/09/17 20:26:19, danakj wrote:
> nit: mention this assumes 60hz?

Done.

https://codereview.chromium.org/23796002/diff/45001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/23796002/diff/45001/cc/output/output_surface....
cc/output/output_surface.cc:215: skipped_begin_frame_args_ = BeginFrameArgs();
On 2013/09/17 20:26:19, danakj wrote:
> Are these changes covered by a unit test?

No I will add one.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/frame_rate_c...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/frame_rate_c...
cc/scheduler/frame_rate_controller.cc:81: args.deadline += deadline_adjustment_;
On 2013/09/17 20:26:19, danakj wrote:
> Is this covered by a unit test?

There are few ways to handle the first frame's deadline with different trade
offs (faster 1st frame vs lower latency). I'm not sure I want to add a test that
forces a particular implementation.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/frame_rate_c...
cc/scheduler/frame_rate_controller.cc:106: void
FrameRateController::SetDeadlineAdjustment(base::TimeDelta delta) {
On 2013/09/17 20:26:19, danakj wrote:
> I find no callers of this function, is it just dead code? Trying to understand
> what the deadline adjustment is.

It's called in OutputSurface::InitializeBeginFrameEmulation. For the Browser
compositor, the deadline adjustment is 0. FOr the Renderer compositor, the
deadline is currently just BeginFrameArgs::DefaultDeadlineAdjustment()

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/frame_rate_c...
cc/scheduler/frame_rate_controller.cc:125: args.deadline +=
deadline_adjustment_;
On 2013/09/17 20:26:19, danakj wrote:
> why adjust the deadline after instead of just passing in the adjusted
deadline?
> is this a functional change? if not, the old way reads easier IMO. (same thing
> up above)

I used to have an AdjustDeadline method that would make it very obvious where
the deadlines were being modified, but decided it didn't add much value and
removed it. It doesn't make much sense to structure the code the way it is now
without that method, so I will revert to the old way.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:170: DCHECK(!state_machine_.inside_begin_frame());
On 2013/09/17 20:26:19, danakj wrote:
> Can we dcheck against the begin_frame_state() instead?

I moved DCHECKS to OnBeginFrame, OnBeginFrameDeadlinePending, etc - but I can
add them here for more clarity.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:145: (state_machine_.begin_frame_state() ==
On 2013/09/17 20:26:19, danakj wrote:
> bool at_end_of_deadline = "this thing" might help clarify things?

Good suggestion. Done.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:157: if
(poll_for_draw_triggers_closure_.IsCancelled()) {
On 2013/09/17 20:26:19, danakj wrote:
> As discussed, DCHECK(!needs_begin_frame) with a comment that we should only
poll
> if BeingFrameNeededByImplThread() is false.

Done.

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

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:408: // we check as late possible and
give the tiles more time to initialize.
On 2013/09/17 20:26:19, danakj wrote:
> as late as possible

Done.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:446: // We shouldn't normally accept
commits if there isn't an OutputSurface.
On 2013/09/17 20:26:19, danakj wrote:
> Only if there's a readback? "shouldn't normally" is kind of vague
> 
> How does this moving up here relate to deadline scheduling?

Only during readback, yes.

I should move this to a separate patch. It's not directly related to deadline
scheduling. I just noticed that this function was allowing forced draws to
request a commit when we didn't have an output surface, which seemed strange.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:454: // We should not send the
BeginFrame to the main thread while we are in
On 2013/09/17 20:26:19, danakj wrote:
> Why is this above the forced redraw return true? In that case we've skipped
> frames, do we not want to get one up ASAP?

There is a input latency vs quickness to first frame trade off here. Since we've
already janked, the code here tries to reduce latency by waiting for the next
batch of user input that will come before the next begin frame, since begin
inside BEGIN_FRAME_STATE_IDLE means we should be expecting another begin frame
soon.

Optimal user-input latency is dependent on jdduke's BIR work to make sure user
input actually does arrive before the BeginFrame.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:753: return
!BeginFrameNeededByImplThread() &&
On 2013/09/17 20:26:19, danakj wrote:
> This is just to deal with synchronous compositor right? If the next line is
true
> then this will always be true except on synchronous compositor? Can this be
more
> explicit?

Yes. I will make it explicit.

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (left):

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:86: commit_state_ ==
COMMIT_STATE_READY_TO_COMMIT;
On 2013/09/17 20:26:19, danakj wrote:
> This is going to affect the unit tests that do not want to exit until there's
no
> commit pending:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...
> 
> I'm worried this will add flakiness.

Ok. To recap offline discussions: I thought I needed this change a while ago to
ensure that there was actually a begin frame behind a readback, but the main
thread will be blocked in both cases and getting a readback in either state will
not be an issue.

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

https://codereview.chromium.org/23796002/diff/45001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:55: BEGIN_FRAME_STATE_DEADLINE_PENDING,
On 2013/09/17 20:26:19, danakj wrote:
> I am anti-"pending" in these names as they are not clear to me if we're
waiting
> on something to start, or something that was already started.
> 
> Can INSIDE_BEGIN_FRAME be STARTING_BEGIN_FRAME, and the pending one be
> INSIDE_BEGIN_FRAME?
> 
> Or can we merge into just INSIDE_BEGIN_FRAME?

Done.

[brianderson, 2013-09-18 00:14:38]

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

https://codereview.chromium.org/23796002/diff/154001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:456: if (begin_frame_state_ ==
BEGIN_FRAME_STATE_IDLE &&
In browser tests, is there a way to know when a window can accept gestures?

This condition is the reason for browser_test's OverscrollScreenshot flakiness.
It delays the commit until the next BeginFrame to wait for more user input.
Somehow a delayed commit (or some other side effect of that delayed commit) is
racing with the test's injected swipe to navigate backwards and we never
navigate to the expected page.

If I insert a delay in the test before the injected swipe, the flakiness goes
away. If I get rid of this condition, the flakiness also goes away.

I am tempted to just remove this condition, as it will help improve throughput
at the cost of latency. We'll just be kicking the can down the road though for
when we really want to enforce this restriction.

[brianderson, 2013-09-18 02:04:01]

I talked with @sadrul offline and opened up a bug to debug the racy tests after
this patch lands: https://code.google.com/p/chromium/issues/detail?id=293924

I've disabled the tests on ChromeOS for now.

[commit-bot: I haz the power, 2013-09-18 02:04:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23796002/16027

[commit-bot: I haz the power, 2013-09-18 02:18:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[brianderson, 2013-09-18 02:25:37]

piman, I need an OWNERS review of the two tests I disabled in
web_contents_view_aura_browsertest.cc. Do they look okay to you?

[piman, 2013-09-18 06:05:31]

Can we double-check that we're not breaking functionality here?

[brianderson, 2013-09-18 20:08:50]

On 2013/09/18 06:05:31, piman wrote:
> Can we double-check that we're not breaking functionality here?

It looks like there might be an issue with the main thread sometimes not
receiving notification that an animation has ended. See
https://code.google.com/p/chromium/issues/detail?id=293924 for details.

[brianderson, 2013-09-19 00:35:32]

Animations did end up having a bug. It was possible for a commit to come in and
get drawn without having LTHI::Animate called first, which breaks 

I've implemented a hybrid solution now that puts the animation hooks back into
the draw call, but samples the timestamp in the BeginFrameOnImplThread now.

I am unable to reproduce the failures locally anymore so the latest patch no
longer disable any tests.

[danakj, 2013-09-19 00:37:18]

Think a cc_unittest is in order that can test this closer to the source in a
non-flaky way, so we can continue refactoring and know we don't do this again
sooner?

[brianderson, 2013-09-19 19:01:20]

On 2013/09/19 00:37:18, danakj wrote:
> Think a cc_unittest is in order that can test this closer to the source in a
> non-flaky way, so we can continue refactoring and know we don't do this again
> sooner?

In trying to write a test, I'm realizing I don't understand the bug I caused
well enough to know what exactly I was doing wrong. With deadlines disabled by
default, every commit should still have had Animate and UpdateAnimationState
called on it around a draw. So a commit being drawn without Animate being called
on it wasn't the issue like I thought it was.

A couple of the threaded animation webkit layout tests were failing in a more
direct (but still flaky) manner. I had ignored the layout tests until I ran into
the other animation issue. The layout tests were failing because the readback
came back with an unexpected frame of the animation, most likely because I was
animating even if we didn't draw.

Fixing the layout tests happened to fix the flake in the gesture navigation
tests, but @ajuma, @sadrul, and I were all stumped regarding how Impl-side
animation behavior could affect those tests since the UI thread is supposed to
trigger everything on its own.  We don't have a root cause for this issue, so
I'm not sure what to test here.

For the layout tests, debugging and fixing those were more straightforward. To
fail closer to the source, we could add checks in cc_unittests that make sure we
only animate when we draw, but we'd have to condition it on whether we are
visible or not to allow background animating. I would be hesitant to enforce
this rule unless the HTML spec specifically says that certain frames of the
animation must be drawn though.

[ajuma, 2013-09-19 19:13:56]

On 2013/09/19 19:01:20, Brian Anderson wrote:
> For the layout tests, debugging and fixing those were more straightforward. To
> fail closer to the source, we could add checks in cc_unittests that make sure
we
> only animate when we draw, but we'd have to condition it on whether we are
> visible or not to allow background animating. I would be hesitant to enforce
> this rule unless the HTML spec specifically says that certain frames of the
> animation must be drawn though.

I think we want to make sure that after we commit, we don't UpdateAnimationState
before the next call to Animate. I don't think we need to disallow calls to
Animate when we aren't drawing (since, e.g., we do want to call Animate when
we're skipping frames because of checkerboarding).

The animations spec doesn't go into this sort of detail (that's why we can do
things like skip frames and delay animation start times when checkerboarding).

[brianderson, 2013-09-19 19:14:22]

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

https://codereview.chromium.org/23796002/diff/174001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:456: if (begin_frame_state_ ==
BEGIN_FRAME_STATE_IDLE &&
This condition hurts the fallback path compared to ToT, so I'm working on a new
patch set that will only enable this if deadline_scheduling is enabled for now.

Unfortunately, this causes the order of actions and what triggers those actions
to change when deadline scheduling is enabled vs disabled, so I need to modify
the Scheduler tests. (In the current patch set, only timing is affected.)

[danakj, 2013-09-19 19:17:41]

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

https://codereview.chromium.org/23796002/diff/174001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:456: if (begin_frame_state_ ==
BEGIN_FRAME_STATE_IDLE &&
On 2013/09/19 19:14:24, Brian Anderson wrote:
> This condition hurts the fallback path compared to ToT, so I'm working on a
new
> patch set that will only enable this if deadline_scheduling is enabled for
now.
> 
> Unfortunately, this causes the order of actions and what triggers those
actions
> to change when deadline scheduling is enabled vs disabled, so I need to modify
> the Scheduler tests. (In the current patch set, only timing is affected.)

What do you mean by hurts? This sounds really unfortunate, and like we'll be
doing the wrong thing (and testing for it) when we do turn on deadline?

[brianderson, 2013-09-19 19:57:56]

On 2013/09/19 19:17:41, danakj wrote:
>
https://codereview.chromium.org/23796002/diff/174001/cc/scheduler/scheduler_s...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/23796002/diff/174001/cc/scheduler/scheduler_s...
> cc/scheduler/scheduler_state_machine.cc:456: if (begin_frame_state_ ==
> BEGIN_FRAME_STATE_IDLE &&
> On 2013/09/19 19:14:24, Brian Anderson wrote:
> > This condition hurts the fallback path compared to ToT, so I'm working on a
> new
> > patch set that will only enable this if deadline_scheduling is enabled for
> now.
> > 
> > Unfortunately, this causes the order of actions and what triggers those
> actions
> > to change when deadline scheduling is enabled vs disabled, so I need to
modify
> > the Scheduler tests. (In the current patch set, only timing is affected.)
> 
> What do you mean by hurts? This sounds really unfortunate, and like we'll be
> doing the wrong thing (and testing for it) when we do turn on deadline?

For RAF based content, this condition works really well with or without deadline
enabled because the next commit will still start immediately after the draw,
like ToT. If the SetNeedsCommit comes in after the draw however, that commit
won't start until the next vsync and it doesn't get a chance to draw until the
next next vsync. With local testing, this caused me to drop to 30 fps more often
that ToT behavior.

This is not an issue when there is a deadline interval because a new active tree
can still trigger the draw anywhere in between the BeginFrame and the deadline.
The next commit can start anywhere between the BeginFrame and deadline as well.

I am worried about testing of this being enabled vs disabled too. I was in the
process of adding enabled vs disabled tests to the Scheduler unit tests, but
this flag would also affect the behavior of the LTH unit tests.  But if the LTH
unit tests are also sensitive to timing, I think we'd need to add another
dimension to them anyway to make sure we are always testing both configs and not
just the default.

[brianderson, 2013-09-19 21:58:24]

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

https://codereview.chromium.org/23796002/diff/174001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:456: if (begin_frame_state_ ==
BEGIN_FRAME_STATE_IDLE &&
I was wondering how I could have missed this 30fps behavior before and found it
depends on whether Ubercomp or Mailboxes are enabled or not.

Ubercomp was disabled by default recently and I was previously testing mostly
with mailboxes enabled because that is the configuration used by Android.

Without Ubercomp or Mailboxes, the delay through the GPU process causes us to
drop to 30fps much more often if we also delay the commit here in the Browser
compositor. (The Renderer's compositor doesn't have the same issue.)

I should really only disable this condition in the Browser compositor, when
Ubercomp and Mailboxes are not being used, and deadline scheduling is disabled.

That doesn't really help our test situation though, until we can ensure that
Ubercomp or Mailboxes are always used with the Browser compositor.

[brianderson, 2013-09-20 00:37:46]

I got the base files missing error. Moving to:
https://codereview.chromium.org/24304002