Hoist begin frame sources out of scheduler

Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.

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

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b9bf377e238af579e4b6dd01cb4007ba712da742
Cr-Commit-Position: refs/heads/master@{#381558}

[enne (OOO), 2016-03-03 23:04:21]

Description was changed from

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.
==========

to

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-03 23:04:34]

enne@chromium.org changed reviewers:
+ skyostil@chromium.org, sunnyps@chromium.org

[enne (OOO), 2016-03-03 23:04:57]

Description was changed from

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.

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

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-03 23:06:14]

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.cc#o...
cc/scheduler/scheduler.cc:807:
state->SetDouble("authoritative_vsync_interval_ms",
I'm not sure if anybody was using this info, but I'm not sure where it would go
now, so have just removed it.

[Sami, 2016-03-08 19:15:25]

Less magic in the scheduler == good. Thanks for the cleanup, lgtm with assorted
nits.

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.cc#o...
cc/scheduler/scheduler.cc:807:
state->SetDouble("authoritative_vsync_interval_ms",
On 2016/03/03 23:06:14, enne wrote:
> I'm not sure if anybody was using this info, but I'm not sure where it would
go
> now, so have just removed it.

I think it was here just for debugging, so no need to keep it around.

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

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.h#ne...
cc/scheduler/scheduler.h:58: static scoped_ptr<Scheduler> Create(
Could you add a note saying both |client| and |begin_frame_source| need to
outlive the scheduler?

https://codereview.chromium.org/1765723002/diff/1/cc/scheduler/scheduler.h#ne...
cc/scheduler/scheduler.h:159: BeginFrameSource* frame_source_;
nit: Could you add "  // Not owned." to make it clear we don't manage the
source's lifetime?

double-nit: maybe also rename to begin_frame_source_ for consistency?

https://codereview.chromium.org/1765723002/diff/1/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1765723002/diff/1/cc/trees/proxy_impl.h#newco...
cc/trees/proxy_impl.h:155: scoped_ptr<SyntheticBeginFrameSource>
synthetic_frame_source_;
nit: synthetic_begin_frame_source_

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_proxy.h
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_prox...
cc/trees/single_thread_proxy.h:147: scoped_ptr<BeginFrameSource>
begin_frame_source_;
Sorry to latch onto the naming but I would keep this as
external_begin_frame_source_. Otherwise it's not really obvious when this would
be set vs. the synthetic one. WDYT? (same comment for proxy_impl.h)

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_prox...
cc/trees/single_thread_proxy.h:148: scoped_ptr<SyntheticBeginFrameSource>
synthetic_frame_source_;
nit: synthetic_begin_frame_source_

[enne (OOO), 2016-03-08 21:38:02]

Thanks for the review!  You may also want to look at
https://codereview.chromium.org/1762823002 which this depends on.

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_proxy.h
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_prox...
cc/trees/single_thread_proxy.h:147: scoped_ptr<BeginFrameSource>
begin_frame_source_;
On 2016/03/08 at 19:15:25, Sami wrote:
> Sorry to latch onto the naming but I would keep this as
external_begin_frame_source_. Otherwise it's not really obvious when this would
be set vs. the synthetic one. WDYT? (same comment for proxy_impl.h)

This ends up being either the external one or the throttled one.  Would you
rather I have three better named variables, only one of which is valid?

[enne (OOO), 2016-03-08 23:19:11]

On 2016/03/08 at 21:38:02, enne wrote:

> This ends up being either the external one or the throttled one.  Would you
rather I have three better named variables, only one of which is valid?

Ended up just doing this in the latest patch, which I think ends up looking
cleaner.

[sunnyps, 2016-03-09 00:14:57]

Now that we're not using BeginFrameSourceMultiplexer, can we remove that in this
CL?

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

https://codereview.chromium.org/1765723002/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:264: } else if
(scheduler_settings_.use_external_begin_frame_source) {
use_external_begin_frame_source doesn't need to be a scheduler setting any more,
right?

[enne (OOO), 2016-03-09 01:17:05]

On 2016/03/09 at 00:14:57, sunnyps wrote:
> Now that we're not using BeginFrameSourceMultiplexer, can we remove that in
this CL?

Done!! :)

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

https://codereview.chromium.org/1765723002/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:264: } else if
(scheduler_settings_.use_external_begin_frame_source) {
On 2016/03/09 at 00:14:56, sunnyps wrote:
> use_external_begin_frame_source doesn't need to be a scheduler setting any
more, right?

It's a little bit more involved.  There's a layer tree setting too.  Once
OutputSurface can specify a begin frame source, I'll move the existing external
begin frame source logic in render widget into an output surface and then all
this can go away, I think.

[Sami, 2016-03-09 14:36:23]

Still lgtm.

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_proxy.h
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/1765723002/diff/1/cc/trees/single_thread_prox...
cc/trees/single_thread_proxy.h:147: scoped_ptr<BeginFrameSource>
begin_frame_source_;
On 2016/03/08 21:38:02, enne wrote:
> On 2016/03/08 at 19:15:25, Sami wrote:
> > Sorry to latch onto the naming but I would keep this as
> external_begin_frame_source_. Otherwise it's not really obvious when this
would
> be set vs. the synthetic one. WDYT? (same comment for proxy_impl.h)
> 
> This ends up being either the external one or the throttled one.  Would you
> rather I have three better named variables, only one of which is valid?

The new design with precise names for all the three alternatives looks pretty
clear to me.

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

https://codereview.chromium.org/1765723002/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:759: state->SetBoolean("observing_frame_source",
observing_begin_frame_source_);
nit: please update the string to match the new name too.

[enne (OOO), 2016-03-09 18:02:42]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-09 18:02:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1765723002/#ps60001
(title: "Fix trace name")

[enne (OOO), 2016-03-09 18:02:54]

The CQ bit was unchecked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-09 18:02:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765723002/60001

[enne (OOO), 2016-03-09 21:01:26]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-09 21:02:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765723002/60001

[commit-bot: I haz the power, 2016-03-09 21:39:28]

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

[commit-bot: I haz the power, 2016-03-09 21:39:29]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[enne (OOO), 2016-03-10 19:39:55]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-10 19:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765723002/60001

[enne (OOO), 2016-03-10 20:47:58]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-03-10 22:38:19]

Thanks android webview tests for catching a legit dcheck!

Not using the multiplexer did remove one piece of logic, which is where it
discards any begin frames that are in the past relative to the last begin frame
time.  I suspect the OnUpdateVsyncParameters is responsible for creating a weird
time, but I'm not 100% sure.

I need to look at this more closely because this logic isn't something I
understand very well.  One possibility would be to have
BeginFrameObserverBase::OnBeginFrame do this same dropping (instead of
dchecking).

[enne (OOO), 2016-03-10 22:39:06]

More specifically the DCHECK that's being hit is
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...

[enne (OOO), 2016-03-14 23:33:13]

DCHECK should be fixed by https://codereview.chromium.org/1789803003

[enne (OOO), 2016-03-14 23:33:18]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-14 23:33:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1765723002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-14 23:33:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765723002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765723002/80001

[commit-bot: I haz the power, 2016-03-15 00:46:50]

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

[commit-bot: I haz the power, 2016-03-15 00:46:51]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[enne (OOO), 2016-03-16 20:02:38]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-16 20:03:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765723002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765723002/80001

[commit-bot: I haz the power, 2016-03-16 21:51:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-16 21:53:12]

Description was changed from

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.

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

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hoist begin frame sources out of scheduler

To avoid cc::Scheduler having to sometimes own / sometimes create /
switch between sources / manage parameters on sources, hoist all this
logic up to the owners of cc::Scheduler.

This refactoring makes it possible to eventually allow an OutputSurface
to set a BeginFrameSource on its client and then forward that to the
scheduler to use.  That future is made easier by the scheduler having a
single begin frame source managed by its owner.

This patch adds a bit of duplicated logic in three places (single thread
proxy / threaded proxy / tests) for the creation of the begin frame
source.  However, the single thread and threaded versions were already
doing different things (re: authoritative vsync signals).  The hope
is that the vsync information and begin frame source creation for the
synthetic sources can get further hoisted into OutputSurface itself.

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

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b9bf377e238af579e4b6dd01cb4007ba712da742
Cr-Commit-Position: refs/heads/master@{#381558}
==========

[commit-bot: I haz the power, 2016-03-16 21:53:13]

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