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 78 matching lines @@
   ScheduleBeginFrameDeadline();
 }
 
 void DisplayScheduler::OutputSurfaceLost() {
   TRACE_EVENT0("cc", "DisplayScheduler::OutputSurfaceLost");
   output_surface_lost_ = true;
   begin_frame_source_->SetNeedsBeginFrames(false);
   ScheduleBeginFrameDeadline();
 }
 
+void DisplayScheduler::UpdateSurfaceDamageExpectations() {
+  expect_damage_from_root_surface_ = root_surface_damaged_;

[sunnyps, 2015/07/23 03:02:40]

Set root_surface_damaged_ to false here instead of below.

[brianderson, 2015/07/23 18:21:46]

This will work for the case where this method is called on line 126, however it
would be incorrect for the call on 277, which is why I kept it out.

+
+  std::vector<SurfaceId> partial_union =
+      base::STLSetUnion<std::vector<SurfaceId>>(
+          child_surface_ids_damaged_, child_surface_ids_damaged_prev_);
+  child_surface_ids_to_expect_damage_from_ =
+      base::STLSetUnion<std::vector<SurfaceId>>(
+          partial_union, child_surface_ids_damaged_prev_prev_);
+
+  child_surface_ids_damaged_prev_prev_.swap(child_surface_ids_damaged_prev_);
+  child_surface_ids_damaged_prev_.swap(child_surface_ids_damaged_);
+  child_surface_ids_damaged_.clear();
+
+  all_active_child_surfaces_ready_to_draw_ =
+      child_surface_ids_to_expect_damage_from_.empty();
+}
+
 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_);
+  UpdateSurfaceDamageExpectations();
 
-  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;
 }
 
 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.
@@ skipping 111 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_ || expect_damage_from_root_surface_ ||
+                      child_surface_ids_to_expect_damage_from_.size() != 0);

[sunnyps, 2015/07/23 03:02:40]

Can you use all_active_child_surfaces_ready_to_draw_ here?

[brianderson, 2015/07/23 18:21:46]

I think that would work, however
"child_surface_ids_to_expect_damage_from_.size() != 0" expresses the intent more
directly and is more orthogonal to the other conditions since
all_active_child_surfaces_ready_to_draw_ has some overlap in meaning with
needs_draw_.

I should probably change this to the following though:
!child_surface_ids_to_expect_damage_from_.empty()

+
+  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.

[sunnyps, 2015/07/23 03:02:40]

nit: "stay_active" instead of "stay active"

[brianderson, 2015/07/23 18:21:46]

Looks like it's already stay_active, but I'll add |'s around it.

+      // Verify no child surfaces are currently damaged, since
+      // UpdateSurfaceDamageExpectatations will clear that list.
+      DCHECK(child_surface_ids_damaged_.empty());

[sunnyps, 2015/07/23 03:02:40]

Can probably add more DCHECKs here - like DCHECK(!entire_display_damaged_) etc.

[brianderson, 2015/07/23 18:21:46]

Sounds good. I will added a DCHECK(!root_surface_damaged_) too.

+      UpdateSurfaceDamageExpectations();
+    }
   } else {
     // We are going idle, so reset expectations.
     child_surface_ids_to_expect_damage_from_.clear();
+    child_surface_ids_damaged_prev_prev_.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;
 
     begin_frame_source_->SetNeedsBeginFrames(false);
   }
 }
 
 void DisplayScheduler::OnBeginFrameDeadline() {
@@ skipping 10 matching lines @@
 }
 
 void DisplayScheduler::DidSwapBuffersComplete() {
   pending_swaps_--;
   TRACE_EVENT1("cc", "DisplayScheduler::DidSwapBuffersComplete",
                "pending_frames", pending_swaps_);
   ScheduleBeginFrameDeadline();
 }
 
 }  // namespace cc