cc: Introduce forward_begin_frames_to_children and use_external_begin_frame_source

cc: Introduce forward_begin_frames_to_children and use_external_begin_frame_source

This flag is added into the LayerTreeSettings and SchedulerSettings.
This is a small piece of unified BeginFrame scheduling.
forward_begin_frames_to_children will be true when scheduler acts as a BeginFrame publisher
for child Scheduler.
begin_frame_scheduling_enabled is replaced with use_external_begin_frame_source for more
descriptive.
Also, same default value to use_external_begin_frame_source is used on LayerTreeSettings and
SchedulerSettings for less confusing.

This is dependent on https://codereview.chromium.org/619843002/.

R=brianderson@chromium.org
BUG=372086
Committed: https://crrev.com/2880b8edb63e0bb201d045bbbe32d185a9a3c6a3
Cr-Commit-Position: refs/heads/master@{#303743}

[simonhong, 2014-11-04 22:51:57]

simonhong@chromium.org changed reviewers:
+ piman@chromium.org

[simonhong, 2014-11-04 22:51:57]

brianderson@, this is the re-start of previous unified BeginFrame work.
This will be uploaded after "separate ipc for BeginFrame...".
PTAL.

[brianderson, 2014-11-04 23:49:17]

lgtm

[mithro-old, 2014-11-05 00:24:06]

On 2014/11/04 23:49:17, brianderson wrote:

Couple of questions based on your description;

> This two flags are added into the LayerTreeSettings and SchedulerSettings.
> This is a small piece of unified BeginFrame scheduling.

Thank you for working on this and also for splitting parts out into smaller
patches. It's greatly appreciated!

The final goal here is to have the Scheduler use an external BeginFrameSource on
all platforms (except when vsync throttling is disabled), right?

> begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> begin_frame_receiver will be on when scheduler uses external BeginFrameSource.

Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver? 

> begin_frame_publisher will be on when scheduler acts as a BeginFrame publisher
for child Scheduler.

Do you mean "begin_frame_publisher will be on when the scheduler acts as a
BeginFrame publisher for itself rather than using an external BeginFrameSource"?

Tim 'mithro' Ansell

[piman, 2014-11-05 00:39:14]

lgtm

https://codereview.chromium.org/696413003/diff/1/cc/scheduler/scheduler_setti...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/696413003/diff/1/cc/scheduler/scheduler_setti...
cc/scheduler/scheduler_settings.cc:13: : begin_frame_receiver(true),
nit: it would be good to have the same default here and in LayerTreeSettings.
Less confusion.

[simonhong, 2014-11-05 00:54:38]

On 2014/11/05 00:24:06, mithro wrote:
> On 2014/11/04 23:49:17, brianderson wrote:
> 
> Couple of questions based on your description;
> 
> > This two flags are added into the LayerTreeSettings and SchedulerSettings.
> > This is a small piece of unified BeginFrame scheduling.
> 
> Thank you for working on this and also for splitting parts out into smaller
> patches. It's greatly appreciated!
> 
> The final goal here is to have the Scheduler use an external BeginFrameSource
on
> all platforms (except when vsync throttling is disabled), right?

Yes, you're right.
Also, one more thing is making the browser-side scheduler as a publisher of
BeginFrame
to child scheduler (renderer-side scheduler).

> 
> > begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> > begin_frame_receiver will be on when scheduler uses external
BeginFrameSource.
> 
> Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver? 

Scheduler on renderer-side will always act as a BeginFrame receiver.
Scheduler on browser-side will act as both (receiver and publisher).

> 
> > begin_frame_publisher will be on when scheduler acts as a BeginFrame
publisher
> for child Scheduler.
> 
> Do you mean "begin_frame_publisher will be on when the scheduler acts as a
> BeginFrame publisher for itself rather than using an external
BeginFrameSource"?

Yes, both are different.
When scheduler acts as a publisher, it publishes new BeginFrame for child
scheduler (exactly renderer-side scheduler).

> 
> Tim 'mithro' Ansell

[brianderson, 2014-11-05 00:55:35]

On 2014/11/05 00:24:06, mithro wrote:
> On 2014/11/04 23:49:17, brianderson wrote:
> 
> Couple of questions based on your description;
> 
> > This two flags are added into the LayerTreeSettings and SchedulerSettings.
> > This is a small piece of unified BeginFrame scheduling.
> 
> Thank you for working on this and also for splitting parts out into smaller
> patches. It's greatly appreciated!
> 
> The final goal here is to have the Scheduler use an external BeginFrameSource
on
> all platforms (except when vsync throttling is disabled), right?

For the Renderer's Scheduler, yes. For the Browser's Scheduler, however, we will
likely never have an external BeginFrameSource for Linux or Windows.

> 
> > begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> > begin_frame_receiver will be on when scheduler uses external
BeginFrameSource.
> 
> Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver?

This flag is supposed to mean "ViewMsg_BeginFrame receiver", which is slightly
different. Should we rename the flags to begin_frame_message_receiver and
begin_frame_message_publisher to reduce confusion?

> 
> > begin_frame_publisher will be on when scheduler acts as a BeginFrame
publisher
> for child Scheduler.
> 
> Do you mean "begin_frame_publisher will be on when the scheduler acts as a
> BeginFrame publisher for itself rather than using an external
BeginFrameSource"?

What Simon says I think is the correct intention.

begin_frame_message_receiver being false will mean that the scheduler acts as a
BeginFrame publisher for itself.
begin_frame_message_publisher will indicate whether the scheduler acts as a
BeginFrame publisher for child compositors.

I guess the "message" also helps draw the distinction between self and child.
Definitely think we should rename the flags now.

> 
> Tim 'mithro' Ansell

[simonhong, 2014-11-05 01:03:59]

On 2014/11/05 00:55:35, brianderson wrote:
> On 2014/11/05 00:24:06, mithro wrote:
> > On 2014/11/04 23:49:17, brianderson wrote:
> > 
> > Couple of questions based on your description;
> > 
> > > This two flags are added into the LayerTreeSettings and SchedulerSettings.
> > > This is a small piece of unified BeginFrame scheduling.
> > 
> > Thank you for working on this and also for splitting parts out into smaller
> > patches. It's greatly appreciated!
> > 
> > The final goal here is to have the Scheduler use an external
BeginFrameSource
> on
> > all platforms (except when vsync throttling is disabled), right?
> 
> For the Renderer's Scheduler, yes. For the Browser's Scheduler, however, we
will
> likely never have an external BeginFrameSource for Linux or Windows.

Ah, I was confused.
On aura, browser-side scheduler will use synthetic source.
Others (android and mac) will use external BFS.

> 
> > 
> > > begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> > > begin_frame_receiver will be on when scheduler uses external
> BeginFrameSource.
> > 
> > Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver?
> 
> This flag is supposed to mean "ViewMsg_BeginFrame receiver", which is slightly
> different. Should we rename the flags to begin_frame_message_receiver and
> begin_frame_message_publisher to reduce confusion?
> 
> > 
> > > begin_frame_publisher will be on when scheduler acts as a BeginFrame
> publisher
> > for child Scheduler.
> > 
> > Do you mean "begin_frame_publisher will be on when the scheduler acts as a
> > BeginFrame publisher for itself rather than using an external
> BeginFrameSource"?
> 
> What Simon says I think is the correct intention.
> 
> begin_frame_message_receiver being false will mean that the scheduler acts as
a
> BeginFrame publisher for itself.
> begin_frame_message_publisher will indicate whether the scheduler acts as a
> BeginFrame publisher for child compositors.
> 
> I guess the "message" also helps draw the distinction between self and child.
> Definitely think we should rename the flags now.
> 
> > 
> > Tim 'mithro' Ansell

[simonhong, 2014-11-05 01:28:56]

On 2014/11/05 00:55:35, brianderson wrote:
> On 2014/11/05 00:24:06, mithro wrote:
> > On 2014/11/04 23:49:17, brianderson wrote:
> > 
> > Couple of questions based on your description;
> > 
> > > This two flags are added into the LayerTreeSettings and SchedulerSettings.
> > > This is a small piece of unified BeginFrame scheduling.
> > 
> > Thank you for working on this and also for splitting parts out into smaller
> > patches. It's greatly appreciated!
> > 
> > The final goal here is to have the Scheduler use an external
BeginFrameSource
> on
> > all platforms (except when vsync throttling is disabled), right?
> 
> For the Renderer's Scheduler, yes. For the Browser's Scheduler, however, we
will
> likely never have an external BeginFrameSource for Linux or Windows.
> 
> > 
> > > begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> > > begin_frame_receiver will be on when scheduler uses external
> BeginFrameSource.
> > 
> > Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver?
> 
> This flag is supposed to mean "ViewMsg_BeginFrame receiver", which is slightly
> different. Should we rename the flags to begin_frame_message_receiver and
> begin_frame_message_publisher to reduce confusion?
> 
> > 
> > > begin_frame_publisher will be on when scheduler acts as a BeginFrame
> publisher
> > for child Scheduler.
> > 
> > Do you mean "begin_frame_publisher will be on when the scheduler acts as a
> > BeginFrame publisher for itself rather than using an external
> BeginFrameSource"?
> 
> What Simon says I think is the correct intention.
> 
> begin_frame_message_receiver being false will mean that the scheduler acts as
a
> BeginFrame publisher for itself.
> begin_frame_message_publisher will indicate whether the scheduler acts as a
> BeginFrame publisher for child compositors.
> 
> I guess the "message" also helps draw the distinction between self and child.
> Definitely think we should rename the flags now.

begin_frame_message_publisher describes clearly its role in scheduler.
In begin_frame_message_receiver case, it is also used for creating external BFS
now.
If it only means for delivering messages, should we need another flag whether
external BFS is used or not.?

> 
> > 
> > Tim 'mithro' Ansell

[mithro-old, 2014-11-05 01:36:48]

On 2014/11/05 01:03:59, simonhong wrote:
> On 2014/11/05 00:55:35, brianderson wrote:
> > On 2014/11/05 00:24:06, mithro wrote:
> > > On 2014/11/04 23:49:17, brianderson wrote:
> > > 
> > > Couple of questions based on your description;
> > > 
> > > > This two flags are added into the LayerTreeSettings and
SchedulerSettings.
> > > > This is a small piece of unified BeginFrame scheduling.
> > > 
> > > Thank you for working on this and also for splitting parts out into
smaller
> > > patches. It's greatly appreciated!
> > > 
> > > The final goal here is to have the Scheduler use an external
> BeginFrameSource
> > on
> > > all platforms (except when vsync throttling is disabled), right?
> > 
> > For the Renderer's Scheduler, yes. For the Browser's Scheduler, however, we
> will
> > likely never have an external BeginFrameSource for Linux or Windows.
>
https://docs.google.com/a/chromium.org/drawings/d/1WEj-6A-8FmJNIMbd9hvkvxAuOO...

I guess
https://docs.google.com/a/chromium.org/drawings/d/1WEj-6A-8FmJNIMbd9hvkvxAuOO...
needs to be updated to include the idea of the browser scheduler?

> Ah, I was confused.
> On aura, browser-side scheduler will use synthetic source.
> Others (android and mac) will use external BFS.

I also think we are talking about different things here? I'm using "external" to
mean BeginFrames come from outside the scheduler, not necessarily outside
Chrome.

Even when a platform provides something like a BeginFrame message, I still think
we want to run it through a PLL to stabilize timestamps and provide the ability
to predict the next value.

> > > > begin_frame_receiver is same as previous begin_frame_scheduling_enabled.
> > > > begin_frame_receiver will be on when scheduler uses external
> > BeginFrameSource.
> > > 
> > > Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver?
> > 
> > This flag is supposed to mean "ViewMsg_BeginFrame receiver", which is
slightly
> > different. Should we rename the flags to begin_frame_message_receiver and
> > begin_frame_message_publisher to reduce confusion?
> > 
> > > 
> > > > begin_frame_publisher will be on when scheduler acts as a BeginFrame
> > publisher
> > > for child Scheduler.
> > > 
> > > Do you mean "begin_frame_publisher will be on when the scheduler acts as a
> > > BeginFrame publisher for itself rather than using an external
> > BeginFrameSource"?
> > 
> > What Simon says I think is the correct intention.
> > 
> > begin_frame_message_receiver being false will mean that the scheduler acts
as a
> > BeginFrame publisher for itself.

Maybe "begin_frame_self_publish" or something along those lines?

> > begin_frame_message_publisher will indicate whether the scheduler acts as a
> > BeginFrame publisher for child compositors.
> > 
> > I guess the "message" also helps draw the distinction between self and
child.
> > Definitely think we should rename the flags now.

Maybe begin_frame_forward_to_child?

Tim

[brianderson, 2014-11-06 00:33:56]

On 2014/11/05 01:36:48, mithro wrote:
> On 2014/11/05 01:03:59, simonhong wrote:
> > On 2014/11/05 00:55:35, brianderson wrote:
> > > On 2014/11/05 00:24:06, mithro wrote:
> > > > On 2014/11/04 23:49:17, brianderson wrote:
> > > > 
> > > > Couple of questions based on your description;
> > > > 
> > > > > This two flags are added into the LayerTreeSettings and
> SchedulerSettings.
> > > > > This is a small piece of unified BeginFrame scheduling.
> > > > 
> > > > Thank you for working on this and also for splitting parts out into
> smaller
> > > > patches. It's greatly appreciated!
> > > > 
> > > > The final goal here is to have the Scheduler use an external
> > BeginFrameSource
> > > on
> > > > all platforms (except when vsync throttling is disabled), right?
> > > 
> > > For the Renderer's Scheduler, yes. For the Browser's Scheduler, however,
we
> > will
> > > likely never have an external BeginFrameSource for Linux or Windows.
> >
>
https://docs.google.com/a/chromium.org/drawings/d/1WEj-6A-8FmJNIMbd9hvkvxAuOO...
> 
> I guess
>
https://docs.google.com/a/chromium.org/drawings/d/1WEj-6A-8FmJNIMbd9hvkvxAuOO...
> needs to be updated to include the idea of the browser scheduler?
> 
> > Ah, I was confused.
> > On aura, browser-side scheduler will use synthetic source.
> > Others (android and mac) will use external BFS.
> 
> I also think we are talking about different things here? I'm using "external"
to
> mean BeginFrames come from outside the scheduler, not necessarily outside
> Chrome.
> 

Yeah... we have a lot of concepts that are almost the same but aren't quite.
External BeginFrame sources are external to the scheduler.
External BeginFrame sources can be from a message (for the Renderer) or from a
platform hook (for Android and Mac).

I guess my suggestion of begin_frame_message_source doesn't quite make sense
then.

> Even when a platform provides something like a BeginFrame message, I still
think
> we want to run it through a PLL to stabilize timestamps and provide the
ability
> to predict the next value.

Agree in some cases, but don't think this will affect the names in this patch.

> 
> > > > > begin_frame_receiver is same as previous
begin_frame_scheduling_enabled.
> > > > > begin_frame_receiver will be on when scheduler uses external
> > > BeginFrameSource.
> > > > 
> > > > Thanks to my refactor, the Scheduler is *always* a BeginFrame receiver?
> > > 
> > > This flag is supposed to mean "ViewMsg_BeginFrame receiver", which is
> slightly
> > > different. Should we rename the flags to begin_frame_message_receiver and
> > > begin_frame_message_publisher to reduce confusion?
> > > 
> > > > 
> > > > > begin_frame_publisher will be on when scheduler acts as a BeginFrame
> > > publisher
> > > > for child Scheduler.
> > > > 
> > > > Do you mean "begin_frame_publisher will be on when the scheduler acts as
a
> > > > BeginFrame publisher for itself rather than using an external
> > > BeginFrameSource"?
> > > 
> > > What Simon says I think is the correct intention.
> > > 
> > > begin_frame_message_receiver being false will mean that the scheduler acts
> as a
> > > BeginFrame publisher for itself.
> 
> Maybe "begin_frame_self_publish" or something along those lines?

That would invert the polarity of the flag. 
Let's go with use_external_begin_frame_source, which is in the same spirit but
doesn't invert the polarity.

In the end, this flag should get removed (platform-specific code will pass in an
external begin frame source or not) so naming of this one isn't super important.

> 
> > > begin_frame_message_publisher will indicate whether the scheduler acts as
a
> > > BeginFrame publisher for child compositors.
> > > 
> > > I guess the "message" also helps draw the distinction between self and
> child.
> > > Definitely think we should rename the flags now.
> 
> Maybe begin_frame_forward_to_child?

Like this suggestion since it's more descriptive, but maybe it could use more
natural wording like forward_begin_frames_to_children?

> 
> Tim

[simonhong, 2014-11-10 13:18:09]

Comments are addressed.
PTAL!

https://codereview.chromium.org/696413003/diff/1/cc/scheduler/scheduler_setti...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/696413003/diff/1/cc/scheduler/scheduler_setti...
cc/scheduler/scheduler_settings.cc:13: : begin_frame_receiver(true),
On 2014/11/05 00:39:14, piman (Very slow to review) wrote:
> nit: it would be good to have the same default here and in LayerTreeSettings.
> Less confusion.

Done.

[brianderson, 2014-11-10 18:16:24]

Still lgtm.

[piman, 2014-11-10 20:17:14]

lgtm

[mithro-old, 2014-11-10 22:49:06]

LGTM with the new names.

[simonhong, 2014-11-11 22:09:01]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2014-11-11 22:09:51]

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

[commit-bot: I haz the power, 2014-11-11 23:25:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-11 23:26:26]

Patchset 3 (id:??) landed as
https://crrev.com/2880b8edb63e0bb201d045bbbe32d185a9a3c6a3
Cr-Commit-Position: refs/heads/master@{#303743}