Unified BeginFrame scheduling

cc/scheduler/scheduler.cc

Index: cc/scheduler/scheduler.cc
diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc
index a20bfd2b3b0e2c6eece49c94fcfd66258bc5f5f3..9e8923fa9aa5ced5ecb6083c8916ef1c838e6777 100644
--- a/cc/scheduler/scheduler.cc
+++ b/cc/scheduler/scheduler.cc
@@ -91,6 +91,7 @@ Scheduler::Scheduler(
       state_machine_(scheduler_settings),
       inside_process_scheduled_actions_(false),
       inside_action_(SchedulerStateMachine::ACTION_NONE),
+      begin_frame_requested_(false),
       weak_factory_(this) {
   TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler"),
                "Scheduler::Scheduler",
@@ -299,7 +300,10 @@ void Scheduler::SetupNextBeginFrameWhenVSyncThrottlingEnabled(
       // Always request the BeginFrame immediately if it wasn't needed before.
       (needs_begin_frame && !last_set_needs_begin_frame_) ||
       // Only stop requesting BeginFrames after a deadline.
-      (!needs_begin_frame && last_set_needs_begin_frame_ && at_end_of_deadline);
+      (!needs_begin_frame &&
+       last_set_needs_begin_frame_ &&
+       at_end_of_deadline &&
+       !begin_frame_requested_);

[brianderson, 2014/08/21 00:13:59]

Instead of taking begin_frame_requested_ into account here, would it make sense
to take it into account from inside SchedulerStateMachine::BeginFrameNeeded?

[simonhong, 2014/08/26 08:24:47]

Yeap, handling this state by SchdulerStateMachine would make sense!

 
   if (should_call_set_needs_begin_frame) {
     if (settings_.begin_frame_scheduling_enabled) {
@@ -408,6 +412,13 @@ void Scheduler::BeginFrame(const BeginFrameArgs& args) {
   DCHECK(settings_.throttle_frame_production);
 
   BeginFrameArgs adjusted_args(args);
+
+  if (settings_.begin_frame_publisher && begin_frame_requested_) {
+    // |arg.deadline| should be adjusted by subscribed Scheduler.
+    client_->SendBeginFrame(args);
+    begin_frame_requested_ = false;

[brianderson, 2014/08/21 00:13:59]

The child Schedulers should be setting begin_frame_requested explicitly to false
and the parent Scheduler should not set it to false implicitly like this. That
way we reduce the number of messages between the Renderer and Browser.

[simonhong, 2014/08/26 08:24:47]

Yeap, calling SetChildrenNeedBeginFrames() from BeginFrameManager can make much
simpler :)

+  }
+
   adjusted_args.deadline -= EstimatedParentDrawTime();
 
   bool should_defer_begin_frame;
@@ -612,6 +623,20 @@ bool Scheduler::IsBeginMainFrameSent() const {
          SchedulerStateMachine::COMMIT_STATE_BEGIN_MAIN_FRAME_SENT;
 }
 
+void Scheduler::BeginFrameRequested() {
+  DCHECK(settings_.begin_frame_publisher);
+  DCHECK(!begin_frame_requested_);
+
+  begin_frame_requested_ = true;
+
+  // If frame source is already activated, next BeginFrame will be scheduled.
+  if (synthetic_begin_frame_source_->IsActive())
+    return;
+
+  synthetic_begin_frame_source_->SetNeedsBeginFrame(true,

[brianderson, 2014/08/21 00:13:59]

Instead of calling SetNeedsBeginFrame direcly here, please call
ProcessScheduledActions instead, which will go through the common way of setting
up the value for SetNeedsBeginFrame.

That will properly handle the vsync disabled case.

[simonhong, 2014/08/26 08:24:47]

Done.

+                                                    &begin_retro_frame_args_);
+}
+
 void Scheduler::DrawAndSwapIfPossible() {
   DrawResult result = client_->ScheduledActionDrawAndSwapIfPossible();
   state_machine_.DidDrawIfPossibleCompleted(result);