cc: Throttle swaps in Scheduler instead of OutputSurface

cc: Throttle swaps in Scheduler instead of OutputSurface

This doesn't represent a functional change and only moves
the swap throttling logic from the OutputSurface to the
Scheduler.

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

[brianderson, 2014-03-13 21:11:52]

WIP

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

https://codereview.chromium.org/199523002/diff/1/cc/output/output_surface.h#o...
cc/output/output_surface.h:107: void InitializeBeginImplFrameEmulation(
In the interest of simplifying OutputSurface, we could probably move all the
BeginImplFrameEmulation logic into a separate class owned by the LTHI.

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

https://codereview.chromium.org/199523002/diff/1/cc/scheduler/frame_rate_cont...
cc/scheduler/frame_rate_controller.h:28: virtual void
FrameRateControllerTick(const BeginFrameArgs& args) = 0;
The FrameRateController doesn't really do much anymore. Maybe I should just rip
it out?

https://codereview.chromium.org/199523002/diff/1/cc/scheduler/frame_rate_cont...
File cc/scheduler/frame_rate_controller_unittest.cc (left):

https://codereview.chromium.org/199523002/diff/1/cc/scheduler/frame_rate_cont...
cc/scheduler/frame_rate_controller_unittest.cc:70: TEST(FrameRateControllerTest,
TestFrameThrottling_TwoFramesInFlight) {
I need to port these tests over to the scheduler.

https://codereview.chromium.org/199523002/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/199523002/diff/1/ui/compositor/compositor.cc#...
ui/compositor/compositor.cc:216:
!command_line->HasSwitch(switches::kDisableGpuVsync);
This is needed to get aura-based platforms to honor vsync being disabled,
otherwise the scheduler ticks at the vsync rate instead of ticking when the
swap-ack arrives.

I could move this to a separate patch if needed.

[Sami, 2014-03-14 15:34:58]

I like where this is going. Thanks for the cleanup.

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

https://codereview.chromium.org/199523002/diff/1/cc/scheduler/frame_rate_cont...
cc/scheduler/frame_rate_controller.h:28: virtual void
FrameRateControllerTick(const BeginFrameArgs& args) = 0;
On 2014/03/13 21:11:52, brianderson wrote:
> The FrameRateController doesn't really do much anymore. Maybe I should just
rip
> it out?

Yeah, seems like we could mostly get rid of it.

https://codereview.chromium.org/199523002/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/199523002/diff/1/ui/compositor/compositor.cc#...
ui/compositor/compositor.cc:216:
!command_line->HasSwitch(switches::kDisableGpuVsync);
On 2014/03/13 21:11:52, brianderson wrote:
> This is needed to get aura-based platforms to honor vsync being disabled,
> otherwise the scheduler ticks at the vsync rate instead of ticking when the
> swap-ack arrives.
> 
> I could move this to a separate patch if needed.

+1 for a separate patch so we can more easily tell the rest of this is a no-op.

https://codereview.chromium.org/199523002/diff/20001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.h (right):

https://codereview.chromium.org/199523002/diff/20001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.h:33: class CC_EXPORT BeginFrameSource :
TimeSourceClient {
I wonder if we should call this something like SyntheticBeginFrameSource to make
its purpose a little clearer? Up to you.

https://codereview.chromium.org/199523002/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/199523002/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:184: // swap when there is not damage,
for example.
Nit: we skip both the draw & swap when this happens, right? Not just the swap.

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

https://codereview.chromium.org/199523002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1844:
output_surface->SetThrottleFrameProduction(false);
Should this also disable the swap limit in the scheduler? Previously we had the
convention that a swap limit of 0 meant no limit, but I'm not sure if that works
anymore because of the DEFAULT_MAX_FRAMES_PENDING adjustment below.

[danakj, 2014-03-18 19:54:44]

I see WIP in a few places here, and some good feedback from Sami already. Did
you want a review from me yet?

[brianderson, 2014-03-19 10:19:51]

Don't worry about anymore reviews. I'm ripping out more logic from OutputSurface
while still keeping this patch a no op and will have another patch up in a bit.

[brianderson, 2014-03-20 16:57:43]

Hmm, this patch is getting larger than I expected so I might split it up into a
few patches.

I've now migrated the retroactive logic to the scheduler, but still need to
migrate the throttled_frame_production logic over.

[danakj, 2014-03-20 16:59:53]

Btw thank you very much for working to move stuff out of OutputSurface
implementation :)


On Thu, Mar 20, 2014 at 12:57 PM, <brianderson@chromium.org> wrote:

> Hmm, this patch is getting larger than I expected so I might split it up
> into a
> few patches.
>
> I've now migrated the retroactive logic to the scheduler, but still need to
> migrate the throttled_frame_production logic over.
>
> https://codereview.chromium.org/199523002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-04-03 00:46:30]

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

[Sami, 2014-04-08 13:42:18]

Sorry for the delay in getting to this. lgtm.

https://codereview.chromium.org/199523002/diff/120001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/199523002/diff/120001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:184: // swap when there is not damage,
for example.
Not directly related to this patch, but I always wondered why we decide to skip
drawing when there's no damage in LTHI::DrawLayers instead of in
LTHI::PrepareToDraw. It seems like it would be cleaner to have a different
DrawSwapReadbackResult for that case.

https://codereview.chromium.org/199523002/diff/120001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:192: void OnSwapBuffersComplete();
bikeshed: DidSwapBuffersComplete to match the scheduler naming convention? Or
just keep the OutputSurface naming like you did -- up to you.

[danakj, 2014-04-11 15:12:28]

Looks awesome, seems so much simpler to do the same things :)

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:203: if (is_lost_)
Why do you need these is_lost_ checks here?

If needed, can you put the TRACE above them, and include the is_lost_ in the
trace event?

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:99: void Scheduler::DidSwapBuffers() {
state_machine_.DidSwapBuffers(); }
Why no ProcessScheduledActions()?

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:175: // regarding whether or not the swap
actually occured. We might skip the
This overlaps with the DrawSwapReadbackResult::did_swap. Are you planning to
remove one of them? Why do we need both?

https://codereview.chromium.org/199523002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_delegated.cc (left):

https://codereview.chromium.org/199523002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_delegated.cc:243:
host_impl->OnSwapBuffersComplete();
Can you explain why this is no longer needed in tests?

https://codereview.chromium.org/199523002/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:436: void ThreadProxy::SetMaxSwapsPending(int max) {
SetMaxSwapsPendingOnImplThread() ?

[brianderson, 2014-04-11 20:49:40]

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:203: if (is_lost_)
On 2014/04/11 15:12:28, danakj wrote:
> Why do you need these is_lost_ checks here?
> 
> If needed, can you put the TRACE above them, and include the is_lost_ in the
> trace event?

The is_lost_ check should probably go into each SwapBuffers implementation
instead of the DidSwapBuffers notification, like
https://codereview.chromium.org/221833009 does.

It would be even better if we didn't swap if we lose the output surface during a
draw, which happens in unit tests. I don't think we lose the context
synchronously during a draw in normal operation though.

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:99: void Scheduler::DidSwapBuffers() {
state_machine_.DidSwapBuffers(); }
On 2014/04/11 15:12:28, danakj wrote:
> Why no ProcessScheduledActions()?

I guess it wouldn't hurt, but today this will only be called within a callstack
where we are already in a ProcessScheduledActions.  Also, a DidSwapBuffers()
should not trigger any new actions, only throttle some.

I'll add a comment, since I would consider it a bug if we need to call
ProcessScheduledActions here to make proper progress.

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:175: // regarding whether or not the swap
actually occured. We might skip the
On 2014/04/11 15:12:28, danakj wrote:
> This overlaps with the DrawSwapReadbackResult::did_swap. Are you planning to
> remove one of them? Why do we need both?

Hmm, I was under the impression DrawSwapReadbackResult::did_swap would be true
even if there was no damage and we didn't end up swapping. That's only true for
the did_swap in SchedulerStateMachine::UpdateStateOnDraw(bool did_swap) though.

DidSwapBuffers is used for OutputSurface throttling and reflects whether or not
we actually swapped and should expect a swap ack. I'm not sure if there are any
other early outs other than the damage. I guess I early out on is_lost_, so
that's one difference in this patch.

How about I rename the did_swaps to did_request_swap?

https://codereview.chromium.org/199523002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_delegated.cc (left):

https://codereview.chromium.org/199523002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_delegated.cc:243:
host_impl->OnSwapBuffersComplete();
On 2014/04/11 15:12:28, danakj wrote:
> Can you explain why this is no longer needed in tests?

I think it was a bug that they were needed in the first place.

It was causing either timeouts or DCHECKs in the unit tests with the new code. I
think it had to do with the DidSwapBuffers notifications not being throttled by
is_lost_ before this patch.

https://codereview.chromium.org/199523002/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:436: void ThreadProxy::SetMaxSwapsPending(int max) {
On 2014/04/11 15:12:28, danakj wrote:
> SetMaxSwapsPendingOnImplThread() ?

Okay, that would be more consistent.

[brianderson, 2014-04-12 00:09:58]

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
cc/test/fake_output_surface.cc:69: if (is_lost_)
I decided to DCHECK for all other OutputSurface implementations.

[danakj, 2014-04-14 18:14:50]

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:203: if (is_lost_)
On 2014/04/11 20:49:40, brianderson wrote:
> On 2014/04/11 15:12:28, danakj wrote:
> > Why do you need these is_lost_ checks here?
> > 
> > If needed, can you put the TRACE above them, and include the is_lost_ in the
> > trace event?
> 
> The is_lost_ check should probably go into each SwapBuffers implementation
> instead of the DidSwapBuffers notification, like
> https://codereview.chromium.org/221833009 does.
> 
> It would be even better if we didn't swap if we lose the output surface during
a
> draw, which happens in unit tests. I don't think we lose the context
> synchronously during a draw in normal operation though.

The single thread implementation checks for this before swapping. Should the
thread proxy do that also?

https://codereview.chromium.org/199523002/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:210: if (is_lost_)
Since this is an ACK from the browser IPC it makes sense that losing the output
surface would drop future ACKs here instead of having them call back to the
client.

Maybe the client pointer should be unset entire when the OutputSurface is lost?

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/199523002/diff/140001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:175: // regarding whether or not the swap
actually occured. We might skip the
On 2014/04/11 20:49:40, brianderson wrote:
> On 2014/04/11 15:12:28, danakj wrote:
> > This overlaps with the DrawSwapReadbackResult::did_swap. Are you planning to
> > remove one of them? Why do we need both?
> 
> Hmm, I was under the impression DrawSwapReadbackResult::did_swap would be true
> even if there was no damage and we didn't end up swapping. That's only true
for
> the did_swap in SchedulerStateMachine::UpdateStateOnDraw(bool did_swap)
though.
> 
> DidSwapBuffers is used for OutputSurface throttling and reflects whether or
not
> we actually swapped and should expect a swap ack. I'm not sure if there are
any
> other early outs other than the damage. I guess I early out on is_lost_, so
> that's one difference in this patch.
> 
> How about I rename the did_swaps to did_request_swap?

The did_swap result is being dropped right now in
Scheduler::DrawAndSwapIfPossible() and Scheduler::DrawAndSwapForced().

What if those methods called back to the scheduler with the full
DrawSwapReadbackResult?

Right now the did_swap value seems only useful for the
Scheduler::DrawAndReadback() DCHECK on it.

Since we already have this data, it'd be nice to reuse it, or base the DCHECK on
the new mechanism and remove did_swap entirely?

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

https://codereview.chromium.org/199523002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:102: // swapping should not trigger any new actions.
Can we DCHECK that state_machine_.NextAction() is NONE here?

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
cc/test/fake_output_surface.cc:69: if (is_lost_)
On 2014/04/12 00:09:58, brianderson wrote:
> I decided to DCHECK for all other OutputSurface implementations.

Hm, there's no point in a unit test that can't happen in reality. We should
understand why these tests can't actually happen and remove them/change them to
match reality.

In single thread, right now we check for lost context after drawing, and return
false from DoComposite() to avoid swapping.

We could check in the ThreadProxy to get similar behaviour. Right now I don't
think we do, so we could lose the context during draw but do the swap hitting
the DCHECK?

But why do we care if we SwapBuffers on a lost surface anyway?

[brianderson, 2014-04-16 22:41:53]

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

https://codereview.chromium.org/199523002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:102: // swapping should not trigger any new actions.
On 2014/04/14 18:14:51, danakj wrote:
> Can we DCHECK that state_machine_.NextAction() is NONE here?

We are most likely already in a ProcessScheduledActions, since I don't think
DidSwapBuffers is ever called asynchronously, in which case there may be actions
pending that weren't triggered by the swap.

I could check to see if we are not in ProcessScheduledActions, then DCHECK.

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/199523002/diff/160001/cc/test/fake_output_sur...
cc/test/fake_output_surface.cc:69: if (is_lost_)
On 2014/04/14 18:14:51, danakj wrote:
> On 2014/04/12 00:09:58, brianderson wrote:
> > I decided to DCHECK for all other OutputSurface implementations.
> 
> Hm, there's no point in a unit test that can't happen in reality. We should
> understand why these tests can't actually happen and remove them/change them
to
> match reality.
> 
> In single thread, right now we check for lost context after drawing, and
return
> false from DoComposite() to avoid swapping.
> 
> We could check in the ThreadProxy to get similar behaviour. Right now I don't
> think we do, so we could lose the context during draw but do the swap hitting
> the DCHECK?
> 
> But why do we care if we SwapBuffers on a lost surface anyway?

I'm going to remove the is_lost_ checks.

[brianderson, 2014-04-17 00:04:10]

ptal. 

Dana and Antoine: thanks for feedback regarding is_lost_. I went ahead and
removed it from this patch.

[danakj, 2014-04-17 15:05:39]

LGTM

https://codereview.chromium.org/199523002/diff/220001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/199523002/diff/220001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:436: void
ThreadProxy::SetMaxSwapsPendingOnImplThread(int max) {
+enne FYI: STP will need to do this also.

[brianderson, 2014-04-17 18:47:05]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-17 18:47:19]

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

[commit-bot: I haz the power, 2014-04-17 20:19:11]

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

[commit-bot: I haz the power, 2014-04-17 20:19:11]

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

[brianderson, 2014-04-17 23:53:55]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-17 23:54:27]

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

[commit-bot: I haz the power, 2014-04-18 05:06:19]

Change committed as 264732