cc: Consider Surface active if frame received recently

cc/surfaces/display_scheduler.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/surfaces/display_scheduler.h"
 
 #include <vector>
 
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 10 matching lines @@
       task_runner_(task_runner),
       output_surface_lost_(false),
       root_surface_resources_locked_(true),
       inside_begin_frame_deadline_interval_(false),
       needs_draw_(false),
       entire_display_damaged_(false),
       all_active_child_surfaces_ready_to_draw_(false),
       pending_swaps_(0),
       max_pending_swaps_(max_pending_swaps),
       root_surface_damaged_(false),
-      expect_damage_from_root_surface_(false),
+      root_surface_active_(false),
+      current_active_child_surface_ids_index_(0),
       weak_ptr_factory_(this) {
   begin_frame_source_->AddObserver(this);
   begin_frame_deadline_closure_ = base::Bind(
       &DisplayScheduler::OnBeginFrameDeadline, weak_ptr_factory_.GetWeakPtr());
 }
 
 DisplayScheduler::~DisplayScheduler() {
   begin_frame_source_->RemoveObserver(this);
 }
 
@@ skipping 30 matching lines @@
 void DisplayScheduler::SurfaceDamaged(SurfaceId surface_id) {
   TRACE_EVENT1("cc", "DisplayScheduler::SurfaceDamaged", "surface_id",
                surface_id.id);
 
   needs_draw_ = true;
 
   if (surface_id == root_surface_id_) {
     root_surface_damaged_ = true;
   } else {
     child_surface_ids_damaged_.insert(surface_id);
+    UpdateFutureActiveChildSurfaceIDs(surface_id);

[sunnyps, 2015/07/23 21:10:38]

nit: Instead of a separate method, just update the active surfaces sets here and
add a comment.

 
     // TODO(mithro): Use hints from SetNeedsBeginFrames and SwapAborts.
     all_active_child_surfaces_ready_to_draw_ = base::STLIncludes(
-        child_surface_ids_damaged_, child_surface_ids_to_expect_damage_from_);
+        child_surface_ids_damaged_, CurrentActiveChildSurfaceIDs());
   }
 
   begin_frame_source_->SetNeedsBeginFrames(!output_surface_lost_);
   ScheduleBeginFrameDeadline();
 }
 
 void DisplayScheduler::OutputSurfaceLost() {
   TRACE_EVENT0("cc", "DisplayScheduler::OutputSurfaceLost");
   output_surface_lost_ = true;
   begin_frame_source_->SetNeedsBeginFrames(false);
   ScheduleBeginFrameDeadline();
 }
 
 void DisplayScheduler::DrawAndSwap() {
   TRACE_EVENT0("cc", "DisplayScheduler::DrawAndSwap");
   DCHECK_LT(pending_swaps_, max_pending_swaps_);
   DCHECK(!output_surface_lost_);
 
   bool success = client_->DrawAndSwap();
   if (!success)
     return;
 
-  child_surface_ids_to_expect_damage_from_ =
-      base::STLSetIntersection<std::vector<SurfaceId>>(
-          child_surface_ids_damaged_, child_surface_ids_damaged_prev_);
+  UpdateActiveSurfaces();

[sunnyps, 2015/07/23 21:10:38]

nit: No need for a separate method here. It's easier to read if all the updating
happens here.

 
-  child_surface_ids_damaged_prev_.swap(child_surface_ids_damaged_);
-  child_surface_ids_damaged_.clear();
-
+  // Update state regarding what needs to be drawn.
   needs_draw_ = false;
   entire_display_damaged_ = false;
-  all_active_child_surfaces_ready_to_draw_ =
-      child_surface_ids_to_expect_damage_from_.empty();
-
-  expect_damage_from_root_surface_ = root_surface_damaged_;
   root_surface_damaged_ = false;
+  child_surface_ids_damaged_.clear();
 }
 
 bool DisplayScheduler::OnBeginFrameDerivedImpl(const BeginFrameArgs& args) {
   base::TimeTicks now = base::TimeTicks::Now();
   TRACE_EVENT2("cc", "DisplayScheduler::BeginFrame", "args", args.AsValue(),
                "now", now);
 
   // If we get another BeginFrame before the previous deadline,
   // synchronously trigger the previous deadline before progressing.
   if (inside_begin_frame_deadline_interval_) {
@@ skipping 35 matching lines @@
            current_begin_frame_args_.interval;
   }
 
   // TODO(mithro): Be smarter about resize deadlines.
   if (entire_display_damaged_) {
     TRACE_EVENT_INSTANT0("cc", "Entire display damaged",
                          TRACE_EVENT_SCOPE_THREAD);
     return base::TimeTicks();
   }
 
-  bool root_ready_to_draw =
-      !expect_damage_from_root_surface_ || root_surface_damaged_;
+  bool root_ready_to_draw = !root_surface_active_ || root_surface_damaged_;
 
   if (all_active_child_surfaces_ready_to_draw_ && root_ready_to_draw) {
     TRACE_EVENT_INSTANT0("cc", "All active surfaces ready",
                          TRACE_EVENT_SCOPE_THREAD);
     return base::TimeTicks();
   }
 
   // Use an earlier deadline if we are only waiting for the root surface
   // in case our expect_damage_from_root_surface heuristic is incorrect.
   // TODO(mithro): Replace this with SetNeedsBeginFrame and SwapAbort
   // logic.
-  if (all_active_child_surfaces_ready_to_draw_ &&
-      expect_damage_from_root_surface_) {
+  if (all_active_child_surfaces_ready_to_draw_ && root_surface_active_) {

[sunnyps, 2015/07/23 21:10:38]

nit: Rename to child_surfaces_ready_to_draw_

[brianderson, 2015/07/23 22:21:48]

Done.

     TRACE_EVENT_INSTANT0("cc", "Waiting for damage from root surface",
                          TRACE_EVENT_SCOPE_THREAD);
     // This adjusts the deadline by DefaultEstimatedParentDrawTime for
     // a second time. The first one represented the Surfaces draw to display
     // latency. This one represents root surface commit+raster+draw latency.
     // We treat the root surface differently since it lives on the same thread
     // as Surfaces and waiting for it too long may push out the Surfaces draw.
     // If we also assume the root surface is fast to start a commit after the
     // beginning of a frame, it'll have a chance to lock its resources, which
     // will cause us to wait for it to unlock its resources above.
@@ skipping 40 matching lines @@
                                 begin_frame_deadline_task_.callback(), delta);
   TRACE_EVENT2("cc", "Using new deadline", "delta", delta.ToInternalValue(),
                "desired_deadline", desired_deadline);
 }
 
 void DisplayScheduler::AttemptDrawAndSwap() {
   inside_begin_frame_deadline_interval_ = false;
   begin_frame_deadline_task_.Cancel();
   begin_frame_deadline_task_time_ = base::TimeTicks();
 
-  if (needs_draw_ && !output_surface_lost_) {
-    if (pending_swaps_ < max_pending_swaps_ && !root_surface_resources_locked_)
-      DrawAndSwap();
+  bool stay_active =
+      !output_surface_lost_ && (needs_draw_ || root_surface_active_ ||
+                                !CurrentActiveChildSurfaceIDs().empty());
+

[sunnyps, 2015/07/23 21:10:38]

nit: This could be better structured as:

if (!stay_active)
  DoSomething;

if (!needs_draw)
  DoSomething;

if (pending_swaps >= max_pending_swaps || root_surface_resources_locked)
  return;

DrawAndSwap();

[brianderson, 2015/07/23 22:21:48]

Done.

+  if (stay_active) {
+    if (needs_draw_) {
+      if (pending_swaps_ < max_pending_swaps_ &&
+          !root_surface_resources_locked_)
+        DrawAndSwap();
+    } else {
+      // In order to properly go idle, make sure to update expectations that
+      // will cause |stay_active| to go false even if we have nothing to draw.
+      // Verify no child surfaces are currently damaged, since
+      // UpdateSurfaceDamageExpectatations will clear that list.
+      DCHECK(child_surface_ids_damaged_.empty());
+      DCHECK(!entire_display_damaged_);
+      DCHECK(!root_surface_damaged_);
+      UpdateActiveSurfaces();
+    }
   } else {
     // We are going idle, so reset expectations.
-    child_surface_ids_to_expect_damage_from_.clear();
-    child_surface_ids_damaged_prev_.clear();
-    child_surface_ids_damaged_.clear();
-    all_active_child_surfaces_ready_to_draw_ = true;
-    expect_damage_from_root_surface_ = false;
-
+    ClearActiveSurfaces();

[sunnyps, 2015/07/23 21:10:38]

nit: Probably doesn't need a separate method.

     begin_frame_source_->SetNeedsBeginFrames(false);
   }
 }
 
 void DisplayScheduler::OnBeginFrameDeadline() {
   TRACE_EVENT0("cc", "DisplayScheduler::OnBeginFrameDeadline");
 
   AttemptDrawAndSwap();
   begin_frame_source_->DidFinishFrame(0);
 }
 
 void DisplayScheduler::DidSwapBuffers() {
   pending_swaps_++;
   TRACE_EVENT1("cc", "DisplayScheduler::DidSwapBuffers", "pending_frames",
                pending_swaps_);
 }
 
 void DisplayScheduler::DidSwapBuffersComplete() {
   pending_swaps_--;
   TRACE_EVENT1("cc", "DisplayScheduler::DidSwapBuffersComplete",
                "pending_frames", pending_swaps_);
   ScheduleBeginFrameDeadline();
 }
 
+std::set<SurfaceId>& DisplayScheduler::CurrentActiveChildSurfaceIDs() {

[sunnyps, 2015/07/23 21:10:38]

nit: inline this

+  return active_child_surface_ids_[current_active_child_surface_ids_index_];
+}
+
+void DisplayScheduler::UpdateFutureActiveChildSurfaceIDs(SurfaceId surface_id) {
+  for (int i = 0; i < kActiveChildSurfaceIdsSize; i++)
+    active_child_surface_ids_[i].insert(surface_id);
+}
+
+void DisplayScheduler::ClearActiveSurfaces() {
+  root_surface_active_ = false;
+
+  current_active_child_surface_ids_index_ = 0;
+  for (int i = 0; i < kActiveChildSurfaceIdsSize; i++)
+    active_child_surface_ids_[i].clear();
+
+  all_active_child_surfaces_ready_to_draw_ = true;
+}
+
+void DisplayScheduler::UpdateActiveSurfaces() {
+  root_surface_active_ = root_surface_damaged_;
+
+  CurrentActiveChildSurfaceIDs().clear();
+
+  current_active_child_surface_ids_index_++;

[sunnyps, 2015/07/23 21:10:38]

nit: index = (index + 1) % N

[brianderson, 2015/07/23 22:21:49]

Done.

+  if (current_active_child_surface_ids_index_ >= kActiveChildSurfaceIdsSize)
+    current_active_child_surface_ids_index_ = 0;
+
+  all_active_child_surfaces_ready_to_draw_ =

[sunnyps, 2015/07/23 21:10:38]

nit: This could be an inline function.

[brianderson, 2015/07/23 22:21:48]

My goal in writing all the helper methods was to put all the
active_child_surface_ids_ logic in one place. This file isn't that big though,
so I'll go ahead and inline the methods that are not used more than once.

UpdateActiveSurfaces is the only one used more than once and will be the only
one I keep.

+      CurrentActiveChildSurfaceIDs().empty();
+}
+
 }  // namespace cc