Dynamically update references when a child activates.

This CL implements an observer pattern for activated child surface,
where each surface will be able to observe activation events
happening in a subset of FrameSinkIds and update SurfaceReferences
accordingly. This is needed since we will no longer refer to the fallback
but instead will use GetLatestInFlightSurface to acquire references from
the SurfaceRanges specified in CompositorFrameMetaData.

Bug: 857575
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8ee1a9633273e116c5e7b1ae840dcc8db7a0407a
Reviewed-on: https://chromium-review.googlesource.com/1157404
Commit-Queue: Andre Kaba <akaba@google.com>
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582773}

cc/trees/layer_tree_host.h

@@ -688,7 +688,7 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
 
   scoped_refptr<HeadsUpDisplayLayer> hud_layer_;
 
-  // The number of SurfaceRanges that have (fallback,primary) set to
+  // The number of SurfaceLayers that have (fallback,primary) set to
   // viz::SurfaceRange.
   base::flat_map<viz::SurfaceRange, int> surface_ranges_;
 

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -122,7 +122,6 @@ void CompositorFrameSinkSupport::OnSurfaceActivated(Surface* surface) {
   }
 
   DCHECK(surface->HasActiveFrame());
-  surface->UpdateSurfaceReferences();
 
   // Check if this is a display root surface and the SurfaceId is changing.
   if (is_root_ && (!referenced_local_surface_id_ ||

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -137,6 +137,14 @@ class SurfaceSynchronizationTest : public testing::Test {
     supports_.erase(it);
   }
 
+  // Returns all the references where |surface_id| is the parent.
+  const base::flat_set<SurfaceId>& GetReferencesFrom(
+      const SurfaceId& surface_id) {
+    return frame_sink_manager()
+        .surface_manager()
+        ->GetSurfacesReferencedByParent(surface_id);
+  }
+
   FrameSinkManagerImpl& frame_sink_manager() { return frame_sink_manager_; }
 
   // Returns all the references where |surface_id| is the parent.
@@ -2697,6 +2705,56 @@ TEST_F(SurfaceSynchronizationTest, SetPreviousFrameSurfaceDoesntCrash) {
                                          MakeDefaultCompositorFrame());
 }
 
+// This test verifies that a parent referencing a SurfaceRange get updated
+// whenever a child surface activates inside this range. This should also update
+// the SurfaceReferences tree.
+TEST_F(SurfaceSynchronizationTest, SurfaceReferencesChangeOnChildActivation) {
+  SetFrameSinkHierarchy(kParentFrameSink, kChildFrameSink1);
+  const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+  const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 3);
+  const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink2, 1);
+
+  parent_support().SubmitCompositorFrame(
+      parent_id.local_surface_id(),
+      MakeCompositorFrame(
+          empty_surface_ids(), {SurfaceRange(child_id1, child_id4)},
+          std::vector<TransferableResource>(), MakeDefaultDeadline()));
+
+  // Verify that no references exist.
+  EXPECT_THAT(GetReferencesFrom(parent_id), empty_surface_ids());
+
+  // Activate |child_id1|.
+  child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that a reference is acquired.
+  EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id1));
+
+  // Activate |child_id2|.
+  child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the reference is updated.
+  EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id2));
+
+  // Activate |child_id4| in a different frame sink.
+  child_support2().SubmitCompositorFrame(child_id4.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the reference is updated.
+  EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id4));
+
+  // Activate |child_id3|.
+  child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the reference will not get updated since |child_id3| is in the
+  // fallback's FrameSinkId.
+  EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id4));
+}
+
 // This test verifies that once a frame sink become invalidated, it should
 // immediately unblock all pending frames depending on that sink.
 TEST_F(SurfaceSynchronizationTest,

components/viz/service/surfaces/surface.cc

@@ -98,29 +98,6 @@ void Surface::UnrefResources(const std::vector<ReturnedResource>& resources) {
     surface_client_->UnrefResources(resources);
 }
 
-void Surface::UpdateSurfaceReferences() {
-  const base::flat_set<SurfaceId>& existing_referenced_surfaces =
-      surface_manager_->GetSurfacesReferencedByParent(surface_id());
-  base::flat_set<SurfaceId> new_referenced_surfaces(
-      active_referenced_surfaces().begin(), active_referenced_surfaces().end(),
-      base::KEEP_FIRST_OF_DUPES);
-
-  // Populate list of surface references to add and remove by getting the
-  // difference between existing surface references and surface references for
-  // latest activated CompositorFrame.
-  std::vector<SurfaceReference> references_to_add;
-  std::vector<SurfaceReference> references_to_remove;
-  GetSurfaceReferenceDifference(surface_id(), existing_referenced_surfaces,
-                                new_referenced_surfaces, &references_to_add,
-                                &references_to_remove);
-
-  // Modify surface references stored in SurfaceManager.
-  if (!references_to_add.empty())
-    surface_manager_->AddSurfaceReferences(references_to_add);
-  if (!references_to_remove.empty())
-    surface_manager_->RemoveSurfaceReferences(references_to_remove);
-}
-
 void Surface::RejectCompositorFramesToFallbackSurfaces() {
   for (const SurfaceRange& surface_range :
        GetPendingFrame().metadata.referenced_surfaces) {
@@ -146,6 +123,65 @@ void Surface::RejectCompositorFramesToFallbackSurfaces() {
   }
 }
 
+void Surface::UpdateSurfaceReferences() {
+  const base::flat_set<SurfaceId>& existing_referenced_surfaces =
+      surface_manager_->GetSurfacesReferencedByParent(surface_id());
+
+  // Populate list of surface references to add and remove by getting the
+  // difference between existing surface references and surface references for
+  // latest activated CompositorFrame.
+  std::vector<SurfaceReference> references_to_add;
+  std::vector<SurfaceReference> references_to_remove;
+  GetSurfaceReferenceDifference(surface_id(), existing_referenced_surfaces,
+                                active_referenced_surfaces(),
+                                &references_to_add, &references_to_remove);
+
+  // Modify surface references stored in SurfaceManager.
+  if (!references_to_add.empty())
+    surface_manager_->AddSurfaceReferences(references_to_add);
+  if (!references_to_remove.empty())
+    surface_manager_->RemoveSurfaceReferences(references_to_remove);
+}
+
+void Surface::OnChildActivated(const SurfaceId& activated_id) {
+  DCHECK(HasActiveFrame());
+
+  for (size_t i = 0;
+       i < active_frame_data_->frame.metadata.referenced_surfaces.size(); i++) {
+    const SurfaceRange& surface_range =
+        active_frame_data_->frame.metadata.referenced_surfaces[i];
+    const SurfaceId& last_id = last_surface_id_for_range_[i];
+
+    // If |activated_id| is in fallback's FrameSinkId but we already reference a
+    // SurfaceId in the primary's FrameSinkId, we do nothing.
+    if (surface_range.HasDifferentFrameSinkIds() && last_id.is_valid() &&
+        last_id.frame_sink_id() == surface_range.end().frame_sink_id() &&
+        activated_id.frame_sink_id() ==
+            surface_range.start()->frame_sink_id()) {
+      continue;
+    }
+
+    if (surface_range.IsInRangeInclusive(activated_id)) {
+      // Remove the old reference.
+      if (last_id.is_valid()) {
+        auto old_it = active_referenced_surfaces_.find(last_id);
+        if (old_it != active_referenced_surfaces_.end())
+          active_referenced_surfaces_.erase(old_it);
+        surface_manager_->RemoveSurfaceReferences(
+            {SurfaceReference(surface_info_.id(), last_id)});
+      }
+
+      // Add a new reference.
+      active_referenced_surfaces_.insert(activated_id);
+      surface_manager_->AddSurfaceReferences(
+          {SurfaceReference(surface_info_.id(), activated_id)});
+
+      // Update the referenced surface for this range.
+      last_surface_id_for_range_[i] = activated_id;
+    }
+  }
+}
+
 void Surface::Close() {
   closed_ = true;
 }
@@ -306,6 +342,56 @@ void Surface::ActivatePendingFrame(base::Optional<base::TimeDelta> duration) {
   ActivateFrame(std::move(frame_data), duration);
 }
 
+void Surface::UpdateObservedSinks(
+    const base::flat_set<FrameSinkId>& new_observed_sinks) {
+  std::vector<FrameSinkId> sinks_to_remove;
+  std::vector<FrameSinkId> sinks_to_add;
+
+  for (const FrameSinkId& sink_id : new_observed_sinks)
+    if (observed_sinks_.count(sink_id) == 0)
+      sinks_to_add.push_back(sink_id);
+
+  for (const FrameSinkId& sink_id : observed_sinks_)
+    if (new_observed_sinks.count(sink_id) == 0)
+      sinks_to_remove.push_back(sink_id);
+
+  for (const FrameSinkId& sink_id : sinks_to_remove) {
+    observed_sinks_.erase(sink_id);
+    surface_manager_->RemoveActivationObserver(sink_id, surface_info_.id());
+  }
+
+  for (const FrameSinkId& sink_id : sinks_to_add) {
+    observed_sinks_.insert(sink_id);
+    surface_manager_->AddActivationObserver(sink_id, surface_info_.id());
+  }
+}
+
+void Surface::RecomputeActiveReferencedSurfaces() {
+  // Extract the latest in flight surface from the ranges in the frame then
+  // notify SurfaceManager of the new references.
+  active_referenced_surfaces_.clear();
+  last_surface_id_for_range_.clear();
+  base::flat_set<FrameSinkId> new_observed_sinks;
+  for (const SurfaceRange& surface_range :
+       active_frame_data_->frame.metadata.referenced_surfaces) {
+    // Observe frame sinks of both endpoints of the range.
+    new_observed_sinks.insert(surface_range.end().frame_sink_id());
+    if (surface_range.HasDifferentFrameSinkIds())
+      new_observed_sinks.insert(surface_range.start()->frame_sink_id());
+
+    Surface* surface =
+        surface_manager_->GetLatestInFlightSurface(surface_range);
+    if (surface) {
+      active_referenced_surfaces_.insert(surface->surface_id());
+      last_surface_id_for_range_.push_back(surface->surface_id());
+    } else {
+      last_surface_id_for_range_.push_back(SurfaceId());
+    }
+  }
+  UpdateObservedSinks(new_observed_sinks);
+  UpdateSurfaceReferences();
+}
+
 // A frame is activated if all its Surface ID dependences are active or a
 // deadline has hit and the frame was forcibly activated. |duration| is a
 // measure of the time the frame has spent waiting on dependencies to arrive.
@@ -331,12 +417,7 @@ void Surface::ActivateFrame(FrameData frame_data,
 
   active_frame_data_ = std::move(frame_data);
 
-  active_referenced_surfaces_.clear();
-  for (SurfaceRange surface_range :
-       active_frame_data_->frame.metadata.referenced_surfaces) {
-    if (surface_range.start())
-      active_referenced_surfaces_.emplace_back(*surface_range.start());
-  }
+  RecomputeActiveReferencedSurfaces();
 
   for (auto& copy_request : old_copy_requests)
     RequestCopyOfOutput(std::move(copy_request));

components/viz/service/surfaces/surface.h

@@ -121,11 +121,6 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
 
   bool needs_sync_tokens() const { return needs_sync_tokens_; }
 
-  // Updates surface references of the surface using the referenced
-  // surfaces from the most recent CompositorFrame.
-  // Modifies surface references stored in SurfaceManager.
-  void UpdateSurfaceReferences();
-
   // Returns false if |frame| is invalid.
   // |frame_rejected_callback| will be called once if the frame will not be
   // displayed.
@@ -182,7 +177,7 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   void NotifyAggregatedDamage(const gfx::Rect& damage_rect,
                               base::TimeTicks expected_display_time);
 
-  const std::vector<SurfaceId>& active_referenced_surfaces() const {
+  const base::flat_set<SurfaceId>& active_referenced_surfaces() const {
     return active_referenced_surfaces_;
   }
 
@@ -211,6 +206,10 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // Called when this surface will be included in the next display frame.
   void OnWillBeDrawn();
 
+  // Called when |surface_id| is activated for the first time and its part of a
+  // referenced SurfaceRange.
+  void OnChildActivated(const SurfaceId& surface_id);
+
  private:
   struct SequenceNumbers {
     uint32_t parent_sequence_number = 0u;
@@ -238,10 +237,25 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // children to catch up to the parent.
   void RejectCompositorFramesToFallbackSurfaces();
 
+  // Updates surface references of the surface using the referenced
+  // surfaces from the most recent CompositorFrame.
+  // Modifies surface references stored in SurfaceManager.
+  void UpdateSurfaceReferences();
+
   // Called to prevent additional CompositorFrames from being accepted into this
   // surface. Once a Surface is closed, it cannot accept CompositorFrames again.
   void Close();
 
+  // Updates the FrameSinkIds observed by this surface to be equal to
+  // |new_observed_sinks|.
+  void UpdateObservedSinks(
+      const base::flat_set<FrameSinkId>& new_observed_sinks);
+
+  // Recomputes active references for this surface when it activates. This
+  // method will also update the observed sinks based on the referenced ranges
+  // in the submitted compositor frame.
+  void RecomputeActiveReferencedSurfaces();
+
   void ActivatePendingFrame(base::Optional<base::TimeDelta> duration);
 
   // Called when all of the surface's dependencies have been resolved.
@@ -291,7 +305,20 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // passes the local_id in the map, then this surface is no longer interested
   // in observing activations for that FrameSinkId.
   base::flat_map<FrameSinkId, SequenceNumbers> frame_sink_id_dependencies_;
-  std::vector<SurfaceId> active_referenced_surfaces_;
+
+  // A set of all valid SurfaceIds contained |last_surface_id_for_range_| to
+  // avoid recompution.
+  base::flat_set<SurfaceId> active_referenced_surfaces_;
+
+  // Keeps track of the referenced surface for each SurfaceRange. i.e the i-th
+  // element is the referenced SurfaceId in the i-th SurfaceRange. If a
+  // SurfaceRange doesn't contain any active surfaces then the corresponding
+  // entry in this vector is an unvalid SurfaceId.
+  std::vector<SurfaceId> last_surface_id_for_range_;
+
+  // Frame sinks that this surface observe for activation events.
+  base::flat_set<FrameSinkId> observed_sinks_;
+
   DISALLOW_COPY_AND_ASSIGN(Surface);
 };
 

components/viz/service/surfaces/surface_manager.cc

@@ -568,10 +568,35 @@ bool SurfaceManager::SurfaceModified(const SurfaceId& surface_id,
 void SurfaceManager::FirstSurfaceActivation(const SurfaceInfo& surface_info) {
   CHECK(thread_checker_.CalledOnValidThread());
 
+  // Notify every Surface interested in knowing about activation events in
+  // |surface_info.surface_id()|'s frame sink.
+  for (const SurfaceId& sink_observer :
+       activation_observers_[surface_info.id().frame_sink_id()]) {
+    Surface* observer_surface = GetSurfaceForId(sink_observer);
+    if (observer_surface)
+      observer_surface->OnChildActivated(surface_info.id());
+  }
+
   for (auto& observer : observer_list_)
     observer.OnFirstSurfaceActivation(surface_info);
 }
 
+void SurfaceManager::AddActivationObserver(const FrameSinkId& sink_id,
+                                           const SurfaceId& surface_id) {
+  activation_observers_[sink_id].insert(surface_id);
+}
+
+void SurfaceManager::RemoveActivationObserver(const FrameSinkId& sink_id,
+                                              const SurfaceId& surface_id) {
+  if (activation_observers_.count(sink_id) == 0)
+    return;
+
+  base::flat_set<SurfaceId>& observers = activation_observers_[sink_id];
+  observers.erase(surface_id);
+  if (observers.empty())
+    activation_observers_.erase(sink_id);
+}
+
 void SurfaceManager::SurfaceActivated(
     Surface* surface,
     base::Optional<base::TimeDelta> duration) {

components/viz/service/surfaces/surface_manager.h

@@ -108,6 +108,14 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   // Called when a surface has an active frame for the first time.
   void FirstSurfaceActivation(const SurfaceInfo& surface_info);
 
+  // Add |surface_id| as an observer for |sink_id|.
+  void AddActivationObserver(const FrameSinkId& sink_id,
+                             const SurfaceId& surface_id);
+
+  // Remove |surface_id| from the observers of |sink_id|.
+  void RemoveActivationObserver(const FrameSinkId& sink_id,
+                                const SurfaceId& surface_id);
+
   // Called when a CompositorFrame within |surface| has activated. |duration| is
   // a measure of the time the frame has spent waiting on dependencies to
   // arrive. If |duration| is base::nullopt, then that indicates that this frame
@@ -324,6 +332,10 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   base::flat_map<FrameSinkId, base::flat_set<LocalSurfaceId>>
       persistent_references_by_frame_sink_id_;
 
+  // A map storing SurfaceIds interested in knowing about activation events
+  // happending in FrameSinkId.
+  base::flat_map<FrameSinkId, base::flat_set<SurfaceId>> activation_observers_;
+
   // Timer to remove old temporary references that aren't removed after an
   // interval of time. The timer will started/stopped so it only runs if there
   // are temporary references. Also the timer isn't used with Android WebView.