cc: Always use SetNeedsBeginFrame to request the next BeginFrame

cc: Always use SetNeedsBeginFrame to request the next BeginFrame

This avoids relying on SwapBuffers to implicitly trigger the next
BeginFrame. Only a single code path is used to request the next
BeginFrame now: SetNeedsBeginFrame(true).

This avoids issues where OutputSurface subclasses might not call
OutputSurface::DidSwap() when they need to or when we early out
a swap request and don't actually swap anything.

BUG=289755

[enne (OOO), 2013-09-13 17:48:44]

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

https://codereview.chromium.org/23892022/diff/1/cc/output/output_surface.cc#o...
cc/output/output_surface.cc:195: begin_frame_pending_ = false;
Pardon my ongoing confusion, but I still can't wrap my head around what
begin_frame_pending_ is supposed to mean.  I think I see what it's doing, but I
can't get past the naming.

begin_frame_pending_ seems to imply that BeginFrame has been sent from
OutputSurface to the Scheduler and that the Scheduler is not ready for another
one until another OutputSurface::SetNeedsBeginFrame(true) has been called.  So
it almost seems like !begin_frame_pending_ is equivalent to
client_ready_for_begin_frame_?

https://codereview.chromium.org/23892022/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23892022/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:745: if
(last_frame_number_swap_performed_ == current_frame_number_)
Just as a sanity check, the bugfix here is that although the state machine is
swapping, maybe some output surface implementation doesn't swap, and thus the
next begin frame isn't unblocked.  By moving the check down a level (or two) to
the state machine, it knows when swaps have really happened and can request the
next begin frame without depending on the output surface to also unblock.

[brianderson, 2013-09-13 18:24:45]

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

https://codereview.chromium.org/23892022/diff/1/cc/output/output_surface.cc#o...
cc/output/output_surface.cc:195: begin_frame_pending_ = false;
On 2013/09/13 17:48:44, enne wrote:
> Pardon my ongoing confusion, but I still can't wrap my head around what
> begin_frame_pending_ is supposed to mean.  I think I see what it's doing, but
I
> can't get past the naming.
> 
> begin_frame_pending_ seems to imply that BeginFrame has been sent from
> OutputSurface to the Scheduler and that the Scheduler is not ready for another
> one until another OutputSurface::SetNeedsBeginFrame(true) has been called.  So
> it almost seems like !begin_frame_pending_ is equivalent to
> client_ready_for_begin_frame_?

Your understand is correct.  begin_frame_pending_ is an ambiguous name since it
might refer to the BeginFrame message, the emulated BeginFrame, or the client's
BeginFrame.  client_ready_for_begin_frame_ sounds like an excellent replacement.

https://codereview.chromium.org/23892022/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23892022/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:745: if
(last_frame_number_swap_performed_ == current_frame_number_)
On 2013/09/13 17:48:44, enne wrote:
> Just as a sanity check, the bugfix here is that although the state machine is
> swapping, maybe some output surface implementation doesn't swap, and thus the
> next begin frame isn't unblocked.  By moving the check down a level (or two)
to
> the state machine, it knows when swaps have really happened and can request
the
> next begin frame without depending on the output surface to also unblock.

Your understand is correct. The Scheduler can accept the next BeginFrame after a
swap request whether or not the swap actually occurred. ToT has the Scheduler
explicitly request the next BeginFrame when the OutputSurface's swap didn't
implicitly request the next BeginFrame. Having the Scheduler detect that
implicit request is too confusing and error prone, so this patch only uses the
explicit request.

This particular diff, however, is used to avoid negative glitches in
SetNeedsBeginFrame between two BeginFrames. Since we just drew, it's likely that
we don't think we need another BeginFrame for a fraction of a VSync until we get
another user-input or draw trigger.

A negative glitch is bad because it will propagate all the way to the Browser,
at which point the Browser might sample the SetNeedsBeginFrame value at an
inopportune time, delaying the BeginFrame for a VSync.

[brianderson, 2013-09-13 18:37:43]

ptal

[enne (OOO), 2013-09-13 18:39:18]

lgtm

https://codereview.chromium.org/23892022/diff/12001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23892022/diff/12001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:746: // at an innpportune time, delaying
the next BeginFrame.
speling

[commit-bot: I haz the power, 2013-09-13 18:43:50]

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

[brianderson, 2013-09-13 19:51:40]

This conflicts with epenner's patch in:
https://codereview.chromium.org/23495022/

I'm going to go ahead and rebase my patch on top of his, since I have other
patches to rebase on top of his anyway.

[brianderson, 2013-09-13 20:15:56]

Base files missing (again again again!)
Rebased patch here:
https://codereview.chromium.org/23498035