Route HitTestRegionList through FrameSinkManager

This change routes HitTestRegionList objects received by
CompositorFrameSinkSupport to the HitTestManager.

The HitTestAggregator will query the HitTestManager for the
active HitTestRegionLists when aggregating the data.

HitTestManager also manages the lifetime of HitTestRegionList
objects by deleting old objects when surfaces are discarded
and deleting objects that correspond to older frames when
a new surface is activated.

This change also modifies HitTestAggregator memory management
so that new memory is allocated as needed while aggregating
HitTestRegionList data.

Bug: 732399
Change-Id: Iaedec31a34f04446a539acd10d77c6fe18034d44
Reviewed-on: https://chromium-review.googlesource.com/650220
Commit-Queue: Gary Klassen <gklassen@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506780}

components/viz/service/BUILD.gn

@@ -110,6 +110,8 @@ viz_component("service") {
     "hit_test/hit_test_aggregator.cc",
     "hit_test/hit_test_aggregator.h",
     "hit_test/hit_test_aggregator_delegate.h",
+    "hit_test/hit_test_manager.cc",
+    "hit_test/hit_test_manager.h",
     "surfaces/direct_surface_reference_factory.cc",
     "surfaces/direct_surface_reference_factory.h",
     "surfaces/surface.cc",

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -172,9 +172,6 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
     }
   }
 
-  frame_sink_manager()->SubmitHitTestRegionList(
-      current_surface_id_, frame_index, std::move(hit_test_region_list));
-
   Surface* prev_surface =
       surface_manager_->GetSurfaceForId(current_surface_id_);
   Surface* current_surface = nullptr;
@@ -219,6 +216,9 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
     surface_manager_->DestroySurface(prev_surface->surface_id());
   }
 
+  frame_sink_manager()->SubmitHitTestRegionList(
+      current_surface_id_, frame_index, std::move(hit_test_region_list));
+
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
 

components/viz/service/frame_sinks/frame_sink_manager_impl.cc

@@ -35,7 +35,9 @@ FrameSinkManagerImpl::FrameSinkManagerImpl(
     DisplayProvider* display_provider)
     : display_provider_(display_provider),
       surface_manager_(lifetime_type),
+      hit_test_manager_(this),
       binding_(this) {
+  surface_manager_.AddObserver(&hit_test_manager_);
   surface_manager_.AddObserver(this);
 }
 
@@ -47,6 +49,7 @@ FrameSinkManagerImpl::~FrameSinkManagerImpl() {
   DCHECK_EQ(clients_.size(), 0u);
   DCHECK_EQ(registered_sources_.size(), 0u);
   surface_manager_.RemoveObserver(this);
+  surface_manager_.RemoveObserver(&hit_test_manager_);
 }
 
 void FrameSinkManagerImpl::BindAndSetClient(
@@ -368,8 +371,13 @@ void FrameSinkManagerImpl::SubmitHitTestRegionList(
     const SurfaceId& surface_id,
     uint64_t frame_index,
     mojom::HitTestRegionListPtr hit_test_region_list) {
-  // TODO(gklassen): Route hit_test_region_list to appropriate
-  // matching RootCompositorFrameSink
+  hit_test_manager_.SubmitHitTestRegionList(surface_id, frame_index,
+                                            std::move(hit_test_region_list));
+}
+
+uint64_t FrameSinkManagerImpl::GetActiveFrameIndex(
+    const SurfaceId& surface_id) {
+  return surface_manager_.GetSurfaceForId(surface_id)->GetActiveFrameIndex();
 }
 
 void FrameSinkManagerImpl::OnAggregatedHitTestRegionListUpdated(

components/viz/service/frame_sinks/frame_sink_manager_impl.h

@@ -16,6 +16,7 @@
 #include "base/threading/thread_checker.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "components/viz/service/frame_sinks/primary_begin_frame_source.h"
+#include "components/viz/service/hit_test/hit_test_manager.h"
 #include "components/viz/service/surfaces/surface_manager.h"
 #include "components/viz/service/surfaces/surface_observer.h"
 #include "components/viz/service/viz_service_export.h"
@@ -112,6 +113,8 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl : public SurfaceObserver,
 
   SurfaceManager* surface_manager() { return &surface_manager_; }
 
+  const HitTestManager* hit_test_manager() { return &hit_test_manager_; }
+
   // SurfaceObserver implementation.
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;
   void OnSurfaceActivated(const SurfaceId& surface_id) override;
@@ -146,6 +149,9 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl : public SurfaceObserver,
       uint64_t frame_index,
       mojom::HitTestRegionListPtr hit_test_region_list);
 
+  // This method is virtual so the implementation can be modified in unit tests.
+  virtual uint64_t GetActiveFrameIndex(const SurfaceId& surface_id);
+
  private:
   friend class cc::test::SurfaceSynchronizationTest;
 
@@ -191,6 +197,8 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl : public SurfaceObserver,
   // surfaces are destroyed before |primary_source_|.
   SurfaceManager surface_manager_;
 
+  HitTestManager hit_test_manager_;
+
   std::unordered_map<FrameSinkId,
                      std::unique_ptr<mojom::CompositorFrameSink>,
                      FrameSinkIdHash>

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -31,7 +31,7 @@ RootCompositorFrameSinkImpl::RootCompositorFrameSinkImpl(
           true /* needs_sync_points */)),
       display_begin_frame_source_(std::move(begin_frame_source)),
       display_(std::move(display)),
-      hit_test_aggregator_(this) {
+      hit_test_aggregator_(frame_sink_manager->hit_test_manager(), this) {
   DCHECK(display_begin_frame_source_);
   DCHECK(display_);
 

components/viz/service/hit_test/hit_test_aggregator.h

@@ -7,6 +7,7 @@
 
 #include "components/viz/common/hit_test/aggregated_hit_test_region.h"
 #include "components/viz/common/surfaces/surface_id.h"
+#include "components/viz/service/hit_test/hit_test_manager.h"
 #include "components/viz/service/surfaces/surface_observer.h"
 #include "components/viz/service/viz_service_export.h"
 #include "services/viz/public/interfaces/hit_test/hit_test_region_list.mojom.h"
@@ -14,25 +15,21 @@
 namespace viz {
 class HitTestAggregatorDelegate;
 
-// HitTestAggregator collects HitTestRegionList objects from surfaces and
-// aggregates them into a DisplayHitTesData structue made available in
-// shared memory to enable efficient hit testing across processes.
+// HitTestAggregator assembles the list of HitTestRegion objects that define the
+// hit test information required for one display. Active HitTestRegionList
+// information is obtained from the HitTestManager.  The resulting list is made
+// available in shared memory and used by HitTestQuery to enable efficient hit
+// testing across processes.
 //
 // This is intended to be created in the viz or GPU process. For mus+ash this
 // will be true after the mus process split.
-class VIZ_SERVICE_EXPORT HitTestAggregator : public SurfaceObserver {
+class VIZ_SERVICE_EXPORT HitTestAggregator {
  public:
   // |delegate| owns and outlives HitTestAggregator.
-  explicit HitTestAggregator(HitTestAggregatorDelegate* delegate);
+  HitTestAggregator(const HitTestManager* hit_test_manager,
+                    HitTestAggregatorDelegate* delegate);
   ~HitTestAggregator();
 
-  // Called when HitTestRegionList is submitted along with every call
-  // to SubmitCompositorFrame.  This is collected in pending_ until
-  // surfaces are aggregated and put on the display.
-  void SubmitHitTestRegionList(
-      const SurfaceId& surface_id,
-      mojom::HitTestRegionListPtr hit_test_region_list);
-
   // Performs the work of Aggregate by creating a PostTask so that
   // the work is not directly on the call.
   void PostTaskAggregate(const SurfaceId& display_surface_id);
@@ -47,34 +44,10 @@ class VIZ_SERVICE_EXPORT HitTestAggregator : public SurfaceObserver {
   // delegate.
   void Swap();
 
- protected:
-  // SurfaceObserver:
-  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
-  void OnSurfaceActivated(const SurfaceId& surface_id) override {}
-  void OnSurfaceDestroyed(const SurfaceId& surface_id) override {}
-  bool OnSurfaceDamaged(const SurfaceId& surface_id,
-                        const BeginFrameAck& ack) override;
-  void OnSurfaceDiscarded(const SurfaceId& surface_id) override;
-  void OnSurfaceDamageExpected(const SurfaceId& surface_id,
-                               const BeginFrameArgs& args) override {}
-
-  // Called when a surface has been aggregated and added to the
-  // display frame.  HitTestRegionList objects are held but ignored until
-  // this happens.  HitTestRegionList for the surface is copied from |pending_|
-  // to |active_| in this method.
-  void OnSurfaceWillDraw(const SurfaceId& surface_id) override;
-
-  // The collection of received HitTestRegionList objects that have not yet
-  // been added to the DisplayFrame (OnSurfaceWillDraw has not been called).
-  std::map<SurfaceId, mojom::HitTestRegionListPtr> pending_;
-
-  // The collection of HitTestRegionList objects that have been added to the
-  // DisplayFrame (OnSurfaceWillDraw has been called).
-  std::map<SurfaceId, mojom::HitTestRegionListPtr> active_;
-
-  // Keeps track of the number of regions in the active list
-  // so that we know when we exceed the available length.
-  uint32_t active_region_count_ = 0;
+ private:
+  friend class TestHitTestAggregator;
+
+  const HitTestManager* const hit_test_manager_;
 
   mojo::ScopedSharedBufferHandle read_handle_;
   mojo::ScopedSharedBufferHandle write_handle_;
@@ -93,22 +66,37 @@ class VIZ_SERVICE_EXPORT HitTestAggregator : public SurfaceObserver {
 
   HitTestAggregatorDelegate* const delegate_;
 
- private:
   // Allocates memory for the AggregatedHitTestRegion array.
   void AllocateHitTestRegionArray();
-  void AllocateHitTestRegionArray(uint32_t length);
 
+  // Resizes memory for the AggregatedHitTestRegion array. |size| indicates the
+  // number of elements.
+  void ResizeHitTestRegionArray(uint32_t size);
+
+  void GrowRegionList();
   void SwapHandles();
 
   // Appends the root element to the AggregatedHitTestRegion array.
   void AppendRoot(const SurfaceId& surface_id);
 
-  // Appends a region to the HitTestRegionList structure to recursively
-  // build the tree.
-  size_t AppendRegion(AggregatedHitTestRegion* regions,
-                      size_t region_index,
+  // Appends a |region| to the HitTestRegionList structure to recursively
+  // build the tree.  |region_index| indicates the current index of the end of
+  // the list.
+  size_t AppendRegion(size_t region_index,
                       const mojom::HitTestRegionPtr& region);
 
+  // Populates the HitTestRegion element at the given element |index|.
+  // Access to the HitTestRegion list is localized to this call
+  // in order to prevent errors if the array is resized during aggregation.
+  void SetRegionAt(size_t index,
+                   const FrameSinkId& frame_sink_id,
+                   uint32_t flags,
+                   const gfx::Rect& rect,
+                   const gfx::Transform& transform,
+                   int32_t child_count);
+  // Marks the element at the given index as the end of list.
+  void MarkEndAt(size_t index);
+
   // Handles the case when this object is deleted after
   // the PostTaskAggregation call is scheduled but before invocation.
   base::WeakPtrFactory<HitTestAggregator> weak_ptr_factory_;

components/viz/service/hit_test/hit_test_manager.cc

+// Copyright 2017 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 "components/viz/service/hit_test/hit_test_aggregator.h"
+
+#include "components/viz/common/hit_test/aggregated_hit_test_region.h"
+#include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
+#include "components/viz/service/hit_test/hit_test_aggregator_delegate.h"
+
+namespace viz {
+
+namespace {
+// TODO(gklassen): Review and select appropriate sizes based on
+// telemetry / UMA.
+constexpr uint32_t kMaxRegionsPerSurface = 1024;
+}  // namespace
+
+HitTestManager::HitTestManager(FrameSinkManagerImpl* frame_sink_manager)
+    : frame_sink_manager_(frame_sink_manager) {}
+
+HitTestManager::~HitTestManager() = default;
+
+void HitTestManager::SubmitHitTestRegionList(
+    const SurfaceId& surface_id,
+    const uint64_t frame_index,
+    mojom::HitTestRegionListPtr hit_test_region_list) {
+  if (!ValidateHitTestRegionList(surface_id, hit_test_region_list))
+    return;
+  // TODO(gklassen): Runtime validation that hit_test_region_list is valid.
+  // TODO(gklassen): Inform FrameSink that the hit_test_region_list is invalid.
+  // TODO(gklassen): FrameSink needs to inform the host of a difficult renderer.
+  hit_test_region_lists_[surface_id][frame_index] =
+      std::move(hit_test_region_list);
+}
+
+bool HitTestManager::ValidateHitTestRegionList(
+    const SurfaceId& surface_id,
+    const mojom::HitTestRegionListPtr& hit_test_region_list) {
+  if (!hit_test_region_list)
+    return false;
+  if (hit_test_region_list->regions.size() > kMaxRegionsPerSurface)
+    return false;
+  for (auto& region : hit_test_region_list->regions) {
+    if (!ValidateHitTestRegion(surface_id, region))
+      return false;
+  }
+  return true;
+}
+
+bool HitTestManager::ValidateHitTestRegion(
+    const SurfaceId& surface_id,
+    const mojom::HitTestRegionPtr& hit_test_region) {
+  // If client_id is 0 then use the client_id that
+  // matches the compositor frame.
+  if (hit_test_region->frame_sink_id.client_id() == 0) {
+    hit_test_region->frame_sink_id =
+        FrameSinkId(surface_id.frame_sink_id().client_id(),
+                    hit_test_region->frame_sink_id.sink_id());
+  }
+  // TODO(gklassen): Verify that this client is allowed to submit hit test
+  // data for the region associated with this |frame_sink_id|.
+  // TODO(gklassen): Ensure that |region->frame_sink_id| is a child of
+  // |frame_sink_id|.
+  if (hit_test_region->flags & mojom::kHitTestChildSurface) {
+    if (!hit_test_region->local_surface_id.has_value() ||
+        !hit_test_region->local_surface_id->is_valid()) {
+      return false;
+    }
+  }
+  return true;
+}
+
+bool HitTestManager::OnSurfaceDamaged(const SurfaceId& surface_id,
+                                      const BeginFrameAck& ack) {
+  return false;
+}
+
+void HitTestManager::OnSurfaceDiscarded(const SurfaceId& surface_id) {
+  hit_test_region_lists_.erase(surface_id);
+}
+
+void HitTestManager::OnSurfaceActivated(const SurfaceId& surface_id) {
+  // When a Surface is activated we can confidently remove all
+  // associated HitTestRegionList objects with an older frame_index.
+  auto search = hit_test_region_lists_.find(surface_id);
+  if (search == hit_test_region_lists_.end())
+    return;
+
+  uint64_t frame_index = frame_sink_manager_->GetActiveFrameIndex(surface_id);
+
+  auto& frame_index_map = search->second;
+  for (auto it = frame_index_map.begin(); it != frame_index_map.end();) {
+    if (it->first != frame_index)
+      it = frame_index_map.erase(it);
+    else
+      ++it;
+  }
+}
+
+const mojom::HitTestRegionList* HitTestManager::GetActiveHitTestRegionList(
+    const SurfaceId& surface_id) const {
+  auto search = hit_test_region_lists_.find(surface_id);
+  if (search == hit_test_region_lists_.end())
+    return nullptr;
+
+  uint64_t frame_index = frame_sink_manager_->GetActiveFrameIndex(surface_id);
+
+  auto& frame_index_map = search->second;
+  auto search2 = frame_index_map.find(frame_index);
+  if (search2 == frame_index_map.end())
+    return nullptr;
+
+  return search2->second.get();
+}
+
+}  // namespace viz

components/viz/service/hit_test/hit_test_manager.h

+// Copyright 2017 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.
+
+#ifndef COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_MANAGER_H_
+#define COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_MANAGER_H_
+
+#include "components/viz/common/hit_test/aggregated_hit_test_region.h"
+#include "components/viz/common/surfaces/surface_id.h"
+#include "components/viz/service/surfaces/surface_manager.h"
+#include "components/viz/service/surfaces/surface_observer.h"
+#include "components/viz/service/viz_service_export.h"
+#include "services/viz/public/interfaces/hit_test/hit_test_region_list.mojom.h"
+
+namespace viz {
+
+class FrameSinkManagerImpl;
+
+// HitTestManager manages the collection of HitTestRegionList objects
+// submitted in calls to SubmitCompositorFrame.  This collection is
+// used by HitTestAggregator.
+class VIZ_SERVICE_EXPORT HitTestManager : public SurfaceObserver {
+ public:
+  explicit HitTestManager(FrameSinkManagerImpl* frame_sink_manager);
+  virtual ~HitTestManager();
+
+  // Called when HitTestRegionList is submitted along with every call
+  // to SubmitCompositorFrame.
+  void SubmitHitTestRegionList(
+      const SurfaceId& surface_id,
+      const uint64_t frame_index,
+      mojom::HitTestRegionListPtr hit_test_region_list);
+
+  // Returns the HitTestRegionList corresponding to the given
+  // surface_id and the active CompositorFrame matched by frame_index.
+  const mojom::HitTestRegionList* GetActiveHitTestRegionList(
+      const SurfaceId& surface_id) const;
+
+  // SurfaceObserver:
+  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
+  void OnSurfaceActivated(const SurfaceId& surface_id) override;
+  void OnSurfaceDestroyed(const SurfaceId& surface_id) override {}
+  bool OnSurfaceDamaged(const SurfaceId& surface_id,
+                        const BeginFrameAck& ack) override;
+  void OnSurfaceDiscarded(const SurfaceId& surface_id) override;
+  void OnSurfaceDamageExpected(const SurfaceId& surface_id,
+                               const BeginFrameArgs& args) override {}
+  void OnSurfaceWillDraw(const SurfaceId& surface_id) override {}
+
+ private:
+  friend class TestHitTestManager;
+
+  bool ValidateHitTestRegionList(
+      const SurfaceId& surface_id,
+      const mojom::HitTestRegionListPtr& hit_test_region_list);
+  bool ValidateHitTestRegion(const SurfaceId& surface_id,
+                             const mojom::HitTestRegionPtr& hit_test_region);
+
+  FrameSinkManagerImpl* const frame_sink_manager_;
+
+  std::map<SurfaceId, base::flat_map<uint64_t, mojom::HitTestRegionListPtr>>
+      hit_test_region_lists_;
+
+  DISALLOW_COPY_AND_ASSIGN(HitTestManager);
+};
+
+}  // namespace viz
+
+#endif  // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_MANAGER_H_