Send overlay promotion hints only to displayed resources.

This change affects how overlays are chosen on Android.

Previously, DisplayResourceProvider would notify all resources that
requested a promotion hint, unless they were scheduled for deletion.
The intent was that only the resources that are used for the current
frame would receive hints.

However, that no longer works.  The DisplayResourceProvider was
sending hints to resources that weren't considered for overlay,
such as resources used by a previous CompositorFrame.  The result
was that the requestor would be told that the resource wasn't
promotable, and would try to switch away from SurfaceView.  In
reality, it just wasn't supposed to be on the screen.

Instead, we now explicitly construct a list of resource IDs that are
used by DrawQuads, and limit the hints to those.  We do this only
if any resource (displayed or not) is requesting promotion hints,
which should prevent the overlay processor from doing the extra
work except when the results actually will be used.

Bug: 900438
Change-Id: Iba56e256b08233c9c05bd299d0ecd4a556807ccd
Reviewed-on: https://chromium-review.googlesource.com/c/1310498
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604655}

components/viz/service/display/display_resource_provider.cc

@@ -168,12 +168,13 @@ bool DisplayResourceProvider::OnMemoryDump(
 
 #if defined(OS_ANDROID)
 void DisplayResourceProvider::SendPromotionHints(
-    const OverlayCandidateList::PromotionHintInfoMap& promotion_hints) {
+    const OverlayCandidateList::PromotionHintInfoMap& promotion_hints,
+    const ResourceIdSet& requestor_set) {
   GLES2Interface* gl = ContextGL();
   if (!gl)
     return;
 
-  for (const auto& id : wants_promotion_hints_set_) {
+  for (const auto& id : requestor_set) {
     auto it = resources_.find(id);
     if (it == resources_.end())
       continue;
@@ -210,15 +211,28 @@ bool DisplayResourceProvider::IsBackedBySurfaceTexture(ResourceId id) {
   return resource->transferable.is_backed_by_surface_texture;
 }
 
-bool DisplayResourceProvider::WantsPromotionHintForTesting(ResourceId id) {
-  return wants_promotion_hints_set_.count(id) > 0;
-}
-
 size_t DisplayResourceProvider::CountPromotionHintRequestsForTesting() {
   return wants_promotion_hints_set_.size();
 }
 #endif
 
+bool DisplayResourceProvider::DoesResourceWantPromotionHint(
+    ResourceId id) const {
+#if defined(OS_ANDROID)
+  return wants_promotion_hints_set_.count(id) > 0;
+#else
+  return false;
+#endif
+}
+
+bool DisplayResourceProvider::DoAnyResourcesWantPromotionHints() const {
+#if defined(OS_ANDROID)
+  return wants_promotion_hints_set_.size() > 0;
+#else
+  return false;
+#endif
+}
+
 bool DisplayResourceProvider::IsOverlayCandidate(ResourceId id) {
   ChildResource* resource = TryGetResource(id);
   // TODO(ericrk): We should never fail TryGetResource, but we appear to

components/viz/service/display/display_resource_provider.h

@@ -78,23 +78,32 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
 
 #if defined(OS_ANDROID)
   // Send an overlay promotion hint to all resources that requested it via
-  // |wants_promotion_hints_set_|.  |promotable_hints| contains all the
-  // resources that should be told that they're promotable.  Others will be told
-  // that they're not promotable right now.
+  // |requestor_set|.  |promotable_hints| contains all the resources that should
+  // be told that they're promotable.  Others will be told that they're not.
+  //
+  // We don't use |wants_promotion_hints_set_| in place of |requestor_set|,
+  // since we might have resources that aren't used for drawing.  Sending a hint
+  // for a resource that wasn't even considered for overlay would be misleading
+  // to the requestor; the resource might be overlayable except that nobody
+  // tried to do it.
   void SendPromotionHints(
-      const OverlayCandidateList::PromotionHintInfoMap& promotion_hints);
+      const OverlayCandidateList::PromotionHintInfoMap& promotion_hints,
+      const ResourceIdSet& requestor_set);
 
   // Indicates if this resource is backed by an Android SurfaceTexture, and thus
   // can't really be promoted to an overlay.
   bool IsBackedBySurfaceTexture(ResourceId id);
 
-  // Indicates if this resource wants to receive promotion hints.
-  bool WantsPromotionHintForTesting(ResourceId id);
-
   // Return the number of resources that request promotion hints.
   size_t CountPromotionHintRequestsForTesting();
 #endif
 
+  // Indicates if this resource wants to receive promotion hints.
+  bool DoesResourceWantPromotionHint(ResourceId id) const;
+
+  // Return true if and only if any resource wants a promotion hint.
+  bool DoAnyResourcesWantPromotionHints() const;
+
   bool IsResourceSoftwareBacked(ResourceId id);
   GLenum GetResourceTextureTarget(ResourceId id);
   // Return the format of the underlying buffer that can be used for scanout.

components/viz/service/display/display_resource_provider_unittest.cc

@@ -1372,12 +1372,14 @@ TEST_P(DisplayResourceProviderTest, OverlayPromotionHint) {
   // up in the request set before we wait, then the attempt to notify them wil;
   // DCHECK when we try to lock them for reading in SendPromotionHints.
   EXPECT_EQ(0u, resource_provider_->CountPromotionHintRequestsForTesting());
+  EXPECT_FALSE(resource_provider_->DoAnyResourcesWantPromotionHints());
   {
     resource_provider_->WaitSyncToken(mapped_id1);
     DisplayResourceProvider::ScopedReadLockGL lock(resource_provider_.get(),
                                                    mapped_id1);
   }
   EXPECT_EQ(1u, resource_provider_->CountPromotionHintRequestsForTesting());
+  EXPECT_TRUE(resource_provider_->DoAnyResourcesWantPromotionHints());
 
   EXPECT_EQ(list[0].mailbox_holder.sync_token, gl_->last_waited_sync_token());
   ResourceIdSet resource_ids_to_receive;
@@ -1394,11 +1396,11 @@ TEST_P(DisplayResourceProviderTest, OverlayPromotionHint) {
   // Make sure that the request for a promotion hint was noticed.
   EXPECT_TRUE(resource_provider_->IsOverlayCandidate(mapped_id1));
   EXPECT_TRUE(resource_provider_->IsBackedBySurfaceTexture(mapped_id1));
-  EXPECT_TRUE(resource_provider_->WantsPromotionHintForTesting(mapped_id1));
+  EXPECT_TRUE(resource_provider_->DoesResourceWantPromotionHint(mapped_id1));
 
   EXPECT_TRUE(resource_provider_->IsOverlayCandidate(mapped_id2));
   EXPECT_FALSE(resource_provider_->IsBackedBySurfaceTexture(mapped_id2));
-  EXPECT_FALSE(resource_provider_->WantsPromotionHintForTesting(mapped_id2));
+  EXPECT_FALSE(resource_provider_->DoesResourceWantPromotionHint(mapped_id2));
 
   // ResourceProvider maintains a set of promotion hint requests that should be
   // cleared when resources are deleted.
@@ -1407,6 +1409,7 @@ TEST_P(DisplayResourceProviderTest, OverlayPromotionHint) {
   child_resource_provider_->ReceiveReturnsFromParent(returned_to_child);
 
   EXPECT_EQ(0u, resource_provider_->CountPromotionHintRequestsForTesting());
+  EXPECT_FALSE(resource_provider_->DoAnyResourcesWantPromotionHints());
 
   resource_provider_->DestroyChild(child_id);
 

components/viz/service/display/overlay_candidate.cc

@@ -420,4 +420,15 @@ void OverlayCandidateList::AddPromotionHint(const OverlayCandidate& candidate) {
   promotion_hint_info_map_[candidate.resource_id] = candidate.display_rect;
 }
 
+void OverlayCandidateList::AddToPromotionHintRequestorSetIfNeeded(
+    const DisplayResourceProvider* resource_provider,
+    const DrawQuad* quad) {
+  if (quad->material != DrawQuad::STREAM_VIDEO_CONTENT)
+    return;
+  ResourceId id = StreamVideoDrawQuad::MaterialCast(quad)->resource_id();
+  if (!resource_provider->DoesResourceWantPromotionHint(id))
+    return;
+  promotion_hint_requestor_set_.insert(id);
+}
+
 }  // namespace viz

components/viz/service/display/overlay_candidate.h

@@ -145,8 +145,17 @@ class VIZ_SERVICE_EXPORT OverlayCandidateList
   // overlay, if one backs them with a SurfaceView.
   PromotionHintInfoMap promotion_hint_info_map_;
 
+  // Set of resources that have requested a promotion hint that also have quads
+  // that use them.
+  ResourceIdSet promotion_hint_requestor_set_;
+
   // Helper to insert |candidate| into |promotion_hint_info_|.
   void AddPromotionHint(const OverlayCandidate& candidate);
+
+  // Add |quad| to |promotion_hint_requestors_| if it is requesting a hint.
+  void AddToPromotionHintRequestorSetIfNeeded(
+      const DisplayResourceProvider* resource_provider,
+      const DrawQuad* quad);
 };
 
 }  // namespace viz

components/viz/service/display/overlay_processor.cc

@@ -32,7 +32,8 @@ class SendPromotionHintsBeforeReturning {
       : resource_provider_(resource_provider), candidates_(candidates) {}
   ~SendPromotionHintsBeforeReturning() {
     resource_provider_->SendPromotionHints(
-        candidates_->promotion_hint_info_map_);
+        candidates_->promotion_hint_info_map_,
+        candidates_->promotion_hint_requestor_set_);
   }
 
  private:

components/viz/service/display/overlay_strategy_underlay.cc

@@ -7,6 +7,7 @@
 #include "build/build_config.h"
 #include "components/viz/common/quads/draw_quad.h"
 #include "components/viz/common/quads/solid_color_draw_quad.h"
+#include "components/viz/service/display/display_resource_provider.h"
 #include "components/viz/service/display/overlay_candidate_validator.h"
 
 namespace viz {
@@ -28,8 +29,19 @@ bool OverlayStrategyUnderlay::Attempt(
     OverlayCandidateList* candidate_list,
     std::vector<gfx::Rect>* content_bounds) {
   QuadList& quad_list = render_pass->quad_list;
+  const bool compute_hints =
+      resource_provider->DoAnyResourcesWantPromotionHints();
+
   for (auto it = quad_list.begin(); it != quad_list.end(); ++it) {
     OverlayCandidate candidate;
+
+    // If we're computing the list of all resources that requested promotion
+    // hints, then add this candidate to the set if needed.
+    if (compute_hints) {
+      candidate_list->AddToPromotionHintRequestorSetIfNeeded(resource_provider,
+                                                             *it);
+    }
+
     if (!OverlayCandidate::FromDrawQuad(resource_provider, output_color_matrix,
                                         *it, &candidate) ||
         (opaque_mode_ == OpaqueMode::RequireOpaqueCandidates &&
@@ -76,6 +88,13 @@ bool OverlayStrategyUnderlay::Attempt(
       // |candidate| would have to fall back to a texture.
       candidate_list->promotion_hint_info_map_.clear();
       candidate_list->AddPromotionHint(candidate);
+      if (compute_hints) {
+        // Finish the quad list to find any other resources.
+        for (; it != quad_list.end(); ++it) {
+          candidate_list->AddToPromotionHintRequestorSetIfNeeded(
+              resource_provider, *it);
+        }
+      }
       return true;
     } else {
       // If |candidate| should get a promotion hint, then remember that now.