Fix Windows resize jitter with SkiaRenderer.

Display::Resize() was using ShallowFinishCHROMIUM() to ensure that all
pending swap buffer calls at the current size were issued before
returning. This is because D3D will scale swapped output if it doesn't
match the window size. Unfortunately there isn't an equivalent to
ShallowFinishCHROMIUM() for SkiaRenderer.

RootCompositorFrameSinkImpl::DisableSwapUntilResize() would run a
callback after calling Display::Resize() with size (0, 0). Resize()
would only return after all swap buffers calls were issued since
ShallowFinishCHROMIUM() would block until then.

Add Display::DisableSwapUntilResize() that take a callback and runs it
after all pending swaps have completed. This works with both GLRenderer
and SkiaRenderer plus it doesn't block the display compositor thread
while waiting for swaps to complete.

Finally remove RendererSettings::finish_rendering_on_resize. This
setting was only true on Windows and only did anything when Resize() was
called with size (0, 0) for DisableSwapUntilResize(). Since
DisableSwapUntilResize() is also only used on Windows, just assume we
want to finish rendering if DisableSwapUntilResize() is
called.

Bug: 809722
Change-Id: Ia6117dd03d8da9dbfcb1dc97d197d85c8634d647
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652685Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669004}

components/viz/common/display/renderer_settings.h

@@ -27,7 +27,6 @@ class VIZ_COMMON_EXPORT RendererSettings {
   bool force_antialiasing = false;
   bool force_blending_with_shaders = false;
   bool partial_swap_enabled = false;
-  bool finish_rendering_on_resize = false;
   bool should_clear_root_render_pass = true;
   bool release_overlay_resources_after_gpu_query = false;
   bool tint_gl_composited_content = false;

components/viz/host/renderer_settings_creation.cc

@@ -51,9 +51,7 @@ RendererSettings CreateRendererSettings() {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   renderer_settings.partial_swap_enabled =
       !command_line->HasSwitch(switches::kUIDisablePartialSwap);
-#if defined(OS_WIN)
-  renderer_settings.finish_rendering_on_resize = true;
-#elif defined(OS_MACOSX)
+#if defined(OS_MACOSX)
   renderer_settings.release_overlay_resources_after_gpu_query = true;
   renderer_settings.auto_resize_output_surface = false;
 #elif defined(OS_CHROMEOS)

components/viz/service/display/display.cc

@@ -153,6 +153,9 @@ Display::~Display() {
   }
 #endif
 
+  if (no_pending_swaps_callback_)
+    std::move(no_pending_swaps_callback_).Run();
+
   for (auto& observer : observers_)
     observer.OnDisplayDestroyed();
   observers_.Clear();
@@ -250,21 +253,41 @@ void Display::Resize(const gfx::Size& size) {
 
   TRACE_EVENT0("viz", "Display::Resize");
 
-  // Need to ensure all pending swaps have executed before the window is
-  // resized, or D3D11 will scale the swap output.
-  if (settings_.finish_rendering_on_resize) {
-    if (!swapped_since_resize_ && scheduler_)
-      scheduler_->ForceImmediateSwapIfPossible();
-    if (swapped_since_resize_ && output_surface_ &&
-        output_surface_->context_provider())
-      output_surface_->context_provider()->ContextGL()->ShallowFinishCHROMIUM();
-  }
+  // Resize() shouldn't be called while waiting for pending swaps to ack unless
+  // it's being called with size (0, 0) to disable DrawAndSwap().
+  DCHECK(no_pending_swaps_callback_.is_null() || size.IsEmpty());
+
   swapped_since_resize_ = false;
   current_surface_size_ = size;
   if (scheduler_)
     scheduler_->DisplayResized();
 }
 
+void Display::DisableSwapUntilResize(
+    base::OnceClosure no_pending_swaps_callback) {
+  TRACE_EVENT0("viz", "Display::DisableSwapUntilResize");
+  DCHECK(no_pending_swaps_callback_.is_null());
+
+  if (!current_surface_size_.IsEmpty()) {
+    DCHECK(scheduler_);
+
+    if (!swapped_since_resize_)
+      scheduler_->ForceImmediateSwapIfPossible();
+
+    if (no_pending_swaps_callback && scheduler_->pending_swaps() > 0 &&
+        (output_surface_->context_provider() ||
+         output_surface_->AsSkiaOutputSurface())) {
+      no_pending_swaps_callback_ = std::move(no_pending_swaps_callback);
+    }
+
+    Resize(gfx::Size());
+  }
+
+  // There are no pending swaps for current size so immediately run callback.
+  if (no_pending_swaps_callback)
+    std::move(no_pending_swaps_callback).Run();
+}
+
 void Display::SetColorMatrix(const SkMatrix44& matrix) {
   if (output_surface_)
     output_surface_->set_color_matrix(matrix);
@@ -577,8 +600,12 @@ bool Display::DrawAndSwap() {
 }
 
 void Display::DidReceiveSwapBuffersAck() {
-  if (scheduler_)
+  if (scheduler_) {
     scheduler_->DidReceiveSwapBuffersAck();
+    if (no_pending_swaps_callback_ && scheduler_->pending_swaps() == 0)
+      std::move(no_pending_swaps_callback_).Run();
+  }
+
   if (renderer_)
     renderer_->SwapBuffersComplete();
 }

components/viz/service/display/display.h

@@ -98,6 +98,14 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
   void SetVisible(bool visible);
   void Resize(const gfx::Size& new_size);
 
+  // Stop drawing until Resize() is called with a new size. If the display
+  // hasn't drawn a frame at the current size *and* it's possible to immediately
+  // draw then this will run DrawAndSwap() first.
+  //
+  // |no_pending_swaps_callback| will be run there are no more swaps pending and
+  // may be run immediately.
+  void DisableSwapUntilResize(base::OnceClosure no_pending_swaps_callback);
+
   // Sets the color matrix that will be used to transform the output of this
   // display. This is only supported for GPU compositing.
   void SetColorMatrix(const SkMatrix44& matrix);
@@ -197,6 +205,9 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
       std::pair<base::TimeTicks, std::vector<Surface::PresentedCallback>>>
       pending_presented_callbacks_;
 
+  // Callback that will be run after all pending swaps have acked.
+  base::OnceClosure no_pending_swaps_callback_;
+
   int64_t swapped_trace_id_ = 0;
   int64_t last_presented_trace_id_ = 0;
 

components/viz/service/display/display_scheduler.h

@@ -44,6 +44,8 @@ class VIZ_SERVICE_EXPORT DisplayScheduler : public BeginFrameObserverBase,
                    bool wait_for_all_surfaces_before_draw = false);
   ~DisplayScheduler() override;
 
+  int pending_swaps() const { return pending_swaps_; }
+
   void SetClient(DisplaySchedulerClient* client);
 
   void SetVisible(bool visible);

components/viz/service/display/display_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/null_task_runner.h"
 #include "cc/base/math_util.h"
@@ -142,18 +143,11 @@ class DisplayTest : public testing::Test {
                                      std::move(output_surface));
   }
 
-  void SetUpGpuDisplay(const RendererSettings& settings,
-                       std::unique_ptr<TestGLES2Interface> context = nullptr) {
-    std::unique_ptr<FakeOutputSurface> output_surface;
-    scoped_refptr<TestContextProvider> provider;
-    if (context) {
-      provider = TestContextProvider::Create(std::move(context));
-
-    } else {
-      provider = TestContextProvider::Create();
-    }
+  void SetUpGpuDisplay(const RendererSettings& settings) {
+    scoped_refptr<TestContextProvider> provider = TestContextProvider::Create();
     provider->BindToCurrentThread();
-    output_surface = FakeOutputSurface::Create3d(std::move(provider));
+    std::unique_ptr<FakeOutputSurface> output_surface =
+        FakeOutputSurface::Create3d(std::move(provider));
     output_surface_ = output_surface.get();
 
     CreateDisplaySchedulerAndDisplay(settings, kArbitraryFrameSinkId,
@@ -237,7 +231,6 @@ class DisplayTest : public testing::Test {
 TEST_F(DisplayTest, DisplayDamaged) {
   RendererSettings settings;
   settings.partial_swap_enabled = true;
-  settings.finish_rendering_on_resize = true;
   SetUpSoftwareDisplay(settings);
   gfx::ColorSpace color_space_1 = gfx::ColorSpace::CreateXYZD50();
   gfx::ColorSpace color_space_2 = gfx::ColorSpace::CreateSCRGBLinear();
@@ -458,7 +451,8 @@ TEST_F(DisplayTest, DisplayDamaged) {
     EXPECT_EQ(4u, output_surface_->num_sent_frames());
   }
 
-  // Resize should cause a swap if no frame was swapped at the previous size.
+  // DisableSwapUntilResize() should cause a swap if no frame was swapped at the
+  // previous size.
   {
     id_allocator_.GenerateId();
     display_->SetLocalSurfaceId(
@@ -484,6 +478,7 @@ TEST_F(DisplayTest, DisplayDamaged) {
     EXPECT_FALSE(scheduler_->has_new_root_surface);
 
     scheduler_->swapped = false;
+    display_->DisableSwapUntilResize(base::OnceClosure());
     display_->Resize(gfx::Size(100, 100));
     EXPECT_TRUE(scheduler_->swapped);
     EXPECT_EQ(5u, output_surface_->num_sent_frames());
@@ -530,9 +525,7 @@ TEST_F(DisplayTest, DisplayDamaged) {
 
 // Verifies latency info is stored only up to a limit if a swap fails.
 void DisplayTest::LatencyInfoCapTest(bool over_capacity) {
-  RendererSettings settings;
-  settings.finish_rendering_on_resize = true;
-  SetUpSoftwareDisplay(settings);
+  SetUpSoftwareDisplay(RendererSettings());
 
   StubDisplayClient client;
   display_->Initialize(&client, manager_.surface_manager());
@@ -582,6 +575,7 @@ void DisplayTest::LatencyInfoCapTest(bool over_capacity) {
   CompositorFrame frame3 =
       CompositorFrameBuilder().AddRenderPass(kOutputRect, kDamageRect).Build();
   support_->SubmitCompositorFrame(local_surface_id, std::move(frame3));
+  EXPECT_TRUE(display_->DrawAndSwap());
 
   // Verify whether or not LatencyInfo was dropped.
   size_t expected_size = 1;  // The Display adds its own latency info.
@@ -602,12 +596,7 @@ TEST_F(DisplayTest, OverLatencyInfoCap) {
   LatencyInfoCapTest(true);
 }
 
-class MockedGLES2Interface : public TestGLES2Interface {
- public:
-  MOCK_METHOD0(ShallowFinishCHROMIUM, void());
-};
-
-TEST_F(DisplayTest, Finish) {
+TEST_F(DisplayTest, DisableSwapUntilResize) {
   id_allocator_.GenerateId();
   LocalSurfaceId local_surface_id1(
       id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
@@ -617,13 +606,8 @@ TEST_F(DisplayTest, Finish) {
 
   RendererSettings settings;
   settings.partial_swap_enabled = true;
-  settings.finish_rendering_on_resize = true;
 
-  auto gl = std::make_unique<MockedGLES2Interface>();
-  MockedGLES2Interface* gl_ptr = gl.get();
-  EXPECT_CALL(*gl_ptr, ShallowFinishCHROMIUM()).Times(0);
-
-  SetUpGpuDisplay(settings, std::move(gl));
+  SetUpGpuDisplay(settings);
 
   StubDisplayClient client;
   display_->Initialize(&client, manager_.surface_manager());
@@ -643,22 +627,26 @@ TEST_F(DisplayTest, Finish) {
     SubmitCompositorFrame(&pass_list, local_surface_id1);
   }
 
-  display_->DrawAndSwap();
+  EXPECT_FALSE(scheduler_->swapped);
 
-  // First resize and draw shouldn't finish.
-  testing::Mock::VerifyAndClearExpectations(gl_ptr);
+  // DisableSwapUntilResize() should trigger a swap because we have a frame of
+  // the correct size and haven't swapped at that size yet.
+  bool swap_callback_run = false;
+  display_->DisableSwapUntilResize(base::BindLambdaForTesting(
+      [&swap_callback_run]() { swap_callback_run = true; }));
+  EXPECT_TRUE(scheduler_->swapped);
+  EXPECT_TRUE(swap_callback_run);
 
-  EXPECT_CALL(*gl_ptr, ShallowFinishCHROMIUM());
   display_->Resize(gfx::Size(150, 150));
-  testing::Mock::VerifyAndClearExpectations(gl_ptr);
+  scheduler_->swapped = false;
 
-  // Another resize without a swap doesn't need to finish.
-  EXPECT_CALL(*gl_ptr, ShallowFinishCHROMIUM()).Times(0);
+  // DisableSwapUntilResize() won't trigger a swap because there is no frame
+  // of the correct size to draw.
   display_->SetLocalSurfaceId(local_surface_id2, 1.f);
+  display_->DisableSwapUntilResize(base::OnceClosure());
+  EXPECT_FALSE(scheduler_->swapped);
   display_->Resize(gfx::Size(200, 200));
-  testing::Mock::VerifyAndClearExpectations(gl_ptr);
 
-  EXPECT_CALL(*gl_ptr, ShallowFinishCHROMIUM()).Times(0);
   {
     RenderPassList pass_list;
     auto pass = RenderPass::Create();
@@ -670,13 +658,16 @@ TEST_F(DisplayTest, Finish) {
     SubmitCompositorFrame(&pass_list, local_surface_id2);
   }
 
+  // DrawAndSwap() should trigger a swap at current size.
   display_->DrawAndSwap();
+  EXPECT_TRUE(scheduler_->swapped);
+  scheduler_->swapped = false;
 
-  testing::Mock::VerifyAndClearExpectations(gl_ptr);
+  // DisableSwapUntilResize() won't trigger another swap because we already
+  // swapped a frame at the current size.
+  display_->DisableSwapUntilResize(base::OnceClosure());
+  EXPECT_FALSE(scheduler_->swapped);
 
-  EXPECT_CALL(*gl_ptr, ShallowFinishCHROMIUM());
-  display_->Resize(gfx::Size(250, 250));
-  testing::Mock::VerifyAndClearExpectations(gl_ptr);
   TearDownDisplay();
 }
 

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -156,8 +156,7 @@ void RootCompositorFrameSinkImpl::SetDisplayVisible(bool visible) {
 
 void RootCompositorFrameSinkImpl::DisableSwapUntilResize(
     DisableSwapUntilResizeCallback callback) {
-  display_->Resize(gfx::Size());
-  std::move(callback).Run();
+  display_->DisableSwapUntilResize(std::move(callback));
 }
 
 void RootCompositorFrameSinkImpl::Resize(const gfx::Size& size) {

services/viz/privileged/interfaces/compositing/renderer_settings.mojom

@@ -10,7 +10,6 @@ import "ui/gfx/mojo/color_space.mojom";
 
 struct RendererSettings {
   bool allow_antialiasing;
-  bool finish_rendering_on_resize;
   bool force_antialiasing;
   bool force_blending_with_shaders;
   bool tint_gl_composited_content;

services/viz/privileged/interfaces/compositing/renderer_settings_struct_traits.cc

@@ -20,7 +20,6 @@ bool StructTraits<viz::mojom::RendererSettingsDataView, viz::RendererSettings>::
   out->force_antialiasing = data.force_antialiasing();
   out->force_blending_with_shaders = data.force_blending_with_shaders();
   out->partial_swap_enabled = data.partial_swap_enabled();
-  out->finish_rendering_on_resize = data.finish_rendering_on_resize();
   out->should_clear_root_render_pass = data.should_clear_root_render_pass();
   out->release_overlay_resources_after_gpu_query =
       data.release_overlay_resources_after_gpu_query();

services/viz/privileged/interfaces/compositing/renderer_settings_struct_traits.h

@@ -36,10 +36,6 @@ struct StructTraits<viz::mojom::RendererSettingsDataView,
     return input.partial_swap_enabled;
   }
 
-  static bool finish_rendering_on_resize(const viz::RendererSettings& input) {
-    return input.finish_rendering_on_resize;
-  }
-
   static bool should_clear_root_render_pass(
       const viz::RendererSettings& input) {
     return input.should_clear_root_render_pass;

services/viz/privileged/interfaces/struct_traits_unittest.cc

@@ -24,7 +24,6 @@ TEST_F(StructTraitsTest, RendererSettings) {
   input.force_antialiasing = true;
   input.force_blending_with_shaders = true;
   input.partial_swap_enabled = true;
-  input.finish_rendering_on_resize = true;
   input.should_clear_root_render_pass = false;
   input.release_overlay_resources_after_gpu_query = true;
   input.show_overdraw_feedback = true;
@@ -40,8 +39,6 @@ TEST_F(StructTraitsTest, RendererSettings) {
   EXPECT_EQ(input.force_blending_with_shaders,
             output.force_blending_with_shaders);
   EXPECT_EQ(input.partial_swap_enabled, output.partial_swap_enabled);
-  EXPECT_EQ(input.finish_rendering_on_resize,
-            output.finish_rendering_on_resize);
   EXPECT_EQ(input.should_clear_root_render_pass,
             output.should_clear_root_render_pass);
   EXPECT_EQ(input.release_overlay_resources_after_gpu_query,

ui/compositor/test/fake_context_factory.cc

@@ -24,9 +24,7 @@
 namespace ui {
 
 FakeContextFactory::FakeContextFactory() {
-#if defined(OS_WIN)
-  renderer_settings_.finish_rendering_on_resize = true;
-#elif defined(OS_MACOSX)
+#if defined(OS_MACOSX)
   renderer_settings_.release_overlay_resources_after_gpu_query = true;
   // Ensure that tests don't wait for frames that will never come.
   ui::CATransactionCoordinator::Get().DisableForTesting();

ui/compositor/test/in_process_context_factory.cc

@@ -170,9 +170,7 @@ InProcessContextFactory::InProcessContextFactory(
   DCHECK_NE(gl::GetGLImplementation(), gl::kGLImplementationNone)
       << "If running tests, ensure that main() is calling "
       << "gl::GLSurfaceTestSupport::InitializeOneOff()";
-#if defined(OS_WIN)
-  renderer_settings_.finish_rendering_on_resize = true;
-#elif defined(OS_MACOSX)
+#if defined(OS_MACOSX)
   renderer_settings_.release_overlay_resources_after_gpu_query = true;
   // Ensure that tests don't wait for frames that will never come.
   ui::CATransactionCoordinator::Get().DisableForTesting();

ui/gl/gl_surface_egl.cc

@@ -1562,6 +1562,8 @@ gfx::SwapResult NativeViewGLSurfaceEGL::PostSubBuffer(
     int width,
     int height,
     PresentationCallback callback) {
+  TRACE_EVENT2("gpu", "NativeViewGLSurfaceEGL:PostSubBuffer", "width", width,
+               "height", height);
   DCHECK(supports_post_sub_buffer_);
   if (!CommitAndClearPendingOverlays()) {
     DVLOG(1) << "Failed to commit pending overlay planes.";