cc: Handle retroactive BeginFrames in the Scheduler.

cc: Handle retroactive BeginFrames in the Scheduler.

This will be the first step allowing us to pull logic out
of the OutputSurface.

Initially, the retroactive logic will exist in both the
OutputSurface and the scheduler. Then, as all logic that
depends on the retroactive logic has moved to the
scheduler, I will remove the retroactive logic in
OutputSurface.

There are now 4 BeginFrames in the scheduler:
1) BeginFrame: from the OutputSurface.
2) BeginRetroFrame: a deferred BeginFrame.
3) BeginImplFrame: starts a compositor frame.
4) BeginMainFrame: starts a main frame.

BUG=358409
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263435

[brianderson, 2014-03-31 23:18:18]

ptal.

Scheduler tests for the retroactive logic pending.

[Sami, 2014-04-01 13:09:09]

Nice, I think this is where the logic really belongs. I was wondering if we need
a setting for this to avoid breaking WebView?

https://codereview.chromium.org/218633010/diff/1/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/218633010/diff/1/cc/output/output_surface.cc#...
cc/output/output_surface.cc:185:
check_for_retroactive_begin_impl_frame_pending_)
Rename this flag too?

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

https://codereview.chromium.org/218633010/diff/1/cc/output/output_surface.h#n...
cc/output/output_surface.h:179: BeginFrameArgs skipped_begin_impl_frame_args_;
skipped_begin_frame_args_?

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:160: // Handle Retroactive BeginFrames
nit: Add '.'.

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:163: begin_retro_frame_args_.IsValid()) {
Do we also need to check for needs_begin_frame?

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:156: BeginFrameArgs begin_frame_args_;
It seems like we could do without this field and instead pass the args to
BeginImplFrame etc. directly. Not sure if it's clearer than what we have now,
but it might make the data flow easier to see. WDYT?

[brianderson, 2014-04-01 19:12:53]

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:163: begin_retro_frame_args_.IsValid()) {
On 2014/04/01 13:09:10, Sami wrote:
> Do we also need to check for needs_begin_frame?

That would be better, yes. I was thinking that we could get the post task out of
the way just in case needs_begin_frame does become true soon, but that would
make us decide the deadline with less information than we'd otherwise have.

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:156: BeginFrameArgs begin_frame_args_;
On 2014/04/01 13:09:10, Sami wrote:
> It seems like we could do without this field and instead pass the args to
> BeginImplFrame etc. directly. Not sure if it's clearer than what we have now,
> but it might make the data flow easier to see. WDYT?

I liked the idea of having access to the original BeginFrame deadline in
addition to the adjusted BeginImplFrame deadline.

I'll change begin_frame_args_ to a deque, which will make the data flow very
explicit and also make long deadlines easier to implement in the future.

[brianderson, 2014-04-02 04:15:56]

PTAL. Tests fixed and new test added.

The retroactive logic in the scheduler is effectively dead in production code
and is only used in the new scheduler test.

I tried porting just the retroactive SetNeedsBeginFrame logic in the first few
patch sets, but ran into issues. It's possible for a BeginFrame to slip out of
the OutputSurface that should have been swap throttled. Once it slips through,
the scheduler will handle that BeginRetroFrame even if the OutputSurface is swap
throttled. I think I'll need to port all use cases at once in a followup patch,
instead of doing each case one by one.

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:163: begin_retro_frame_args_.IsValid()) {
On 2014/04/01 19:12:53, brianderson wrote:
> On 2014/04/01 13:09:10, Sami wrote:
> > Do we also need to check for needs_begin_frame?
> 
> That would be better, yes. I was thinking that we could get the post task out
of
> the way just in case needs_begin_frame does become true soon, but that would
> make us decide the deadline with less information than we'd otherwise have.

Done.

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

https://codereview.chromium.org/218633010/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:156: BeginFrameArgs begin_frame_args_;
On 2014/04/01 19:12:53, brianderson wrote:
> On 2014/04/01 13:09:10, Sami wrote:
> > It seems like we could do without this field and instead pass the args to
> > BeginImplFrame etc. directly. Not sure if it's clearer than what we have
now,
> > but it might make the data flow easier to see. WDYT?
> 
> I liked the idea of having access to the original BeginFrame deadline in
> addition to the adjusted BeginImplFrame deadline.
> 
> I'll change begin_frame_args_ to a deque, which will make the data flow very
> explicit and also make long deadlines easier to implement in the future.

I ended up getting rid of begin_frame_args_ like you suggested. We can store it
if we need it in the future. I still created a deque of begin_retro_frame_args
though.

[brianderson, 2014-04-02 16:57:49]

https://codereview.chromium.org/218633010/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:250: now <
Yay for unit tests. This should have been a >. Fixed in most recent patch.

https://codereview.chromium.org/218633010/diff/60001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/218633010/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:191:
scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
I added an test task runner so we can make sure the deadline and BeginRetroFrame
tasks are properly posted.

[brianderson, 2014-04-02 22:12:27]

Now depends on:
https://codereview.chromium.org/222413004
https://codereview.chromium.org/222003007
https://codereview.chromium.org/222743004

[Sami, 2014-04-03 15:55:58]

Thanks for splitting this up a bit. Overall I think this looks great --
especially the idea to use a task runner. Just a few questions below.

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface...
cc/output/output_surface.cc:209: client_ready_for_begin_frame_ = true;
Re: this and DidLoseOutputSurface -- not sure why we needed to chance these two
places? Does this change the swap throttling behavior somehow?

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:180: if (needs_begin_frame &&
!begin_retro_frame_posted_ &&
Not sure if you saw my earlier question: do we need to disable this for WebView?

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:187: base::Bind(&Scheduler::BeginRetroFrame,
weak_factory_.GetWeakPtr()));
While we're at it should we make this a pre-created closure like the other ones?

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:249: DCHECK(!begin_retro_frame_args_.empty());
DCHECK(begin_retro_frame_posted_)?

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:255:
AdjustedBeginImplFrameDeadline(begin_retro_frame_args_.front())) {
Side note: the client call to compute the estimated draw duration recomputes the
duration percentile from scratch every time from all the samples it has. We
might want to optimize it at some point.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:284: begin_impl_frame_args_ = args;
Do we need to move this down for a particular reason? The skipping logic above
will use the previous frame's timings to skip in the current frame, which isn't
too bad but seems weird.

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

https://codereview.chromium.org/218633010/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1487: // when we draw.
Neat!

[brianderson, 2014-04-03 16:36:17]

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface...
cc/output/output_surface.cc:209: client_ready_for_begin_frame_ = true;
On 2014/04/03 15:55:58, Sami wrote:
> Re: this and DidLoseOutputSurface -- not sure why we needed to chance these
two
> places? Does this change the swap throttling behavior somehow?

Good catch. They shouldn't be needed. I'll try removing them.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:180: if (needs_begin_frame &&
!begin_retro_frame_posted_ &&
On 2014/04/03 15:55:58, Sami wrote:
> Not sure if you saw my earlier question: do we need to disable this for
WebView?

Yes, I need to disable this for WebView.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:187: base::Bind(&Scheduler::BeginRetroFrame,
weak_factory_.GetWeakPtr()));
On 2014/04/03 15:55:58, Sami wrote:
> While we're at it should we make this a pre-created closure like the other
ones?

Oops. Yes.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:249: DCHECK(!begin_retro_frame_args_.empty());
On 2014/04/03 15:55:58, Sami wrote:
> DCHECK(begin_retro_frame_posted_)?

Good idea.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:255:
AdjustedBeginImplFrameDeadline(begin_retro_frame_args_.front())) {
On 2014/04/03 15:55:58, Sami wrote:
> Side note: the client call to compute the estimated draw duration recomputes
the
> duration percentile from scratch every time from all the samples it has. We
> might want to optimize it at some point.

I will pull out the duration calculation.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:284: begin_impl_frame_args_ = args;
On 2014/04/03 15:55:58, Sami wrote:
> Do we need to move this down for a particular reason? The skipping logic above
> will use the previous frame's timings to skip in the current frame, which
isn't
> too bad but seems weird.

Bad cherry-pick. Will move back up.

[brianderson, 2014-04-03 21:55:55]

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/output/output_surface...
cc/output/output_surface.cc:209: client_ready_for_begin_frame_ = true;
On 2014/04/03 15:55:58, Sami wrote:
> Re: this and DidLoseOutputSurface -- not sure why we needed to chance these
two
> places? Does this change the swap throttling behavior somehow?

Done.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:180: if (needs_begin_frame &&
!begin_retro_frame_posted_ &&
On 2014/04/03 15:55:58, Sami wrote:
> Not sure if you saw my earlier question: do we need to disable this for
WebView?

Done.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:187: base::Bind(&Scheduler::BeginRetroFrame,
weak_factory_.GetWeakPtr()));
On 2014/04/03 15:55:58, Sami wrote:
> While we're at it should we make this a pre-created closure like the other
ones?

Done.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:249: DCHECK(!begin_retro_frame_args_.empty());
On 2014/04/03 15:55:58, Sami wrote:
> DCHECK(begin_retro_frame_posted_)?

Done.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:255:
AdjustedBeginImplFrameDeadline(begin_retro_frame_args_.front())) {
On 2014/04/03 15:55:58, Sami wrote:
> Side note: the client call to compute the estimated draw duration recomputes
the
> duration percentile from scratch every time from all the samples it has. We
> might want to optimize it at some point.

Done.

https://codereview.chromium.org/218633010/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:284: begin_impl_frame_args_ = args;
On 2014/04/03 15:55:58, Sami wrote:
> Do we need to move this down for a particular reason? The skipping logic above
> will use the previous frame's timings to skip in the current frame, which
isn't
> too bad but seems weird.

Done.

[brianderson, 2014-04-07 18:02:21]

https://codereview.chromium.org/218633010/diff/100001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/100001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:248: if
(settings_.using_synchronous_renderer_compositor ||
This is where I prevent WebView from using the retroactive logic and the DCHECK
on line 237 double checks we don't use the retroactive logic.

[Sami, 2014-04-08 11:25:25]

Thanks, lgtm!

https://codereview.chromium.org/218633010/diff/120001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/218633010/diff/120001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:1367: args.deadline +=
base::TimeDelta::FromHours(1);
I wish all our deadlines were this long :)

https://codereview.chromium.org/218633010/diff/120001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/218633010/diff/120001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:454: DCHECK(IsImplThread());
nit: Could remove this DCHECK since impl() also does it.

[brianderson, 2014-04-08 19:45:45]

Piman, can I get an OWNERS review for content?

[piman, 2014-04-08 21:22:53]

lgtm

[brianderson, 2014-04-08 21:38:06]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2014-04-08 21:38:46]

The CQ bit was unchecked by brianderson@chromium.org

[danakj, 2014-04-08 22:52:42]

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:182: if (needs_begin_frame)
Can you expand the comment to explain this if-check and its relation to the
posted BeginFrame that the OutputSurface will produce just above this.

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:268: while (!begin_retro_frame_args_.empty() &&
LGTM with the feedback from the convo we had in chat. It does not seem super
clear from reading this code why/how we guarantee that this queue eventually
drains rather than always running the previously posted retro frame forever and
never running the current frame as a result.

[danakj, 2014-04-08 22:53:10]

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:182: if (needs_begin_frame)
On 2014/04/08 22:52:42, danakj wrote:
> Can you expand the comment to explain this if-check and its relation to the
> posted BeginFrame that the OutputSurface will produce just above this.

(Or possibly this should be last_set_needs_begin_frame_ and a test to show why)

[brianderson, 2014-04-10 18:27:11]

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:182: if (needs_begin_frame)
On 2014/04/08 22:53:11, danakj wrote:
> On 2014/04/08 22:52:42, danakj wrote:
> > Can you expand the comment to explain this if-check and its relation to the
> > posted BeginFrame that the OutputSurface will produce just above this.
> 
> (Or possibly this should be last_set_needs_begin_frame_ and a test to show
why)

I will expand this comment and use last_set_needs_begin_frame_ to keep this
condition more consistent with Scheduler::BeginFrame.

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:249: (last_set_needs_begin_frame_ &&
begin_retro_frame_args_.empty() &&
To recap my chat with Dana here:

I will check explicitly for begin_retro_frame_posted_ here so it's clearer that
there isn't a race condition between a newly arriving BeginFrame and a
BeginRetroFrame.

begin_retro_frame_args_.empty() already guards against the race condition
because it implies begin_retro_frame_posted_ is false, but it's better to be
explicit.

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:254: begin_retro_frame_args_.push_back(args);
In my chat with Dana, I suggested that unconditionally queuing the
BeginFramesArgs might simplify the data flow. After thinking about it some more,
however, I think that we want to treat the front BeginFrameArgs as a special
case often enough that keeping it in the queue constrains us more than it helps.

For example, if we want to limit the size of the queue, we'd pop the front
BeginFrameArgs which would be weird if we were currently using the front
BeginFrameArgs. Also, I don't want to always expire all BeginFrame args if the
deadline is consistently in the past.

https://codereview.chromium.org/218633010/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:268: while (!begin_retro_frame_args_.empty() &&
On 2014/04/08 22:52:43, danakj wrote:
> LGTM with the feedback from the convo we had in chat. It does not seem super
> clear from reading this code why/how we guarantee that this queue eventually
> drains rather than always running the previously posted retro frame forever
and
> never running the current frame as a result.

Today, we should always end up with only one un-expired BeginRetroFrame because
deadlines will not be greater than the next frame time. I could add a DCHECK,
but am afraid of systems with sometimes non-monotonic timestamps triggering
false positives.

In the future, long deadlines could result in us not draining the queue if we
don't catch up. But if we consistently can't catch up, our fallback should be to
lower our frame rate, which we are planning to solve separately.

I will add a comment.

[brianderson, 2014-04-10 19:26:59]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-10 19:27:03]

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

[commit-bot: I haz the power, 2014-04-10 20:40:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:40:55]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[brianderson, 2014-04-10 21:38:21]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-10 21:39:23]

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

[commit-bot: I haz the power, 2014-04-11 18:08:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 18:08:28]

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

[brianderson, 2014-04-11 22:39:29]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-11 22:41:10]

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

[commit-bot: I haz the power, 2014-04-12 01:07:18]

Change committed as 263435