cc: Consider Surface active if frame received recently

cc/surfaces/display_scheduler.cc

Index: cc/surfaces/display_scheduler.cc
diff --git a/cc/surfaces/display_scheduler.cc b/cc/surfaces/display_scheduler.cc
index 98f52e52b60ca4c53a846032eb57eaf77017ef1e..61bdecb0bbacc0f09fedee76ec5bd2a35d513106 100644
--- a/cc/surfaces/display_scheduler.cc
+++ b/cc/surfaces/display_scheduler.cc
@@ -28,7 +28,8 @@ DisplayScheduler::DisplayScheduler(DisplaySchedulerClient* client,
       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(
@@ -79,10 +80,11 @@ void DisplayScheduler::SurfaceDamaged(SurfaceId 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_);
@@ -105,20 +107,13 @@ void DisplayScheduler::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_);
-
-  child_surface_ids_damaged_prev_.swap(child_surface_ids_damaged_);
-  child_surface_ids_damaged_.clear();
+  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.

 
+  // 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) {
@@ -174,8 +169,7 @@ base::TimeTicks DisplayScheduler::DesiredBeginFrameDeadlineTime() {
     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",
@@ -187,8 +181,7 @@ base::TimeTicks DisplayScheduler::DesiredBeginFrameDeadlineTime() {
   // 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
@@ -249,17 +242,28 @@ void DisplayScheduler::AttemptDrawAndSwap() {
   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);
   }
 }
@@ -284,4 +288,36 @@ void DisplayScheduler::DidSwapBuffersComplete() {
   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