Added ability to dynamically toggle frame throttling in the scheduler.

Added ability to dynamically toggle frame throttling in the scheduler.

In order to allow Vsync to be toggled by dev tools we need to both set the swap
interval to 0 on systems that support it and prevent the scheduler from
throttling frames.

BUG=437172
Committed: https://crrev.com/274110611d66427435521032dcbaceca8a502455
Cr-Commit-Position: refs/heads/master@{#310139}

[bajones, 2014-12-15 22:30:20]

bajones@chromium.org changed reviewers:
+ brianderson@chromium.org, enne@chromium.org

[bajones, 2014-12-15 22:30:20]

This is an area of the code I'm not terribly familiar with, so please feel free
to flag me down if there are better/easier ways to accomplish the same goal.

The intent is that LayerTreeHost::SetThrottleFrameProduction will be called by
RenderWidgetCompositor in response to a "Disable Vsync" checkbox being toggled
in DevTools.

[enne (OOO), 2014-12-15 23:20:33]

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

[enne (OOO), 2014-12-15 23:21:07]

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:238: throttle_frame_production_ =
settings_.throttle_frame_production && throttle;
It's a little odd to my eyes to have both a scheduler setting and a toggle
function.  Could you just have the toggle function?  What's the setting for?

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:190: BeginFrameSource* unthrottled_frame_source_;
Maybe it's not possible, but I kind of wish that this was a detail that the
scheduler didn't have to know about.  BeginFrameSource itself is pretty
abstract, so it seems like it could be possible to make the external begin frame
source implement this unthrottled setting and then the scheduler doesn't have to
know anything about it.

(Also, when the devtools button is clicked, does both the renderer and the
browser need to use unthrottled frames? Or, is this just a renderer thing?)

[bajones, 2014-12-15 23:37:58]

On 2014/12/15 at 23:21:07, enne wrote:
>
https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.cc#ne...
> cc/scheduler/scheduler.cc:238: throttle_frame_production_ =
settings_.throttle_frame_production && throttle;
> It's a little odd to my eyes to have both a scheduler setting and a toggle
function.  Could you just have the toggle function?  What's the setting for?

Thanks for pointing this out. It reminded me that I wanted to look into this
further.

As far as I can tell the setting is only there so that if you launch Chrome with
--disable-vsync it creates and unthrottled frame source. The new checkbox is
effectively going to be a dynamic version of the command line, so logically they
should be able to share the same code path and that's probably the right thing
to do. The question I had was that if a user launched with --disable-vsync
should they be able to turn vsync on again afterwords via devtools? That feels
messy to me, which is why they're currently preserved as two different paths
here, but I'd love to hear any thoughts on the subject!

>
https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.h#new...
> cc/scheduler/scheduler.h:190: BeginFrameSource* unthrottled_frame_source_;
> Maybe it's not possible, but I kind of wish that this was a detail that the
scheduler didn't have to know about.  BeginFrameSource itself is pretty
abstract, so it seems like it could be possible to make the external begin frame
source implement this unthrottled setting and then the scheduler doesn't have to
know anything about it.

This is probably a better question from Brian.
 
> (Also, when the devtools button is clicked, does both the renderer and the
browser need to use unthrottled frames? Or, is this just a renderer thing?)

The frame throttling logic only touches the renderer, but for the full feature
to work we'll also need to plumb through a way to set the GPU SwapInterval,
which (AFAIK) does need to be routed through the browser process. I've got that
working already and am prepping code for a future CL.

[brianderson, 2014-12-16 22:36:42]

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:238: throttle_frame_production_ =
settings_.throttle_frame_production && throttle;
On 2014/12/15 23:21:07, enne wrote:
> It's a little odd to my eyes to have both a scheduler setting and a toggle
> function.  Could you just have the toggle function?  What's the setting for?

Can we make the command line flag only set the initial state of
throttle_frame_production_ and the dev tools setting?

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:190: BeginFrameSource* unthrottled_frame_source_;
On 2014/12/15 23:21:07, enne wrote:
> Maybe it's not possible, but I kind of wish that this was a detail that the
> scheduler didn't have to know about.  BeginFrameSource itself is pretty
> abstract, so it seems like it could be possible to make the external begin
frame
> source implement this unthrottled setting and then the scheduler doesn't have
to
> know anything about it.

We could move the existing internal synthetic BeginFrame sources just outside
into the Proxy, then pass in a BeginFrameSourceMultiplexer as the primary
source, but we'd have to duplicate logic for both TP and STP. It would clean up
the scheduler, but complicate the proxies. I have a slight preference for the
way Brandon currently has it.

> 
> (Also, when the devtools button is clicked, does both the renderer and the
> browser need to use unthrottled frames? Or, is this just a renderer thing?)


Once the Browser switches over to using the STP w/ scheduler, we'll need to pipe
the selection over to the Browser as well so that it can also use the
unthrottled source. Otherwise it will indirectly swap ack limit the Renderer.
Might be a little annoying to track how many Renderers are unthrottled to decide
whether the Browser should also be unthrottled.

[brianderson, 2014-12-16 22:36:42]

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:238: throttle_frame_production_ =
settings_.throttle_frame_production && throttle;
On 2014/12/15 23:21:07, enne wrote:
> It's a little odd to my eyes to have both a scheduler setting and a toggle
> function.  Could you just have the toggle function?  What's the setting for?

Can we make the command line flag only set the initial state of
throttle_frame_production_ and the dev tools setting?

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

https://codereview.chromium.org/811523002/diff/1/cc/scheduler/scheduler.h#new...
cc/scheduler/scheduler.h:190: BeginFrameSource* unthrottled_frame_source_;
On 2014/12/15 23:21:07, enne wrote:
> Maybe it's not possible, but I kind of wish that this was a detail that the
> scheduler didn't have to know about.  BeginFrameSource itself is pretty
> abstract, so it seems like it could be possible to make the external begin
frame
> source implement this unthrottled setting and then the scheduler doesn't have
to
> know anything about it.

We could move the existing internal synthetic BeginFrame sources just outside
into the Proxy, then pass in a BeginFrameSourceMultiplexer as the primary
source, but we'd have to duplicate logic for both TP and STP. It would clean up
the scheduler, but complicate the proxies. I have a slight preference for the
way Brandon currently has it.

> 
> (Also, when the devtools button is clicked, does both the renderer and the
> browser need to use unthrottled frames? Or, is this just a renderer thing?)


Once the Browser switches over to using the STP w/ scheduler, we'll need to pipe
the selection over to the Browser as well so that it can also use the
unthrottled source. Otherwise it will indirectly swap ack limit the Renderer.
Might be a little annoying to track how many Renderers are unthrottled to decide
whether the Browser should also be unthrottled.

[bajones, 2014-12-16 23:20:20]

As Brian suggested, I've updated the CL so that
scheduler_settings.unthrottle_frame_production (which is basically just "Was
Chrome launched with --disable-gpu-vsync?") is only responsible for setting the
initial throttling state and can be overridden afterwards. Additionally I've
also removed the ability of the Primary frame source to be a BackToBack
(unthrottled) source, since we always need the ability to toggle back and forth
now.

PTAL!

[brianderson, 2014-12-16 23:38:02]

https://codereview.chromium.org/811523002/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/811523002/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:90:
throttle_frame_production_(scheduler_settings.throttle_frame_production),
The meaning of throttle_frame_production is slightly differnent. Should we get
rid of the scheduler and LTHI setting and just check for the flag in the
constructor here?

[mithro-old, 2014-12-17 04:35:35]

mithro@mithis.com changed reviewers:
+ mithro@mithis.com

[mithro-old, 2014-12-17 04:35:36]

Adding myself as a reviewer so I don't forget about this patch.

[mithro-old, 2014-12-17 09:13:05]

On 2014/12/17 04:35:36, mithro wrote:
> Adding myself as a reviewer so I don't forget about this patch.

This patch has no tests, please test the functionality you are adding here.

[bajones, 2014-12-19 00:01:20]

Added test with brianderson@'s help. (Thank you!) PTAL.

[mithro-old, 2014-12-19 04:11:19]

Generally looking pretty good. Great to see that this was such a small and
simple CL.

Just a FYI, the mess that is the BeginFrameSourceConstructor might be able to go
away with https://code.google.com/p/chromium/issues/detail?id=440281

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

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:243: scheduler_->FrameProductionThrottled())
{
I think we are actually better doing something like;

scheduler_->external_begin_frame_source_ &&
scheduler_->external_begin_frame_source_.NeedsBeginFrames();

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2198:
scheduler_settings.use_external_begin_frame_source = true;
I think you want to actually check the BeginFrameArgs objects which get used
here.

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2325: 
You probably want a simple test for going to/from the background source here.

You also probably want a test which shows you can't accidently go from the
throttled state to an external BFS which doesn't exist?

https://codereview.chromium.org/811523002/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/811523002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:489: int swap_interval_;
This value doesn't seem to be used anywhere?

[mithro-old, 2014-12-19 04:12:29]

FYI too https://code.google.com/p/chromium/issues/detail?id=443185

[bajones, 2014-12-19 04:47:53]

On 2014/12/19 at 04:12:29, mithro wrote:
> FYI too https://code.google.com/p/chromium/issues/detail?id=443185

Thanks for the follow up review! Might not be able to address the feedback
immediately, as I'm going out of town till the 29th. Sounds like I might have
some rebasing to do at that point, so thanks for pointing out the related
tickets! :)

[mithro-old, 2014-12-19 04:48:48]

On 2014/12/19 04:47:53, bajones wrote:
> On 2014/12/19 at 04:12:29, mithro wrote:
> > FYI too https://code.google.com/p/chromium/issues/detail?id=443185
> 
> Thanks for the follow up review! Might not be able to address the feedback
> immediately, as I'm going out of town till the 29th. Sounds like I might have
> some rebasing to do at that point, so thanks for pointing out the related
> tickets! :)

I'm out till the 16th Feb 2015, so Brian will have to give you the LGTM.

Tim

[brianderson, 2014-12-19 23:04:59]

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

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2261: client.task_runner().RunPendingTasks();
 // Run posted deadline.
Comment should read "Run posted deadline and BeginFrame."

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2268: client.task_runner().RunPendingTasks();
 // Run posted BeginFrame.
Comment should read "Run posted deadline."

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2274: // If we don't swap on the deadline, we
wait for the next BeginFrame.
Probably just delete this block. We went through one full cycle with each source
by the time we get here.

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2311: scheduler->SetNeedsRedraw();
What happens if you RunPendingTasks after here?

https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:2323: scheduler->SetNeedsRedraw();
Don't need to SetNeedsRedraw here.

[bajones, 2015-01-05 22:47:43]

Thanks for the feedback, Brian! Addressed your comments.

On 2014/12/19 at 23:04:59, brianderson wrote:
>
https://codereview.chromium.org/811523002/diff/60001/cc/scheduler/scheduler_u...
> cc/scheduler/scheduler_unittest.cc:2311: scheduler->SetNeedsRedraw();
> What happens if you RunPendingTasks after here?
No change to the test behavior.

[brianderson, 2015-01-05 23:50:46]

lgtm.

Do you plan to land the browser-side logic as a separate patch? Coordinating the
setting across multiple Renderers will be tricky to get right.

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

https://codereview.chromium.org/811523002/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:68: TRACE_EVENT1("cc", "Scheduler::Scheduler()",
"PrimaryFrameSource",
PrimaryFrameSource -> UnthrottledFrameSource

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

https://codereview.chromium.org/811523002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:488: int swap_interval_;
Doesn't look like this variable is used?

[bajones, 2015-01-06 18:33:24]

On 2015/01/05 at 23:50:46, brianderson wrote:
> Do you plan to land the browser-side logic as a separate patch? Coordinating
the setting across multiple Renderers will be tricky to get right.

Yes, the logic that triggers this code will be in upcoming patches. I'll make
sure that you're CCed on them. 

>
https://codereview.chromium.org/811523002/diff/80001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.h:488: int swap_interval_;
> Doesn't look like this variable is used?

Good catch. Probably a leftover from an upcoming patch that I forgot to remove
while breaking it up for committing.

[bajones, 2015-01-06 18:33:36]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-01-06 18:34:27]

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

[commit-bot: I haz the power, 2015-01-06 20:54:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-06 20:56:12]

Patchset 6 (id:??) landed as
https://crrev.com/274110611d66427435521032dcbaceca8a502455
Cr-Commit-Position: refs/heads/master@{#310139}