cc: Move scheduling logic out of OutputSurface

cc: Move scheduling logic out of OutputSurface

This moves the vsync throttling enabled/disabled
logic and the synthetic/emulated BeginFrame logic
out of OutputSurface and moves it to the Scheduler.

This then allows us to also remove the retroactive
BeginFrame logic from OutputSurface since the
Scheduler also has retroactive BeginFrame logic.

BUG=246861
BUG=251909
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266348

[brianderson, 2014-04-03 02:41:25]

Depends on:
https://codereview.chromium.org/199523002
and the rest.

[brianderson, 2014-04-04 02:16:26]

hold off on reviewing this one. lots of tests need fixing

[brianderson, 2014-04-08 02:31:48]

Rebased and tests fixed now.

[Sami, 2014-04-08 13:46:00]

Woo, another nice red patch.

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface.cc:256: client_->DidSwapBuffers();
Should we guard against is_lost_?

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface_client.h:39: // if OnSwapBuffersComplete arrives for
the previous OutputSurface.
It feels like this comment belongs to OutputSurface::SwapBuffers() or elsewhere
in the implementation, because this isn't something the client has to worry
about. Also, replace "scheduler" with "client" to keep this more generic.

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:54: Scheduler::~Scheduler() {}
Should probably stop the time source here.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:155: state_machine_.OnSwapBuffersComplete();
That DCHECK in Scheduler::DidSwapBuffers could go here too.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:226:
SetupNextBeginFrameWhenVSyncEnabled(needs_begin_frame);
bikeshed: s/VSync/Throttling/ to keep the terms a little more clearly defined.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:378: // Not if frame production isn't throttled
though, since all frames will be
This is getting a little confusing. Should we perhaps have a separate deque for
synthetic BeginFrames or something like ::BeginSyntheticFrame()?

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:69: void SetEstimatedParentDrawTime(base::TimeDelta
draw_time);
Since DrawDurationEstimate() is already in SchedulerClient seems like 
SetEstimatedParentDrawTime would fit there too. WDYT?

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:185: scoped_refptr<TimeSource>
time_source_for_synthetic_begin_frames_;
Is there a way we could isolate the synthetic BeginFrame logic to an
inner/implementation-side class? Eventually we want to get rid of it, so it
would be convenient to have it all in one place. Also, it feels weird to have
Scheduler derive from TimeSourceClient since it's more of an implementation
detail than public scheduler interface.

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

https://codereview.chromium.org/221833009/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(biranderson): Don't use a
hard-coded parent draw time.
Hey no assigning TODOs to others :)

[brianderson, 2014-04-09 02:52:03]

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface.cc:256: client_->DidSwapBuffers();
On 2014/04/08 13:46:01, Sami wrote:
> Should we guard against is_lost_?

Good idea. I will return early if is_lost_ here and double check that other
implementations also check for is_lost_.

I was really hoping I could get rid of the is_lost_ member variable in
OutputSurface, but I had a hard time thinking of a way to prevent DidSwapBuffers
and OnSwapBuffersComplete from unexpectedly leaking into the Scheduler that
wasn't racy otherwise.

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface_client.h:39: // if OnSwapBuffersComplete arrives for
the previous OutputSurface.
On 2014/04/08 13:46:01, Sami wrote:
> It feels like this comment belongs to OutputSurface::SwapBuffers() or
elsewhere
> in the implementation, because this isn't something the client has to worry
> about. Also, replace "scheduler" with "client" to keep this more generic.

Sounds good.

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:54: Scheduler::~Scheduler() {}
On 2014/04/08 13:46:01, Sami wrote:
> Should probably stop the time source here.

The timesource cleans itself up, but it wouldn't hurt.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:155: state_machine_.OnSwapBuffersComplete();
On 2014/04/08 13:46:01, Sami wrote:
> That DCHECK in Scheduler::DidSwapBuffers could go here too.

Good idea.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:226:
SetupNextBeginFrameWhenVSyncEnabled(needs_begin_frame);
On 2014/04/08 13:46:01, Sami wrote:
> bikeshed: s/VSync/Throttling/ to keep the terms a little more clearly defined.

I was afraid of confusing vsync throttling with swap-ack throttling. Would it be
ok to s/VSync/VSyncThrottling/ instead?

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:378: // Not if frame production isn't throttled
though, since all frames will be
On 2014/04/08 13:46:01, Sami wrote:
> This is getting a little confusing. Should we perhaps have a separate deque
for
> synthetic BeginFrames or something like ::BeginSyntheticFrame()?

This is for the unthrottled (vsync disabled) frames, which uses neither the
synthetic logic nor the BeginFrame logic. My thinking was that we are always
trying to catch up when vsync is disabled and using the retroactive logic kind
of makes sense in a way and it happens to work out well in the code. It is a
little confusing though, so I could create a separate mechanism for that.

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:69: void SetEstimatedParentDrawTime(base::TimeDelta
draw_time);
On 2014/04/08 13:46:01, Sami wrote:
> Since DrawDurationEstimate() is already in SchedulerClient seems like 
> SetEstimatedParentDrawTime would fit there too. WDYT?

SetEstimatedParentDrawTime is likely to be an async message sent from the
Browser eventually, so I think it makes sense to keep it as a method into the
scheduler.

I was also hoping to move the other estimation logic into the scheduler
eventually, so it's all in one place; at which point, the estimated times won't
be a client call.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:185: scoped_refptr<TimeSource>
time_source_for_synthetic_begin_frames_;
On 2014/04/08 13:46:01, Sami wrote:
> Is there a way we could isolate the synthetic BeginFrame logic to an
> inner/implementation-side class? Eventually we want to get rid of it, so it
> would be convenient to have it all in one place. Also, it feels weird to have
> Scheduler derive from TimeSourceClient since it's more of an implementation
> detail than public scheduler interface.

The goal was to get rid of it before, but I'm not sure if that's feasible
anymore since we'll need it for scheduling the Browser compositor.

To remove the inheritance, I could create an adaptor class or register a
callback rather than a client with the TimeSource.

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

https://codereview.chromium.org/221833009/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(biranderson): Don't use a
hard-coded parent draw time.
On 2014/04/08 13:46:01, Sami wrote:
> Hey no assigning TODOs to others :)

D'oh.

[brianderson, 2014-04-10 23:45:58]

I need to add scheduler tests for BeginUnthrottledFrame and
SyntheticBegFrameSource.

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface.cc:256: client_->DidSwapBuffers();
On 2014/04/08 13:46:01, Sami wrote:
> Should we guard against is_lost_?

Done.

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/221833009/diff/20001/cc/output/output_surface...
cc/output/output_surface_client.h:39: // if OnSwapBuffersComplete arrives for
the previous OutputSurface.
On 2014/04/08 13:46:01, Sami wrote:
> It feels like this comment belongs to OutputSurface::SwapBuffers() or
elsewhere
> in the implementation, because this isn't something the client has to worry
> about. Also, replace "scheduler" with "client" to keep this more generic.

Done.

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:54: Scheduler::~Scheduler() {}
On 2014/04/09 02:52:05, brianderson wrote:
> On 2014/04/08 13:46:01, Sami wrote:
> > Should probably stop the time source here.
> 
> The timesource cleans itself up, but it wouldn't hurt.

Done.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:155: state_machine_.OnSwapBuffersComplete();
On 2014/04/08 13:46:01, Sami wrote:
> That DCHECK in Scheduler::DidSwapBuffers could go here too.

Done.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:226:
SetupNextBeginFrameWhenVSyncEnabled(needs_begin_frame);
On 2014/04/09 02:52:05, brianderson wrote:
> On 2014/04/08 13:46:01, Sami wrote:
> > bikeshed: s/VSync/Throttling/ to keep the terms a little more clearly
defined.
> 
> I was afraid of confusing vsync throttling with swap-ack throttling. Would it
be
> ok to s/VSync/VSyncThrottling/ instead?

Done.

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:378: // Not if frame production isn't throttled
though, since all frames will be
On 2014/04/09 02:52:05, brianderson wrote:
> On 2014/04/08 13:46:01, Sami wrote:
> > This is getting a little confusing. Should we perhaps have a separate deque
> for
> > synthetic BeginFrames or something like ::BeginSyntheticFrame()?
> 
> This is for the unthrottled (vsync disabled) frames, which uses neither the
> synthetic logic nor the BeginFrame logic. My thinking was that we are always
> trying to catch up when vsync is disabled and using the retroactive logic kind
> of makes sense in a way and it happens to work out well in the code. It is a
> little confusing though, so I could create a separate mechanism for that.

Added a BeginUnthrottledFrame with a separate closure. It also doesn't use the
retroactive logic anymore.

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

https://codereview.chromium.org/221833009/diff/20001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:185: scoped_refptr<TimeSource>
time_source_for_synthetic_begin_frames_;
On 2014/04/09 02:52:05, brianderson wrote:
> On 2014/04/08 13:46:01, Sami wrote:
> > Is there a way we could isolate the synthetic BeginFrame logic to an
> > inner/implementation-side class? Eventually we want to get rid of it, so it
> > would be convenient to have it all in one place. Also, it feels weird to
have
> > Scheduler derive from TimeSourceClient since it's more of an implementation
> > detail than public scheduler interface.
> 
> The goal was to get rid of it before, but I'm not sure if that's feasible
> anymore since we'll need it for scheduling the Browser compositor.
> 
> To remove the inheritance, I could create an adaptor class or register a
> callback rather than a client with the TimeSource.

I ended up using an inner class like you originally suggested.

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

https://codereview.chromium.org/221833009/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(biranderson): Don't use a
hard-coded parent draw time.
On 2014/04/08 13:46:01, Sami wrote:
> Hey no assigning TODOs to others :)

Done.

[danakj, 2014-04-11 15:42:22]

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

https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface...
cc/output/output_surface.h:145: // Note: DidSwapBuffers and
OnSwapBuffersComplete should not be called on
In browser process land we've avoided doing things on the wrong OutputSurface by
generating an ID with each output surface and passing it around/binding it in
callbacks to identify the output surface. Then when we do stuff we check if the
id == the current output surface id and can respond accordingly. Would such a
thing work here?

[brianderson, 2014-04-11 20:21:28]

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

https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface...
cc/output/output_surface.h:145: // Note: DidSwapBuffers and
OnSwapBuffersComplete should not be called on
On 2014/04/11 15:42:22, danakj wrote:
> In browser process land we've avoided doing things on the wrong OutputSurface
by
> generating an ID with each output surface and passing it around/binding it in
> callbacks to identify the output surface. Then when we do stuff we check if
the
> id == the current output surface id and can respond accordingly. Would such a
> thing work here?

The id check works for making sure that an Ack for OS1 doesn't affect OS2
*after* we've created OS2.

The is_lost_ check makes sure that an Ack for OS1 doesn't make it to the
scheduler after we lose OS1 but *before* we've created OS2.

[danakj, 2014-04-14 18:19:27]

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

https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface...
cc/output/output_surface.h:145: // Note: DidSwapBuffers and
OnSwapBuffersComplete should not be called on
On 2014/04/11 20:21:28, brianderson wrote:
> On 2014/04/11 15:42:22, danakj wrote:
> > In browser process land we've avoided doing things on the wrong
OutputSurface
> by
> > generating an ID with each output surface and passing it around/binding it
in
> > callbacks to identify the output surface. Then when we do stuff we check if
> the
> > id == the current output surface id and can respond accordingly. Would such
a
> > thing work here?
> 
> The id check works for making sure that an Ack for OS1 doesn't affect OS2
> *after* we've created OS2.
> 
> The is_lost_ check makes sure that an Ack for OS1 doesn't make it to the
> scheduler after we lose OS1 but *before* we've created OS2.

What if we reset the acks in flight and stuff when we create OS2 instead of when
we lose OS1. Could we stop caring about receiving those acks?

[brianderson, 2014-04-14 18:53:58]

On 2014/04/14 18:19:27, danakj wrote:
>
https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface.h
> File cc/output/output_surface.h (right):
> 
>
https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface...
> cc/output/output_surface.h:145: // Note: DidSwapBuffers and
> OnSwapBuffersComplete should not be called on
> On 2014/04/11 20:21:28, brianderson wrote:
> > On 2014/04/11 15:42:22, danakj wrote:
> > > In browser process land we've avoided doing things on the wrong
> OutputSurface
> > by
> > > generating an ID with each output surface and passing it around/binding it
> in
> > > callbacks to identify the output surface. Then when we do stuff we check
if
> > the
> > > id == the current output surface id and can respond accordingly. Would
such
> a
> > > thing work here?
> > 
> > The id check works for making sure that an Ack for OS1 doesn't affect OS2
> > *after* we've created OS2.
> > 
> > The is_lost_ check makes sure that an Ack for OS1 doesn't make it to the
> > scheduler after we lose OS1 but *before* we've created OS2.
> 
> What if we reset the acks in flight and stuff when we create OS2 instead of
when
> we lose OS1. Could we stop caring about receiving those acks?

Although that would make the implementation of each OutputSurface more
straightforward, I think it comes at the cost of us being able to fail fast. The
is_lost_ check allows us to DCHECK in the scheduler that we don't receive an ACK
while the OutputSurface is lost. If an OutputSurface implementation lets an ack
through after it's lost, I think it's also very likely that it might let an ack
through after the new OutputSurface is created.

I imagine it would be very common for acks to arrive before we create the second
OutputSurface, resulting in the race being hard to detect. And, given how
uncommon OutputSurface re-creation is, I think it makes it even more important
to fail fast.

[Sami, 2014-04-16 17:48:22]

Thanks for the fixes. This is looking very good. Just a minor questions below.

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:321: TRACE_EVENT0("cc", "Scheduler::BeginImplFrame");
Did you mean to remove this? Or is this redundant with other trace events now?

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

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:60: protected:
Could be private?

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:323: base::TimeDelta interval = vsync_interval_ >
base::TimeDelta()
When would the vsync interval be zero?

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

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:187: void SetupSyntheticBeginFrames();
nit: Move this before the variable declarations.

[brianderson, 2014-04-16 21:51:30]

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

https://codereview.chromium.org/221833009/diff/40001/cc/output/output_surface...
cc/output/output_surface.h:145: // Note: DidSwapBuffers and
OnSwapBuffersComplete should not be called on
On 2014/04/14 18:19:27, danakj wrote:
> On 2014/04/11 20:21:28, brianderson wrote:
> > On 2014/04/11 15:42:22, danakj wrote:
> > > In browser process land we've avoided doing things on the wrong
> OutputSurface
> > by
> > > generating an ID with each output surface and passing it around/binding it
> in
> > > callbacks to identify the output surface. Then when we do stuff we check
if
> > the
> > > id == the current output surface id and can respond accordingly. Would
such
> a
> > > thing work here?
> > 
> > The id check works for making sure that an Ack for OS1 doesn't affect OS2
> > *after* we've created OS2.
> > 
> > The is_lost_ check makes sure that an Ack for OS1 doesn't make it to the
> > scheduler after we lose OS1 but *before* we've created OS2.
> 
> What if we reset the acks in flight and stuff when we create OS2 instead of
when
> we lose OS1. Could we stop caring about receiving those acks?

Going to go with that solution and avoid being overly defensive in the
Scheduler.

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:321: TRACE_EVENT0("cc", "Scheduler::BeginImplFrame");
On 2014/04/16 17:48:23, Sami wrote:
> Did you mean to remove this? Or is this redundant with other trace events now?

Hmm, I should probably keep it since it won't be clear that a BeginFrame,
BeginRetroFrame, etc became a BeginImplFrame or not.

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

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:60: protected:
On 2014/04/16 17:48:23, Sami wrote:
> Could be private?

Yes.

https://codereview.chromium.org/221833009/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:323: base::TimeDelta interval = vsync_interval_ >
base::TimeDelta()
On 2014/04/16 17:48:23, Sami wrote:
> When would the vsync interval be zero?

I guess it shouldn't be. I'll DCHECK in the CommitVSyncParameters instead.

[brianderson, 2014-04-18 23:32:04]

Rebased and added scheduler tests.

+piman for review of /content.

[Sami, 2014-04-22 18:34:13]

Thanks for the new tests. lgtm as long as Dana's happy with the swap ack
resolution.

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:808: return
!settings_.using_synchronous_renderer_compositor;
Should we also check throttle_frame_production here or is the comment out of
date?

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

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:1306: // When BeginFrame scheduling is
disabled, the scheduler posts it's own
This comment threw me off a bit. Could we say something like "When client-driven
BeginFrames are disabled"? This mode needs a better name :)

[brianderson, 2014-04-22 22:34:49]

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:808: return
!settings_.using_synchronous_renderer_compositor;
On 2014/04/22 18:34:13, Sami wrote:
> Should we also check throttle_frame_production here or is the comment out of
> date?

I will update the comment. The way disabled vsync works now, we will not busy
loop like we used to.

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

https://codereview.chromium.org/221833009/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:1306: // When BeginFrame scheduling is
disabled, the scheduler posts it's own
On 2014/04/22 18:34:13, Sami wrote:
> This comment threw me off a bit. Could we say something like "When
client-driven
> BeginFrames are disabled"? This mode needs a better name :)

Sounds good. Client-drive BeginFrames will make sense from the Scheduler's POV
regardless of what we end up calling it.

[danakj, 2014-04-22 22:36:12]

On Tue, Apr 22, 2014 at 2:34 PM, <skyostil@chromium.org> wrote:

> Thanks for the new tests. lgtm as long as Dana's happy with the swap ack
> resolution.
>

Yup, not adding branches to OutputSurface = me happy.


>
>
> https://codereview.chromium.org/221833009/diff/150001/cc/
> scheduler/scheduler_state_machine.cc
> File cc/scheduler/scheduler_state_machine.cc (right):
>
> https://codereview.chromium.org/221833009/diff/150001/cc/
> scheduler/scheduler_state_machine.cc#newcode808
> cc/scheduler/scheduler_state_machine.cc:808: return
> !settings_.using_synchronous_renderer_compositor;
> Should we also check throttle_frame_production here or is the comment
> out of date?
>
> https://codereview.chromium.org/221833009/diff/150001/cc/
> scheduler/scheduler_unittest.cc
> File cc/scheduler/scheduler_unittest.cc (right):
>
> https://codereview.chromium.org/221833009/diff/150001/cc/
> scheduler/scheduler_unittest.cc#newcode1306
> cc/scheduler/scheduler_unittest.cc:1306: // When BeginFrame scheduling
> is disabled, the scheduler posts it's own
> This comment threw me off a bit. Could we say something like "When
> client-driven BeginFrames are disabled"? This mode needs a better name
> :)
>
> https://codereview.chromium.org/221833009/
>

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

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

https://codereview.chromium.org/221833009/diff/150001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (left):

https://codereview.chromium.org/221833009/diff/150001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:182:
OutputSurface::SwapBuffers(frame);
I shouldn't have removed this. Will add it back.

[brianderson, 2014-04-23 20:35:46]

+piman for OWNERS review of content.

[brianderson, 2014-04-23 21:10:01]

https://codereview.chromium.org/221833009/diff/190001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/221833009/diff/190001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:52: class CC_EXPORT BeginFrameSourceClient {
This isn't actually used anymore. Will delete.

[piman, 2014-04-24 20:27:56]

I did not look at the cc parts in detail, but this looks like a good
simplification overall. Couple of nits, then lgtm.

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

https://codereview.chromium.org/221833009/diff/210001/cc/output/output_surfac...
cc/output/output_surface.cc:344: void OutputSurface::OnSwapBuffersComplete() {
nit: Is anything using this? I think most (all?) derived classes were changed to
call the client directly. Either remove, or make all derived classes consistent
by calling this?

https://codereview.chromium.org/221833009/diff/210001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/221833009/diff/210001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:72:
needs_begin_frame_(false),
nit: nothing sets/reads this AFAICT. Remove?

[brianderson, 2014-04-24 21:33:42]

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

https://codereview.chromium.org/221833009/diff/210001/cc/output/output_surfac...
cc/output/output_surface.cc:344: void OutputSurface::OnSwapBuffersComplete() {
On 2014/04/24 20:27:56, piman wrote:
> nit: Is anything using this? I think most (all?) derived classes were changed
to
> call the client directly. Either remove, or make all derived classes
consistent
> by calling this?

I initially got rid of this method, but put it back because I was worried about
posting a task bound to the client, which might outlive this OutputSurface,
making it possible for a SwapBuffersComplete for an old OutputSurface to arrive
after the new OutputSurface is created.

With this method, we can post a task with a weak pointer to the OutputSurface
that will automatically cancel any pending calls.

I'll add a comment explaining why this method exists since it isn't clear.

https://codereview.chromium.org/221833009/diff/210001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/221833009/diff/210001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:72:
needs_begin_frame_(false),
On 2014/04/24 20:27:56, piman wrote:
> nit: nothing sets/reads this AFAICT. Remove?

I removed needs_begin_frame_ from the base OutputSurface class so needed to add
it to this subclass. Although I didn't add any new logic to this subclass to use
needs_begin_frame_, there is existing logic that needs it.

[brianderson, 2014-04-25 01:36:11]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-25 02:03:49]

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

[commit-bot: I haz the power, 2014-04-25 02:50:28]

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

[commit-bot: I haz the power, 2014-04-25 02:50:32]

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

[danakj, 2014-04-25 15:27:04]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:48:13]

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

[commit-bot: I haz the power, 2014-04-25 22:38:04]

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

[commit-bot: I haz the power, 2014-04-25 22:38:05]

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

[brianderson, 2014-04-25 23:50:13]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:54:49]

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

[commit-bot: I haz the power, 2014-04-26 10:06:09]

Change committed as 266348