Surface synchronization: Auto-Resize on Impl thread

This CL removes the ChildLocalSurfaceIdAllocator on the Blink
main thread. Instead, RenderWidget::DidAutoResize requests a new
LocalSurfaceId from the impl thread and continues to use the
current LocalSurfaceId provided by the parent.

A test was removed that was intended for the pre-child-allocation
code path and another was simplified to verify that DidAutoResize
requests a new LocalSurfaceId. A separate unit test verifies that
the request goes from LayerTreeHost to LayerTreeFrameSink.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8e11ac681aee1c5ea7fa6406d3c6cfd806b79986
Reviewed-on: https://chromium-review.googlesource.com/1051868Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557669}

cc/trees/layer_tree_host.cc

@@ -1081,6 +1081,7 @@ void LayerTreeHost::SetViewportSizeAndScale(
     // TODO(ccameron): This check is not valid on Aura or Mus yet, but should
     // be.
     CHECK(!has_pushed_local_surface_id_from_parent_ ||
+          new_local_surface_id_request_ ||
           !local_surface_id_from_parent_.is_valid());
 #endif
   }
@@ -1189,7 +1190,11 @@ void LayerTreeHost::SetLocalSurfaceIdFromParent(
 }
 
 void LayerTreeHost::RequestNewLocalSurfaceId() {
-  DCHECK(local_surface_id_from_parent_.is_valid());
+  // If surface synchronization is enabled, then we can still request a new
+  // viz::LocalSurfaceId but that request will be deferred until we have a valid
+  // viz::LocalSurfaceId from the parent.
+  DCHECK(settings_.enable_surface_synchronization ||
+         local_surface_id_from_parent_.is_valid());
   if (new_local_surface_id_request_)
     return;
   new_local_surface_id_request_ = true;

cc/trees/layer_tree_host.h

@@ -350,6 +350,9 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
   // Returns the current state of the new LocalSurfaceId request and resets
   // the state.
   bool TakeNewLocalSurfaceIdRequest();
+  bool new_local_surface_id_request_for_testing() const {
+    return new_local_surface_id_request_;
+  }
 
   void SetRasterColorSpace(const gfx::ColorSpace& raster_color_space);
   const gfx::ColorSpace& raster_color_space() const {

components/viz/common/surfaces/child_local_surface_id_allocator.cc

@@ -17,19 +17,10 @@ ChildLocalSurfaceIdAllocator::ChildLocalSurfaceIdAllocator()
 
 const LocalSurfaceId& ChildLocalSurfaceIdAllocator::UpdateFromParent(
     const LocalSurfaceId& parent_allocated_local_surface_id) {
-  // TODO(fsamuel): How a parent allocated LocalSurfaceId is incorporated into
-  // a ChildLocalSurfaceIdAllocator has been loosened. The parent should not
-  // be touching the child_sequence_number but currently there are cases where
-  // there are two ChildLocalSurfaceIdAllocators operating on a LocalSurfaceId.
-  // When that case is eliminated, we can avoid updating the child sequence
-  // number here.
-  if (parent_allocated_local_surface_id.parent_sequence_number() >=
+  if (parent_allocated_local_surface_id.parent_sequence_number() >
       current_local_surface_id_.parent_sequence_number()) {
     current_local_surface_id_.parent_sequence_number_ =
         parent_allocated_local_surface_id.parent_sequence_number_;
-    current_local_surface_id_.child_sequence_number_ =
-        std::max(current_local_surface_id_.child_sequence_number_,
-                 parent_allocated_local_surface_id.child_sequence_number_);
     current_local_surface_id_.embed_token_ =
         parent_allocated_local_surface_id.embed_token_;
   }

components/viz/common/surfaces/child_local_surface_id_allocator_unittest.cc

@@ -99,10 +99,7 @@ TEST(ChildLocalSurfaceIdAllocatorTest,
       parent_updated_child_allocator.GetCurrentLocalSurfaceId();
   EXPECT_EQ(postupdate_local_surface_id.parent_sequence_number(),
             parent_allocated_local_surface_id.parent_sequence_number());
-  // TODO(fsamuel): We shouldn't update the child sequence number but in order
-  // to allow a transition from LocalSurfaceId allocation on the main thread to
-  // the compositor, we will briefly support this.
-  EXPECT_EQ(postupdate_local_surface_id.child_sequence_number(),
+  EXPECT_NE(postupdate_local_surface_id.child_sequence_number(),
             parent_allocated_local_surface_id.child_sequence_number());
   EXPECT_EQ(postupdate_local_surface_id.embed_token(),
             parent_allocated_local_surface_id.embed_token());

content/browser/renderer_host/browser_compositor_view_mac.mm

@@ -353,6 +353,7 @@ void BrowserCompositorMac::UpdateForAutoResize(const gfx::Size& new_size_dip) {
   GetDelegatedFrameHost()->SynchronizeVisualProperties(
       dfh_local_surface_id_allocator_.GetCurrentLocalSurfaceId(), dfh_size_dip_,
       cc::DeadlinePolicy::UseExistingDeadline());
+  client_->SynchronizeVisualProperties();
 }
 
 void BrowserCompositorMac::UpdateVSyncParameters(

content/renderer/gpu/render_widget_compositor.cc

@@ -781,6 +781,14 @@ void RenderWidgetCompositor::SetViewportSizeAndScale(
       device_viewport_size, device_scale_factor, local_surface_id);
 }
 
+void RenderWidgetCompositor::RequestNewLocalSurfaceId() {
+  layer_tree_host_->RequestNewLocalSurfaceId();
+}
+
+bool RenderWidgetCompositor::HasNewLocalSurfaceIdRequest() const {
+  return layer_tree_host_->new_local_surface_id_request_for_testing();
+}
+
 void RenderWidgetCompositor::SetViewportVisibleRect(
     const gfx::Rect& visible_rect) {
   layer_tree_host_->SetViewportVisibleRect(visible_rect);

content/renderer/gpu/render_widget_compositor.h

@@ -124,6 +124,8 @@ class CONTENT_EXPORT RenderWidgetCompositor
   void SetViewportSizeAndScale(const gfx::Size& device_viewport_size,
                                float device_scale_factor,
                                const viz::LocalSurfaceId& local_surface_id);
+  void RequestNewLocalSurfaceId();
+  bool HasNewLocalSurfaceIdRequest() const;
   void SetViewportVisibleRect(const gfx::Rect& visible_rect);
   void SetURLForUkm(const GURL& url);
 

content/renderer/render_view_browsertest.cc

@@ -452,6 +452,8 @@ class RenderViewImplScaleFactorTest : public RenderViewImplBlinkSettingsTest {
         view()->min_size_for_auto_resize();
     visual_properties.max_size_for_auto_resize =
         view()->max_size_for_auto_resize();
+    visual_properties.local_surface_id = base::Optional<viz::LocalSurfaceId>(
+        viz::LocalSurfaceId(1, 1, base::UnguessableToken::Create()));
     visual_properties.content_source_id = view()->GetContentSourceId();
     view()->OnSynchronizeVisualProperties(visual_properties);
     ASSERT_EQ(dsf, view()->GetWebScreenInfo().device_scale_factor);

content/renderer/render_widget.cc

@@ -1238,16 +1238,6 @@ gfx::Size RenderWidget::GetSizeForWebWidget() const {
 }
 
 void RenderWidget::SynchronizeVisualProperties(const VisualProperties& params) {
-  viz::LocalSurfaceId new_local_surface_id;
-  // If the content_source_id of VisualProperties doesn't match
-  // |current_content_source_id_|, then the given LocalSurfaceId was generated
-  // before the navigation. Continue with the resize but don't use the
-  // LocalSurfaceId until the right one comes.
-  if (params.local_surface_id) {
-    new_local_surface_id = child_local_surface_id_allocator_.UpdateFromParent(
-        *params.local_surface_id);
-  }
-
   // The content_source_id that the browser sends us should never be larger than
   // |current_content_source_id_|.
   DCHECK_GE(1u << 30, current_content_source_id_ - params.content_source_id);
@@ -1267,9 +1257,9 @@ void RenderWidget::SynchronizeVisualProperties(const VisualProperties& params) {
           ? gfx::ScaleToCeiledSize(size_,
                                    params.screen_info.device_scale_factor)
           : params.compositor_viewport_pixel_size;
-  UpdateSurfaceAndScreenInfo(new_local_surface_id,
-                             new_compositor_viewport_pixel_size,
-                             params.screen_info);
+  UpdateSurfaceAndScreenInfo(
+      params.local_surface_id.value_or(viz::LocalSurfaceId()),
+      new_compositor_viewport_pixel_size, params.screen_info);
   UpdateCaptureSequenceNumber(params.capture_sequence_number);
   if (compositor_) {
     compositor_->SetBrowserControlsHeight(
@@ -1746,7 +1736,7 @@ void RenderWidget::OnImeFinishComposingText(bool keep_selection) {
 }
 
 void RenderWidget::UpdateSurfaceAndScreenInfo(
-    viz::LocalSurfaceId new_local_surface_id,
+    const viz::LocalSurfaceId& new_local_surface_id,
     const gfx::Size& new_compositor_viewport_pixel_size,
     const ScreenInfo& new_screen_info) {
   bool orientation_changed =
@@ -2174,11 +2164,12 @@ void RenderWidget::DidAutoResize(const gfx::Size& new_size) {
     // |size_| from |compositor_viewport_pixel_size_|. Also note that the
     // calculation of |new_compositor_viewport_pixel_size| does not appear to
     // take into account device emulation.
+    if (compositor_)
+      compositor_->RequestNewLocalSurfaceId();
     gfx::Size new_compositor_viewport_pixel_size =
         gfx::ScaleToCeiledSize(size_, GetWebScreenInfo().device_scale_factor);
-    UpdateSurfaceAndScreenInfo(child_local_surface_id_allocator_.GenerateId(),
-                               new_compositor_viewport_pixel_size,
-                               screen_info_);
+    UpdateSurfaceAndScreenInfo(
+        local_surface_id_, new_compositor_viewport_pixel_size, screen_info_);
   }
 }
 

content/renderer/render_widget.h

@@ -26,7 +26,6 @@
 #include "build/build_config.h"
 #include "cc/input/overscroll_behavior.h"
 #include "cc/input/touch_action.h"
-#include "components/viz/common/surfaces/child_local_surface_id_allocator.h"
 #include "components/viz/common/surfaces/local_surface_id.h"
 #include "content/common/buildflags.h"
 #include "content/common/content_export.h"
@@ -849,10 +848,6 @@ class CONTENT_EXPORT RenderWidget
   // Wraps the |webwidget_| as a MouseLockDispatcher::LockTarget interface.
   std::unique_ptr<MouseLockDispatcher::LockTarget> webwidget_mouse_lock_target_;
 
-  // Sometimes a parent-allocated LocalSurfaceId is used. But other times
-  // the child allocates its own with this.
-  viz::ChildLocalSurfaceIdAllocator child_local_surface_id_allocator_;
-
   viz::LocalSurfaceId local_surface_id_;
 
  private:
@@ -878,7 +873,7 @@ class CONTENT_EXPORT RenderWidget
                     int32_t* routing_id);
 
   void UpdateSurfaceAndScreenInfo(
-      viz::LocalSurfaceId new_local_surface_id,
+      const viz::LocalSurfaceId& new_local_surface_id,
       const gfx::Size& new_compositor_viewport_pixel_size,
       const ScreenInfo& new_screen_info);
 

content/renderer/render_widget_unittest.cc

@@ -22,6 +22,7 @@
 #include "content/public/common/content_features.h"
 #include "content/public/test/mock_render_thread.h"
 #include "content/renderer/devtools/render_widget_screen_metrics_emulator.h"
+#include "content/renderer/gpu/render_widget_compositor.h"
 #include "content/renderer/input/widget_input_handler_manager.h"
 #include "content/test/fake_compositor_dependencies.h"
 #include "content/test/mock_render_process.h"
@@ -178,13 +179,6 @@ class InteractiveRenderWidget : public RenderWidget {
     return local_surface_id_;
   }
 
-  void SetAutoResizeMode(bool enable) { auto_resize_mode_ = enable; }
-
-  void UpdateChildLocalSurfaceIdAllocatorForAutoResize(
-      const viz::LocalSurfaceId& parent_local_surface_id) {
-    child_local_surface_id_allocator_.UpdateFromParent(parent_local_surface_id);
-  }
-
  protected:
   ~InteractiveRenderWidget() override { webwidget_internal_ = nullptr; }
 
@@ -373,79 +367,27 @@ TEST_F(RenderWidgetUnittest, RenderWidgetInputEventUmaMetrics) {
       PASSIVE_LISTENER_UMA_ENUM_CANCELABLE_AND_CANCELED, 1);
 }
 
-// Tests that if a RenderWidget auto-resizes multiple times and receives an IPC
-// with a LocalSurfaceId, it will drop that LocalSurfaceId if it does not
-// correspond to the latest auto-resize request.
-TEST_F(RenderWidgetUnittest, SurfaceSynchronizationAutoResizeThrottling) {
-  if (!features::IsSurfaceSynchronizationEnabled())
-    return;
-
-  constexpr gfx::Size auto_size(100, 100);
+// Tests that if a RenderWidget is auto-resized, it requests a new
+// viz::LocalSurfaceId to be allocated on the impl thread.
+TEST_F(RenderWidgetUnittest, AutoResizeAllocatedLocalSurfaceId) {
   widget()->InitializeLayerTreeView();
-  widget()->SetAutoResizeMode(true);
 
   viz::ParentLocalSurfaceIdAllocator allocator;
-  viz::LocalSurfaceId initial_local_surface_id = allocator.GenerateId();
-  widget()->UpdateChildLocalSurfaceIdAllocatorForAutoResize(
-      initial_local_surface_id);
 
-  // Issue an auto-resize.
-  widget()->DidAutoResize(auto_size);
-  widget()->DidCommitCompositorFrame();
-
-  // Issue another auto-resize but keep it in-flight.
-  constexpr gfx::Size auto_size2(200, 200);
-  widget()->DidAutoResize(auto_size2);
-
-  // Send the LocalSurfaceId for the first Auto-Resize.
+  // Enable auto-resize.
   content::VisualProperties visual_properties;
   visual_properties.auto_resize_enabled = true;
-  visual_properties.min_size_for_auto_resize = auto_size;
-  visual_properties.max_size_for_auto_resize = auto_size2;
-  visual_properties.local_surface_id = allocator.GenerateId();
-  widget()->OnMessageReceived(ViewMsg_SynchronizeVisualProperties(
-      widget()->routing_id(), visual_properties));
-
-  // The LocalSurfaceId should not take because there's another in-flight auto-
-  // resize operation.
-  EXPECT_NE(widget()->local_surface_id(), visual_properties.local_surface_id);
-}
-
-// Tests that if a RenderWidget is auto-resized, it allocates its own
-// viz::LocalSurfaceId
-TEST_F(RenderWidgetUnittest, AutoResizeAllocatedLocalSurfaceId) {
-  viz::LocalSurfaceId fake_parent_local_surface_id(
-      1, base::UnguessableToken::Create());
-  widget()->UpdateChildLocalSurfaceIdAllocatorForAutoResize(
-      fake_parent_local_surface_id);
-  widget()->SetAutoResizeMode(true);
+  visual_properties.min_size_for_auto_resize = gfx::Size(100, 100);
+  visual_properties.max_size_for_auto_resize = gfx::Size(200, 200);
+  visual_properties.local_surface_id = allocator.GetCurrentLocalSurfaceId();
+  widget()->SynchronizeVisualProperties(visual_properties);
+  EXPECT_EQ(allocator.GetCurrentLocalSurfaceId(), widget()->local_surface_id());
+  EXPECT_FALSE(widget()->compositor()->HasNewLocalSurfaceIdRequest());
 
   constexpr gfx::Size size(200, 200);
   widget()->DidAutoResize(size);
-  viz::LocalSurfaceId local_surface_id1 = widget()->local_surface_id();
-
-  constexpr gfx::Size size2(100, 100);
-  widget()->DidAutoResize(size2);
-  viz::LocalSurfaceId local_surface_id2 = widget()->local_surface_id();
-
-  // Our first child allocated LSI should match |fake_parent_local_surface_id|
-  // with an incremented child sequence number.
-  EXPECT_NE(fake_parent_local_surface_id, local_surface_id1);
-  EXPECT_EQ(fake_parent_local_surface_id.parent_sequence_number(),
-            local_surface_id1.parent_sequence_number());
-  EXPECT_EQ(fake_parent_local_surface_id.child_sequence_number() + 1,
-            local_surface_id1.child_sequence_number());
-  EXPECT_EQ(fake_parent_local_surface_id.embed_token(),
-            local_surface_id2.embed_token());
-
-  // Our second child allocated LSI should match the first with an incremented
-  // child sequence number.
-  EXPECT_NE(local_surface_id1, local_surface_id2);
-  EXPECT_EQ(local_surface_id1.parent_sequence_number(),
-            local_surface_id2.parent_sequence_number());
-  EXPECT_EQ(local_surface_id1.child_sequence_number() + 1,
-            local_surface_id2.child_sequence_number());
-  EXPECT_EQ(local_surface_id1.embed_token(), local_surface_id2.embed_token());
+  EXPECT_EQ(allocator.GetCurrentLocalSurfaceId(), widget()->local_surface_id());
+  EXPECT_TRUE(widget()->compositor()->HasNewLocalSurfaceIdRequest());
 }
 
 class PopupRenderWidget : public RenderWidget {