Reland of Consolidate OutputSurface constructors into GL vs Vulkan. (…

…patchset chromium#1 id:1 of https://codereview.chromium.org/2013743002/ ) Reason for revert: Sorry but this patch was innocent: https://codereview.chromium.org/2006143006/ seems suspicious instead. Original issue's description: > Revert of Consolidate OutputSurface constructors into GL vs Vulkan. (patchset chromium#8 id:140001 of https://codereview.chromium.org/2002303002/ ) > > Reason for revert: > This causes flaky test failures (e.g. https://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA%29/builds/78761) > > Original issue's description: > > Consolidate OutputSurface constructors into GL vs Vulkan. > > > > Get rid of the many many constructors on OutputSurface, drop it down > > to constructors for GL based compositing vs Vulkan based. > > > > R=boliu@chromium.org, piman, sievers > > TBR=sky, torne > > BUG=487471 > > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel > > > > Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a > > Cr-Commit-Position: refs/heads/master@{#395478} > > > > Committed: https://crrev.com/d7b005182e7a5174db8aa8e763df67faa46c4cd5 > > Cr-Commit-Position: refs/heads/master@{#395728} > > TBR=boliu@chromium.org,piman@chromium.org,sievers@chromium.org,nyquist@chromium.org,sky@chromium.org,torne@chromium.org,rjkroege@chromium.org,fsamuel@chromium.org,danakj@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=487471 > > Committed: https://crrev.com/f7bae84201b3e573a274bde0f000ee766ac13130 > Cr-Commit-Position: refs/heads/master@{#395833} TBR=boliu@chromium.org,piman@chromium.org,sievers@chromium.org,nyquist@chromium.org,sky@chromium.org,torne@chromium.org,rjkroege@chromium.org,fsamuel@chromium.org,danakj@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=487471 Review-Url: https://codereview.chromium.org/2008893003 Cr-Commit-Position: refs/heads/master@{#395847}
hylschwy · May 25, 2016 · ea85b9d · ea85b9d
1 parent f7109fc
commit ea85b9d
Show file tree

Hide file tree

Showing 37 changed files with 345 additions and 480 deletions.
diff --git a/android_webview/browser/parent_output_surface.cc b/android_webview/browser/parent_output_surface.cc
@@ -11,7 +11,7 @@ namespace android_webview {
 
 ParentOutputSurface::ParentOutputSurface(
     scoped_refptr<cc::ContextProvider> context_provider)
-    : cc::OutputSurface(context_provider) {
+    : cc::OutputSurface(std::move(context_provider), nullptr, nullptr) {
   stencil_state_.stencil_test_enabled = false;
 }
 

diff --git a/blimp/client/feature/compositor/blimp_output_surface.cc b/blimp/client/feature/compositor/blimp_output_surface.cc
@@ -13,9 +13,8 @@ namespace blimp {
 namespace client {
 
 BlimpOutputSurface::BlimpOutputSurface(
-    const scoped_refptr<cc::ContextProvider>& context_provider)
-    : cc::OutputSurface(context_provider) {
-}
+    scoped_refptr<cc::ContextProvider> context_provider)
+    : cc::OutputSurface(std::move(context_provider), nullptr, nullptr) {}
 
 BlimpOutputSurface::~BlimpOutputSurface() {}
 

diff --git a/blimp/client/feature/compositor/blimp_output_surface.h b/blimp/client/feature/compositor/blimp_output_surface.h
@@ -18,7 +18,7 @@ namespace client {
 class BlimpOutputSurface : public cc::OutputSurface {
  public:
   explicit BlimpOutputSurface(
-      const scoped_refptr<cc::ContextProvider>& context_provider);
+      scoped_refptr<cc::ContextProvider> context_provider);
   ~BlimpOutputSurface() override;
 
   // OutputSurface implementation.

diff --git a/cc/output/gl_renderer_unittest.cc b/cc/output/gl_renderer_unittest.cc
@@ -1139,6 +1139,7 @@ class NonReshapableOutputSurface : public FakeOutputSurface {
   explicit NonReshapableOutputSurface(
       std::unique_ptr<TestWebGraphicsContext3D> context3d)
       : FakeOutputSurface(TestContextProvider::Create(std::move(context3d)),
+                          nullptr,
                           false) {
     surface_size_ = gfx::Size(500, 500);
   }
@@ -1761,9 +1762,11 @@ class OutputSurfaceMockContext : public TestWebGraphicsContext3D {
 class MockOutputSurface : public OutputSurface {
  public:
   MockOutputSurface()
-      : OutputSurface(TestContextProvider::Create(
-            std::unique_ptr<TestWebGraphicsContext3D>(
-                new StrictMock<OutputSurfaceMockContext>))) {
+      : OutputSurface(
+            TestContextProvider::Create(
+                base::MakeUnique<StrictMock<OutputSurfaceMockContext>>()),
+            nullptr,
+            nullptr) {
     surface_size_ = gfx::Size(100, 100);
   }
   virtual ~MockOutputSurface() {}

diff --git a/cc/output/output_surface.cc b/cc/output/output_surface.cc
@@ -121,51 +121,19 @@ class SkiaGpuTraceMemoryDump : public SkTraceMemoryDump {
 OutputSurface::OutputSurface(
     scoped_refptr<ContextProvider> context_provider,
     scoped_refptr<ContextProvider> worker_context_provider,
-    scoped_refptr<VulkanContextProvider> vulkan_context_provider,
     std::unique_ptr<SoftwareOutputDevice> software_device)
-    : client_(NULL),
-      context_provider_(std::move(context_provider)),
+    : context_provider_(std::move(context_provider)),
       worker_context_provider_(std::move(worker_context_provider)),
-      vulkan_context_provider_(vulkan_context_provider),
       software_device_(std::move(software_device)),
-      device_scale_factor_(-1),
-      has_alpha_(true),
-      external_stencil_test_enabled_(false),
       weak_ptr_factory_(this) {
   client_thread_checker_.DetachFromThread();
 }
 
-OutputSurface::OutputSurface(scoped_refptr<ContextProvider> context_provider)
-    : OutputSurface(std::move(context_provider),
-                    nullptr,
-                    nullptr,
-                    nullptr) {
-}
-
-OutputSurface::OutputSurface(
-    scoped_refptr<ContextProvider> context_provider,
-    scoped_refptr<ContextProvider> worker_context_provider)
-    : OutputSurface(std::move(context_provider),
-                    std::move(worker_context_provider),
-                    nullptr,
-                    nullptr) {
-}
-
-OutputSurface::OutputSurface(
-    std::unique_ptr<SoftwareOutputDevice> software_device)
-    : OutputSurface(nullptr,
-                    nullptr,
-                    nullptr,
-                    std::move(software_device)) {
-}
-
 OutputSurface::OutputSurface(
-    scoped_refptr<ContextProvider> context_provider,
-    std::unique_ptr<SoftwareOutputDevice> software_device)
-    : OutputSurface(std::move(context_provider),
-                    nullptr,
-                    nullptr,
-                    std::move(software_device)) {
+    scoped_refptr<VulkanContextProvider> vulkan_context_provider)
+    : vulkan_context_provider_(vulkan_context_provider),
+      weak_ptr_factory_(this) {
+  client_thread_checker_.DetachFromThread();
 }
 
 // Forwarded to OutputSurfaceClient

diff --git a/cc/output/output_surface.h b/cc/output/output_surface.h
@@ -46,17 +46,14 @@ class OutputSurfaceClient;
 //      surface (on the compositor thread) and go back to step 1.
 class CC_EXPORT OutputSurface : public base::trace_event::MemoryDumpProvider {
  public:
-  OutputSurface(scoped_refptr<ContextProvider> context_provider,
-                scoped_refptr<ContextProvider> worker_context_provider,
-                scoped_refptr<VulkanContextProvider> vulkan_context_provider,
-                std::unique_ptr<SoftwareOutputDevice> software_device);
-  OutputSurface(scoped_refptr<ContextProvider> context_provider,
-                scoped_refptr<ContextProvider> worker_context_provider);
-  explicit OutputSurface(scoped_refptr<ContextProvider> context_provider);
-  explicit OutputSurface(std::unique_ptr<SoftwareOutputDevice> software_device);
-
-  OutputSurface(scoped_refptr<ContextProvider> context_provider,
-                std::unique_ptr<SoftwareOutputDevice> software_device);
+  // Constructor for GL-based and/or software compositing.
+  explicit OutputSurface(scoped_refptr<ContextProvider> context_provider,
+                         scoped_refptr<ContextProvider> worker_context_provider,
+                         std::unique_ptr<SoftwareOutputDevice> software_device);
+
+  // Constructor for Vulkan-based compositing.
+  explicit OutputSurface(
+      scoped_refptr<VulkanContextProvider> vulkan_context_provider);
 
   ~OutputSurface() override;
 
@@ -172,18 +169,18 @@ class CC_EXPORT OutputSurface : public base::trace_event::MemoryDumpProvider {
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
  protected:
-  OutputSurfaceClient* client_;
-
   void PostSwapBuffersComplete();
 
+  OutputSurfaceClient* client_ = nullptr;
+
   struct OutputSurface::Capabilities capabilities_;
   scoped_refptr<ContextProvider> context_provider_;
   scoped_refptr<ContextProvider> worker_context_provider_;
   scoped_refptr<VulkanContextProvider> vulkan_context_provider_;
   std::unique_ptr<SoftwareOutputDevice> software_device_;
   gfx::Size surface_size_;
-  float device_scale_factor_;
-  bool has_alpha_;
+  float device_scale_factor_ = -1;
+  bool has_alpha_ = true;
   base::ThreadChecker client_thread_checker_;
 
   void SetNeedsRedrawRect(const gfx::Rect& damage_rect);
@@ -192,7 +189,7 @@ class CC_EXPORT OutputSurface : public base::trace_event::MemoryDumpProvider {
   void DetachFromClientInternal();
 
  private:
-  bool external_stencil_test_enabled_;
+  bool external_stencil_test_enabled_ = false;
 
   base::WeakPtrFactory<OutputSurface> weak_ptr_factory_;
 

diff --git a/cc/output/output_surface_unittest.cc b/cc/output/output_surface_unittest.cc
@@ -23,19 +23,23 @@ namespace {
 class TestOutputSurface : public OutputSurface {
  public:
   explicit TestOutputSurface(scoped_refptr<ContextProvider> context_provider)
-      : OutputSurface(context_provider) {}
+      : OutputSurface(std::move(context_provider), nullptr, nullptr) {}
 
   TestOutputSurface(scoped_refptr<ContextProvider> context_provider,
                     scoped_refptr<ContextProvider> worker_context_provider)
-      : OutputSurface(worker_context_provider) {}
+      : OutputSurface(std::move(context_provider),
+                      std::move(worker_context_provider),
+                      nullptr) {}
 
   explicit TestOutputSurface(
       std::unique_ptr<SoftwareOutputDevice> software_device)
-      : OutputSurface(std::move(software_device)) {}
+      : OutputSurface(nullptr, nullptr, std::move(software_device)) {}
 
   TestOutputSurface(scoped_refptr<ContextProvider> context_provider,
                     std::unique_ptr<SoftwareOutputDevice> software_device)
-      : OutputSurface(context_provider, std::move(software_device)) {}
+      : OutputSurface(std::move(context_provider),
+                      nullptr,
+                      std::move(software_device)) {}
 
   void SwapBuffers(CompositorFrame* frame) override {
     client_->DidSwapBuffers();
@@ -117,6 +121,8 @@ TEST(OutputSurfaceTest, ClientPointerIndicatesWorkerBindToClientSuccess) {
   EXPECT_TRUE(client.did_lose_output_surface_called());
 }
 
+// TODO(danakj): Add a test for worker context failure as well when
+// OutputSurface creates/binds it.
 TEST(OutputSurfaceTest, ClientPointerIndicatesBindToClientFailure) {
   scoped_refptr<TestContextProvider> context_provider =
       TestContextProvider::Create();
@@ -132,23 +138,6 @@ TEST(OutputSurfaceTest, ClientPointerIndicatesBindToClientFailure) {
   EXPECT_FALSE(output_surface.HasClient());
 }
 
-TEST(OutputSurfaceTest, ClientPointerIndicatesWorkerBindToClientFailure) {
-  scoped_refptr<TestContextProvider> context_provider =
-      TestContextProvider::Create();
-  scoped_refptr<TestContextProvider> worker_context_provider =
-      TestContextProvider::Create();
-
-  // Lose the context so BindToClient fails.
-  worker_context_provider->UnboundTestContext3d()->set_context_lost(true);
-
-  TestOutputSurface output_surface(context_provider, worker_context_provider);
-  EXPECT_FALSE(output_surface.HasClient());
-
-  FakeOutputSurfaceClient client;
-  EXPECT_FALSE(output_surface.BindToClient(&client));
-  EXPECT_FALSE(output_surface.HasClient());
-}
-
 TEST(OutputSurfaceTest, SoftwareOutputDeviceBackbufferManagement) {
   TestSoftwareOutputDevice* software_output_device =
       new TestSoftwareOutputDevice();

diff --git a/cc/output/overlay_unittest.cc b/cc/output/overlay_unittest.cc
@@ -134,7 +134,7 @@ size_t DefaultOverlayProcessor::GetStrategyCount() {
 class OverlayOutputSurface : public OutputSurface {
  public:
   explicit OverlayOutputSurface(scoped_refptr<ContextProvider> context_provider)
-      : OutputSurface(context_provider) {
+      : OutputSurface(context_provider, nullptr, nullptr) {
     surface_size_ = kDisplaySize;
     device_scale_factor_ = 1;
     is_displayed_as_overlay_plane_ = true;

diff --git a/cc/output/renderer_unittest.cc b/cc/output/renderer_unittest.cc
@@ -27,7 +27,7 @@ class TestOutputSurface : public OutputSurface {
 
 TestOutputSurface::TestOutputSurface(
     scoped_refptr<ContextProvider> context_provider)
-    : OutputSurface(std::move(context_provider)) {}
+    : OutputSurface(std::move(context_provider), nullptr, nullptr) {}
 
 TestOutputSurface::~TestOutputSurface() {
 }

diff --git a/cc/surfaces/surface_display_output_surface.cc b/cc/surfaces/surface_display_output_surface.cc
@@ -20,7 +20,8 @@ SurfaceDisplayOutputSurface::SurfaceDisplayOutputSurface(
     scoped_refptr<ContextProvider> context_provider,
     scoped_refptr<ContextProvider> worker_context_provider)
     : OutputSurface(std::move(context_provider),
-                    std::move(worker_context_provider)),
+                    std::move(worker_context_provider),
+                    nullptr),
       display_client_(nullptr),
       factory_(surface_manager, this),
       allocator_(allocator) {
@@ -37,11 +38,8 @@ SurfaceDisplayOutputSurface::SurfaceDisplayOutputSurface(
     SurfaceManager* surface_manager,
     SurfaceIdAllocator* allocator,
     scoped_refptr<VulkanContextProvider> vulkan_context_provider)
-    : OutputSurface(nullptr,
-                    nullptr,
-                    std::move(vulkan_context_provider),
-                    nullptr),
-      display_client_(NULL),
+    : OutputSurface(std::move(vulkan_context_provider)),
+      display_client_(nullptr),
       factory_(surface_manager, this),
       allocator_(allocator) {
   capabilities_.delegated_rendering = true;

diff --git a/cc/test/failure_output_surface.cc b/cc/test/failure_output_surface.cc
@@ -7,8 +7,7 @@
 namespace cc {
 
 FailureOutputSurface::FailureOutputSurface(bool is_delegating)
-    : FakeOutputSurface(static_cast<ContextProvider*>(nullptr), is_delegating) {
-}
+    : FakeOutputSurface(nullptr, nullptr, is_delegating) {}
 
 bool FailureOutputSurface::BindToClient(OutputSurfaceClient* client) {
   // This will force this output surface to not initialize in LTHI

diff --git a/cc/test/fake_output_surface.cc b/cc/test/fake_output_surface.cc
@@ -18,47 +18,27 @@ FakeOutputSurface::FakeOutputSurface(
     scoped_refptr<ContextProvider> context_provider,
     scoped_refptr<ContextProvider> worker_context_provider,
     bool delegated_rendering)
-    : OutputSurface(context_provider, worker_context_provider),
-      client_(NULL),
-      num_sent_frames_(0),
-      has_external_stencil_test_(false),
-      suspended_for_recycle_(false),
-      framebuffer_(0),
-      overlay_candidate_validator_(nullptr) {
-  capabilities_.delegated_rendering = delegated_rendering;
-}
-
-FakeOutputSurface::FakeOutputSurface(
-    scoped_refptr<ContextProvider> context_provider,
-    bool delegated_rendering)
-    : OutputSurface(context_provider),
-      client_(NULL),
-      num_sent_frames_(0),
-      has_external_stencil_test_(false),
-      suspended_for_recycle_(false),
-      framebuffer_(0),
-      overlay_candidate_validator_(nullptr) {
-  capabilities_.delegated_rendering = delegated_rendering;
-}
+    : FakeOutputSurface(std::move(context_provider),
+                        std::move(worker_context_provider),
+                        nullptr,
+                        delegated_rendering) {}
 
 FakeOutputSurface::FakeOutputSurface(
     std::unique_ptr<SoftwareOutputDevice> software_device,
     bool delegated_rendering)
-    : OutputSurface(std::move(software_device)),
-      client_(NULL),
-      num_sent_frames_(0),
-      has_external_stencil_test_(false),
-      suspended_for_recycle_(false),
-      framebuffer_(0),
-      overlay_candidate_validator_(nullptr) {
-  capabilities_.delegated_rendering = delegated_rendering;
-}
+    : FakeOutputSurface(nullptr,
+                        nullptr,
+                        std::move(software_device),
+                        delegated_rendering) {}
 
 FakeOutputSurface::FakeOutputSurface(
     scoped_refptr<ContextProvider> context_provider,
+    scoped_refptr<ContextProvider> worker_context_provider,
     std::unique_ptr<SoftwareOutputDevice> software_device,
     bool delegated_rendering)
-    : OutputSurface(context_provider, std::move(software_device)),
+    : OutputSurface(std::move(context_provider),
+                    std::move(worker_context_provider),
+                    std::move(software_device)),
       client_(NULL),
       num_sent_frames_(0),
       has_external_stencil_test_(false),

diff --git a/cc/test/fake_output_surface.h b/cc/test/fake_output_surface.h
@@ -60,7 +60,7 @@ class FakeOutputSurface : public OutputSurface {
   static std::unique_ptr<FakeOutputSurface>
   Create3dWithResourcelessSoftwareSupport() {
     return base::WrapUnique(new FakeOutputSurface(
-        TestContextProvider::Create(),
+        TestContextProvider::Create(), TestContextProvider::CreateWorker(),
         base::WrapUnique(new SoftwareOutputDevice), false));
   }
 
@@ -98,8 +98,9 @@ class FakeOutputSurface : public OutputSurface {
 
   static std::unique_ptr<FakeOutputSurface> CreateOffscreen(
       std::unique_ptr<TestWebGraphicsContext3D> context) {
-    std::unique_ptr<FakeOutputSurface> surface(new FakeOutputSurface(
-        TestContextProvider::Create(std::move(context)), false));
+    std::unique_ptr<FakeOutputSurface> surface(
+        new FakeOutputSurface(TestContextProvider::Create(std::move(context)),
+                              TestContextProvider::CreateWorker(), false));
     surface->capabilities_.uses_default_gl_framebuffer = false;
     return surface;
   }
@@ -152,10 +153,6 @@ class FakeOutputSurface : public OutputSurface {
   }
 
  protected:
-  FakeOutputSurface(
-      scoped_refptr<ContextProvider> context_provider,
-      bool delegated_rendering);
-
   FakeOutputSurface(scoped_refptr<ContextProvider> context_provider,
                     scoped_refptr<ContextProvider> worker_context_provider,
                     bool delegated_rendering);
@@ -164,6 +161,7 @@ class FakeOutputSurface : public OutputSurface {
                     bool delegated_rendering);
 
   FakeOutputSurface(scoped_refptr<ContextProvider> context_provider,
+                    scoped_refptr<ContextProvider> worker_context_provider,
                     std::unique_ptr<SoftwareOutputDevice> software_device,
                     bool delegated_rendering);