cc: Throttle swaps in Scheduler instead of OutputSurface

cc/output/output_surface.cc

Index: cc/output/output_surface.cc
diff --git a/cc/output/output_surface.cc b/cc/output/output_surface.cc
index 3aaa1a1768637829e2f5fe5dfdb3b9c221a0563c..777c46c00b80bd5107e543163539b838e5ba27c5 100644
--- a/cc/output/output_surface.cc
+++ b/cc/output/output_surface.cc
@@ -46,11 +46,12 @@ namespace cc {
 OutputSurface::OutputSurface(scoped_refptr<ContextProvider> context_provider)
     : context_provider_(context_provider),
       device_scale_factor_(-1),
-      max_frames_pending_(0),
-      pending_swap_buffers_(0),
+      begin_frame_interval_(BeginFrameArgs::DefaultInterval()),
+      throttle_frame_production_(true),
       needs_begin_frame_(false),
       client_ready_for_begin_frame_(true),
       client_(NULL),
+      is_lost_(false),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false),
       weak_ptr_factory_(this),
@@ -59,11 +60,12 @@ OutputSurface::OutputSurface(scoped_refptr<ContextProvider> context_provider)
 OutputSurface::OutputSurface(scoped_ptr<SoftwareOutputDevice> software_device)
     : software_device_(software_device.Pass()),
       device_scale_factor_(-1),
-      max_frames_pending_(0),
-      pending_swap_buffers_(0),
+      begin_frame_interval_(BeginFrameArgs::DefaultInterval()),
+      throttle_frame_production_(true),
       needs_begin_frame_(false),
       client_ready_for_begin_frame_(true),
       client_(NULL),
+      is_lost_(false),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false),
       weak_ptr_factory_(this),
@@ -74,47 +76,37 @@ OutputSurface::OutputSurface(scoped_refptr<ContextProvider> context_provider,
     : context_provider_(context_provider),
       software_device_(software_device.Pass()),
       device_scale_factor_(-1),
-      max_frames_pending_(0),
-      pending_swap_buffers_(0),
+      begin_frame_interval_(BeginFrameArgs::DefaultInterval()),
+      throttle_frame_production_(true),
       needs_begin_frame_(false),
       client_ready_for_begin_frame_(true),
       client_(NULL),
+      is_lost_(false),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false),
       weak_ptr_factory_(this),
       gpu_latency_history_(kGpuLatencyHistorySize) {}
 
+void OutputSurface::SetThrottleFrameProduction(bool enable) {
+  DCHECK(!frame_rate_controller_);
+  throttle_frame_production_ = enable;
+}
+
 void OutputSurface::InitializeBeginFrameEmulation(
     base::SingleThreadTaskRunner* task_runner,
-    bool throttle_frame_production,
     base::TimeDelta interval) {
-  if (throttle_frame_production) {
-    scoped_refptr<DelayBasedTimeSource> time_source;
-    if (gfx::FrameTime::TimestampsAreHighRes())
-      time_source = DelayBasedTimeSourceHighRes::Create(interval, task_runner);
-    else
-      time_source = DelayBasedTimeSource::Create(interval, task_runner);
-    frame_rate_controller_.reset(new FrameRateController(time_source));
-  } else {
-    frame_rate_controller_.reset(new FrameRateController(task_runner));
-  }
+  DCHECK(throttle_frame_production_);
+  scoped_refptr<DelayBasedTimeSource> time_source;
+  if (gfx::FrameTime::TimestampsAreHighRes())
+    time_source = DelayBasedTimeSourceHighRes::Create(interval, task_runner);
+  else
+    time_source = DelayBasedTimeSource::Create(interval, task_runner);
+  frame_rate_controller_.reset(new FrameRateController(time_source));
 
   frame_rate_controller_->SetClient(this);
-  frame_rate_controller_->SetMaxSwapsPending(max_frames_pending_);
   frame_rate_controller_->SetDeadlineAdjustment(
       capabilities_.adjust_deadline_for_parent ?
           BeginFrameArgs::DefaultDeadlineAdjustment() : base::TimeDelta());
-
-  // The new frame rate controller will consume the swap acks of the old
-  // frame rate controller, so we set that expectation up here.
-  for (int i = 0; i < pending_swap_buffers_; i++)
-    frame_rate_controller_->DidSwapBuffers();
-}
-
-void OutputSurface::SetMaxFramesPending(int max_frames_pending) {
-  if (frame_rate_controller_)
-    frame_rate_controller_->SetMaxSwapsPending(max_frames_pending);
-  max_frames_pending_ = max_frames_pending;
 }
 
 void OutputSurface::CommitVSyncParameters(base::TimeTicks timebase,
@@ -125,17 +117,14 @@ void OutputSurface::CommitVSyncParameters(base::TimeTicks timebase,
                (timebase - base::TimeTicks()).InSecondsF(),
                "interval",
                interval.InSecondsF());
+  begin_frame_interval_ = interval;
   if (frame_rate_controller_)
     frame_rate_controller_->SetTimebaseAndInterval(timebase, interval);
 }
 
-void OutputSurface::FrameRateControllerTick(bool throttled,
-                                            const BeginFrameArgs& args) {
+void OutputSurface::FrameRateControllerTick(const BeginFrameArgs& args) {
   DCHECK(frame_rate_controller_);
-  if (throttled)
-    skipped_begin_frame_args_ = args;
-  else
-    BeginFrame(args);
+  BeginFrame(args);
 }
 
 // Forwarded to OutputSurfaceClient
@@ -148,25 +137,29 @@ void OutputSurface::SetNeedsBeginFrame(bool enable) {
   TRACE_EVENT1("cc", "OutputSurface::SetNeedsBeginFrame", "enable", enable);
   needs_begin_frame_ = enable;
   client_ready_for_begin_frame_ = true;
-  if (frame_rate_controller_) {
+  if (!throttle_frame_production_) {
+    if (enable) {
+      base::TimeTicks frame_time = gfx::FrameTime::Now();
+      base::TimeTicks deadline = frame_time + begin_frame_interval_;
+      skipped_begin_frame_args_ =
+          BeginFrameArgs::Create(frame_time, deadline, begin_frame_interval_);
+    }
+  } else if (frame_rate_controller_) {
     BeginFrameArgs skipped = frame_rate_controller_->SetActive(enable);
     if (skipped.IsValid())
       skipped_begin_frame_args_ = skipped;
   }
+
   if (needs_begin_frame_)
     PostCheckForRetroactiveBeginFrame();
 }
 
 void OutputSurface::BeginFrame(const BeginFrameArgs& args) {
-  TRACE_EVENT2("cc",
+  TRACE_EVENT1("cc",
                "OutputSurface::BeginFrame",
                "client_ready_for_begin_frame_",
-               client_ready_for_begin_frame_,
-               "pending_swap_buffers_",
-               pending_swap_buffers_);
-  if (!needs_begin_frame_ || !client_ready_for_begin_frame_ ||
-      (pending_swap_buffers_ >= max_frames_pending_ &&
-       max_frames_pending_ > 0)) {
+               client_ready_for_begin_frame_);
+  if (!needs_begin_frame_ || !client_ready_for_begin_frame_) {
     skipped_begin_frame_args_ = args;
   } else {
     client_ready_for_begin_frame_ = false;
@@ -201,28 +194,23 @@ void OutputSurface::PostCheckForRetroactiveBeginFrame() {
 void OutputSurface::CheckForRetroactiveBeginFrame() {
   TRACE_EVENT0("cc", "OutputSurface::CheckForRetroactiveBeginFrame");
   check_for_retroactive_begin_frame_pending_ = false;
-  if (gfx::FrameTime::Now() < RetroactiveBeginFrameDeadline())
+  if (!throttle_frame_production_ ||
+      gfx::FrameTime::Now() < RetroactiveBeginFrameDeadline())
     BeginFrame(skipped_begin_frame_args_);
 }
 
 void OutputSurface::DidSwapBuffers() {
-  pending_swap_buffers_++;
-  TRACE_EVENT1("cc", "OutputSurface::DidSwapBuffers",
-               "pending_swap_buffers_", pending_swap_buffers_);
+  if (is_lost_)

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

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?

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

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.

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

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

+    return;
+  TRACE_EVENT0("cc", "OutputSurface::DidSwapBuffers");
   client_->DidSwapBuffers();
-  if (frame_rate_controller_)
-    frame_rate_controller_->DidSwapBuffers();
-  PostCheckForRetroactiveBeginFrame();
 }
 
 void OutputSurface::OnSwapBuffersComplete() {
-  pending_swap_buffers_--;
-  TRACE_EVENT1("cc", "OutputSurface::OnSwapBuffersComplete",
-               "pending_swap_buffers_", pending_swap_buffers_);
-  client_->OnSwapBuffersComplete();
-  if (frame_rate_controller_)
-    frame_rate_controller_->DidSwapBuffersComplete();
-  PostCheckForRetroactiveBeginFrame();
+  if (is_lost_)

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

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?

+    return;
+  TRACE_EVENT0("cc", "OutputSurface::OnSwapBuffersComplete");
+  client_->DidSwapBuffersComplete();
 }
 
 void OutputSurface::ReclaimResources(const CompositorFrameAck* ack) {
@@ -232,13 +220,13 @@ void OutputSurface::ReclaimResources(const CompositorFrameAck* ack) {
 void OutputSurface::DidLoseOutputSurface() {
   TRACE_EVENT0("cc", "OutputSurface::DidLoseOutputSurface");
   client_ready_for_begin_frame_ = true;
-  pending_swap_buffers_ = 0;
   skipped_begin_frame_args_ = BeginFrameArgs();
   if (frame_rate_controller_)
     frame_rate_controller_->SetActive(false);
   pending_gpu_latency_query_ids_.clear();
   available_gpu_latency_query_ids_.clear();
   client_->DidLoseOutputSurface();
+  is_lost_ = true;
 }
 
 void OutputSurface::SetExternalStencilTest(bool enabled) {