Make Surface reference its attached SwapChain

This solves an issues where when switching swapchains the previous
one was destroyed before the new one was created, doing so detached
itself from the Surface, which in turn made the new swapchain not do
a graceful transition via vkSwapchainCreateInfoKHR::oldSwapchain.

Keeping the reference on the surface makes sure we always have
knowledge of the previous swapchain when replacing it. It requires
re-working the lifetime model of NewSwapChainBase to not require a
call to DetachFromSurface in the destructor, and having the Device
explicitly tell a swapchain it got attached on creation (otherwise
there are ASSERTs firing when swapchain creation fails).

In addition, backends are changed to use a SwapChain::Create method
and fail with a validation error (for now) when the previous swapchain
didn't use the same API.

vulkan::SwapChain is updated to use the previous swapchain's device's
fenced deleter to destroy it which is important in the device
switching tests.

The SwapChainValidationTests are updated because with the lifetime
changes the texture view can be kept alive after the application has
lost the last reference to the wgpu::SwapChain.

TBRing since it was reviewed in a different CL (but for the wrong
branch).

TBR=enga@chromium.org
TBR=senorblanco@chromium.org

Bug: dawn:269
Change-Id: Ie4374b5685af990d68969ab9cd7767e53c287ace
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/31041
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/Device.cpp

@@ -1017,21 +1017,24 @@ namespace dawn_native {
             DAWN_TRY(ValidateSwapChainDescriptor(this, surface, descriptor));
         }
 
+        // TODO(dawn:269): Remove this code path once implementation-based swapchains are removed.
         if (surface == nullptr) {
             DAWN_TRY_ASSIGN(*result, CreateSwapChainImpl(descriptor));
         } else {
             ASSERT(descriptor->implementation == 0);
 
             NewSwapChainBase* previousSwapChain = surface->GetAttachedSwapChain();
-            NewSwapChainBase* newSwapChain;
+            ResultOrError<NewSwapChainBase*> maybeNewSwapChain =
-            DAWN_TRY_ASSIGN(newSwapChain,
+                CreateSwapChainImpl(surface, previousSwapChain, descriptor);
-                            CreateSwapChainImpl(surface, previousSwapChain, descriptor));
 
             if (previousSwapChain != nullptr) {
-                ASSERT(!previousSwapChain->IsAttached());
+                previousSwapChain->DetachFromSurface();
             }
-            ASSERT(newSwapChain->IsAttached());
 
+            NewSwapChainBase* newSwapChain = nullptr;
+            DAWN_TRY_ASSIGN(newSwapChain, std::move(maybeNewSwapChain));
+
+            newSwapChain->SetIsAttached();
             surface->SetAttachedSwapChain(newSwapChain);
             *result = newSwapChain;
         }

src/dawn_native/Surface.cpp

@@ -151,8 +151,8 @@ namespace dawn_native {
         }
     }
 
-    NewSwapChainBase* Surface::GetAttachedSwapChain() const {
+    NewSwapChainBase* Surface::GetAttachedSwapChain() {
-        return mSwapChain;
+        return mSwapChain.Get();
     }
 
     void Surface::SetAttachedSwapChain(NewSwapChainBase* swapChain) {

src/dawn_native/Surface.h

@@ -36,7 +36,7 @@ namespace dawn_native {
         Surface(InstanceBase* instance, const SurfaceDescriptor* descriptor);
 
         void SetAttachedSwapChain(NewSwapChainBase* swapChain);
-        NewSwapChainBase* GetAttachedSwapChain() const;
+        NewSwapChainBase* GetAttachedSwapChain();
 
         // These are valid to call on all Surfaces.
         enum class Type { MetalLayer, WindowsHWND, Xlib };
@@ -61,7 +61,7 @@ namespace dawn_native {
         Type mType;
 
         // The swapchain will set this to null when it is destroyed.
-        NewSwapChainBase* mSwapChain = nullptr;
+        Ref<NewSwapChainBase> mSwapChain;
 
         // MetalLayer
         void* mMetalLayer = nullptr;

src/dawn_native/SwapChain.cpp

@@ -261,7 +261,7 @@ namespace dawn_native {
                                        Surface* surface,
                                        const SwapChainDescriptor* descriptor)
         : SwapChainBase(device),
-          mAttached(true),
+          mAttached(false),
           mWidth(descriptor->width),
           mHeight(descriptor->height),
           mFormat(descriptor->format),
@@ -277,18 +277,20 @@ namespace dawn_native {
         }
 
         ASSERT(!mAttached);
-        ASSERT(mSurface == nullptr);
     }
 
     void NewSwapChainBase::DetachFromSurface() {
         if (mAttached) {
             DetachFromSurfaceImpl();
-            GetSurface()->SetAttachedSwapChain(nullptr);
             mSurface = nullptr;
             mAttached = false;
         }
     }
 
+    void NewSwapChainBase::SetIsAttached() {
+        mAttached = true;
+    }
+
     void NewSwapChainBase::Configure(wgpu::TextureFormat format,
                                      wgpu::TextureUsage allowedUsage,
                                      uint32_t width,

src/dawn_native/SwapChain.h

@@ -94,13 +94,15 @@ namespace dawn_native {
                          Surface* surface,
                          const SwapChainDescriptor* descriptor);
 
-        // This is called when the swapchain is detached for any reason:
+        // This is called when the swapchain is detached when one of the following happens:
         //
-        //  - The swapchain is being destroyed.
         //  - The surface it is attached to is being destroyed.
         //  - The swapchain is being replaced by another one on the surface.
         //
-        // The call for the old swapchain being replaced should be called inside the backend
+        // Note that the surface has a Ref on the last swapchain that was used on it so the
+        // SwapChain destructor will only be called after one of the things above happens.
+        //
+        // The call for the detaching previous swapchain should be called inside the backend
         // implementation of SwapChains. This is to allow them to acquire any resources before
         // calling detach to make a seamless transition from the previous swapchain.
         //
@@ -109,6 +111,8 @@ namespace dawn_native {
         // destructor.
         void DetachFromSurface();
 
+        void SetIsAttached();
+
         // Dawn API
         void Configure(wgpu::TextureFormat format,
                        wgpu::TextureUsage allowedUsage,

src/dawn_native/metal/DeviceMTL.mm

@@ -163,7 +163,7 @@ namespace dawn_native { namespace metal {
         Surface* surface,
         NewSwapChainBase* previousSwapChain,
         const SwapChainDescriptor* descriptor) {
-        return new SwapChain(this, surface, previousSwapChain, descriptor);
+        return SwapChain::Create(this, surface, previousSwapChain, descriptor);
     }
     ResultOrError<Ref<TextureBase>> Device::CreateTextureImpl(const TextureDescriptor* descriptor) {
         return AcquireRef(new Texture(this, descriptor));

src/dawn_native/metal/SwapChainMTL.h

@@ -37,13 +37,15 @@ namespace dawn_native { namespace metal {
 
     class SwapChain final : public NewSwapChainBase {
       public:
-        SwapChain(Device* device,
+        static ResultOrError<SwapChain*> Create(Device* device,
-                  Surface* surface,
+                                                Surface* surface,
-                  NewSwapChainBase* previousSwapChain,
+                                                NewSwapChainBase* previousSwapChain,
-                  const SwapChainDescriptor* descriptor);
+                                                const SwapChainDescriptor* descriptor);
+        ~SwapChain() override;
 
       private:
-        ~SwapChain() override;
+        using NewSwapChainBase::NewSwapChainBase;
+        MaybeError Initialize(NewSwapChainBase* previousSwapChain);
 
         CAMetalLayer* mLayer = nullptr;
 

src/dawn_native/metal/SwapChainMTL.mm

@@ -57,22 +57,36 @@ namespace dawn_native { namespace metal {
 
     // SwapChain
 
-    SwapChain::SwapChain(Device* device,
+    // static
-                         Surface* surface,
+    ResultOrError<SwapChain*> SwapChain::Create(Device* device,
-                         NewSwapChainBase* previousSwapChain,
+                                                Surface* surface,
-                         const SwapChainDescriptor* descriptor)
+                                                NewSwapChainBase* previousSwapChain,
-        : NewSwapChainBase(device, surface, descriptor) {
+                                                const SwapChainDescriptor* descriptor) {
-        ASSERT(surface->GetType() == Surface::Type::MetalLayer);
+        std::unique_ptr<SwapChain> swapchain =
+            std::make_unique<SwapChain>(device, surface, descriptor);
+        DAWN_TRY(swapchain->Initialize(previousSwapChain));
+        return swapchain.release();
+    }
+
+    SwapChain::~SwapChain() {
+        DetachFromSurface();
+    }
+
+    MaybeError SwapChain::Initialize(NewSwapChainBase* previousSwapChain) {
+        ASSERT(GetSurface()->GetType() == Surface::Type::MetalLayer);
 
         if (previousSwapChain != nullptr) {
             // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by
             // multiple backends one after the other. It probably needs to block until the backend
             // and GPU are completely finished with the previous swapchain.
-            ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Metal);
+            if (previousSwapChain->GetBackendType() != wgpu::BackendType::Metal) {
+                return DAWN_VALIDATION_ERROR("metal::SwapChain cannot switch between APIs");
+            }
+
             previousSwapChain->DetachFromSurface();
         }
 
-        mLayer = static_cast<CAMetalLayer*>(surface->GetMetalLayer());
+        mLayer = static_cast<CAMetalLayer*>(GetSurface()->GetMetalLayer());
         ASSERT(mLayer != nullptr);
 
         CGSize size = {};
@@ -91,10 +105,8 @@ namespace dawn_native { namespace metal {
 #endif  // defined(DAWN_PLATFORM_MACOS)
 
         // There is no way to control Fifo vs. Mailbox in Metal.
-    }
 
-    SwapChain::~SwapChain() {
+        return {};
-        DetachFromSurface();
     }
 
     MaybeError SwapChain::PresentImpl() {

src/dawn_native/null/DeviceNull.cpp

@@ -140,7 +140,7 @@ namespace dawn_native { namespace null {
         Surface* surface,
         NewSwapChainBase* previousSwapChain,
         const SwapChainDescriptor* descriptor) {
-        return new SwapChain(this, surface, previousSwapChain, descriptor);
+        return SwapChain::Create(this, surface, previousSwapChain, descriptor);
     }
     ResultOrError<Ref<TextureBase>> Device::CreateTextureImpl(const TextureDescriptor* descriptor) {
         return AcquireRef(new Texture(this, descriptor, TextureBase::TextureState::OwnedInternal));
@@ -347,23 +347,31 @@ namespace dawn_native { namespace null {
 
     // SwapChain
 
-    SwapChain::SwapChain(Device* device,
+    // static
-                         Surface* surface,
+    ResultOrError<SwapChain*> SwapChain::Create(Device* device,
-                         NewSwapChainBase* previousSwapChain,
+                                                Surface* surface,
-                         const SwapChainDescriptor* descriptor)
+                                                NewSwapChainBase* previousSwapChain,
-        : NewSwapChainBase(device, surface, descriptor) {
+                                                const SwapChainDescriptor* descriptor) {
+        std::unique_ptr<SwapChain> swapchain =
+            std::make_unique<SwapChain>(device, surface, descriptor);
+        DAWN_TRY(swapchain->Initialize(previousSwapChain));
+        return swapchain.release();
+    }
+
+    MaybeError SwapChain::Initialize(NewSwapChainBase* previousSwapChain) {
         if (previousSwapChain != nullptr) {
             // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by
             // multiple backends one after the other. It probably needs to block until the backend
             // and GPU are completely finished with the previous swapchain.
-            ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Null);
+            if (previousSwapChain->GetBackendType() != wgpu::BackendType::Null) {
-            previousSwapChain->DetachFromSurface();
+                return DAWN_VALIDATION_ERROR("null::SwapChain cannot switch between APIs");
+            }
         }
+
+        return {};
     }
 
-    SwapChain::~SwapChain() {
+    SwapChain::~SwapChain() = default;
-        DetachFromSurface();
-    }
 
     MaybeError SwapChain::PresentImpl() {
         mTexture->Destroy();

src/dawn_native/null/DeviceNull.h

@@ -251,13 +251,15 @@ namespace dawn_native { namespace null {
 
     class SwapChain final : public NewSwapChainBase {
       public:
-        SwapChain(Device* device,
+        static ResultOrError<SwapChain*> Create(Device* device,
-                  Surface* surface,
+                                                Surface* surface,
-                  NewSwapChainBase* previousSwapChain,
+                                                NewSwapChainBase* previousSwapChain,
-                  const SwapChainDescriptor* descriptor);
+                                                const SwapChainDescriptor* descriptor);
+        ~SwapChain() override;
 
       private:
-        ~SwapChain() override;
+        using NewSwapChainBase::NewSwapChainBase;
+        MaybeError Initialize(NewSwapChainBase* previousSwapChain);
 
         Ref<Texture> mTexture;
 

src/dawn_native/vulkan/SwapChainVk.cpp

@@ -189,7 +189,7 @@ namespace dawn_native { namespace vulkan {
         Device* device = ToBackend(GetDevice());
         Adapter* adapter = ToBackend(GetDevice()->GetAdapter());
 
-        VkSwapchainKHR oldVkSwapChain = VK_NULL_HANDLE;
+        VkSwapchainKHR previousVkSwapChain = VK_NULL_HANDLE;
 
         if (previousSwapChain != nullptr) {
             // TODO(cwallez@chromium.org): The first time a surface is used with a Device, check
@@ -198,30 +198,33 @@ namespace dawn_native { namespace vulkan {
             // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by
             // multiple backends one after the other. It probably needs to block until the backend
             // and GPU are completely finished with the previous swapchain.
-            ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Vulkan);
+            if (previousSwapChain->GetBackendType() != wgpu::BackendType::Vulkan) {
-
+                return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between APIs");
-            // The previous swapchain is a dawn_native::vulkan::SwapChain so we can reuse its
+            }
-            // VkSurfaceKHR provided they are on the same instance.
-            // TODO(cwallez@chromium.org): check they are the same instance.
             // TODO(cwallez@chromium.org): use ToBackend once OldSwapChainBase is removed.
             SwapChain* previousVulkanSwapChain = static_cast<SwapChain*>(previousSwapChain);
-            std::swap(previousVulkanSwapChain->mVkSurface, mVkSurface);
 
             // TODO(cwallez@chromium.org): Figure out switching a single surface between multiple
-            // Vulkan devices. Probably needs to block too, but could reuse the surface!
+            // Vulkan devices on different VkInstances. Probably needs to block too!
-            ASSERT(previousSwapChain->GetDevice() == GetDevice());
+            VkInstance previousInstance =
-
+                ToBackend(previousSwapChain->GetDevice())->GetVkInstance();
-            // The previous swapchain was on the same Vulkan device so we can use Vulkan's
+            if (previousInstance != ToBackend(GetDevice())->GetVkInstance()) {
-            // "oldSwapchain" mechanism to ensure a seamless transition. We track the old swapchain
+                return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between instances");
-            // for release immediately so it is not leaked in case of an error. (Vulkan allows
-            // destroying it immediately after the call to vkCreateSwapChainKHR but tracking
-            // using the fenced deleter makes the code simpler).
-            std::swap(previousVulkanSwapChain->mSwapChain, oldVkSwapChain);
-            device->GetFencedDeleter()->DeleteWhenUnused(oldVkSwapChain);
-
-            if (previousSwapChain != this) {
-                previousSwapChain->DetachFromSurface();
             }
+
+            // The previous swapchain is a dawn_native::vulkan::SwapChain so we can reuse its
+            // VkSurfaceKHR provided since they are on the same instance.
+            std::swap(previousVulkanSwapChain->mVkSurface, mVkSurface);
+
+            // The previous swapchain was on the same Vulkan instance so we can use Vulkan's
+            // "oldSwapchain" mechanism to ensure a seamless transition. We track the previous
+            // swapchain for release immediately so it is not leaked in case of an error. (Vulkan
+            // allows destroying it immediately after the call to vkCreateSwapChainKHR but tracking
+            // using the fenced deleter makes the code simpler).
+            std::swap(previousVulkanSwapChain->mSwapChain, previousVkSwapChain);
+            ToBackend(previousSwapChain->GetDevice())
+                ->GetFencedDeleter()
+                ->DeleteWhenUnused(previousVkSwapChain);
         }
 
         if (mVkSurface == VK_NULL_HANDLE) {
@@ -252,7 +255,7 @@ namespace dawn_native { namespace vulkan {
         createInfo.compositeAlpha = VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR;    // TODO
         createInfo.presentMode = mConfig.presentMode;
         createInfo.clipped = false;
-        createInfo.oldSwapchain = oldVkSwapChain;
+        createInfo.oldSwapchain = previousVkSwapChain;
 
         DAWN_TRY(CheckVkSuccess(device->fn.CreateSwapchainKHR(device->GetVkDevice(), &createInfo,
                                                               nullptr, &*mSwapChain),

src/dawn_native/vulkan/VulkanError.cpp

@@ -60,6 +60,12 @@ namespace dawn_native { namespace vulkan {
                 return "VK_ERROR_FORMAT_NOT_SUPPORTED";
             case VK_ERROR_FRAGMENTED_POOL:
                 return "VK_ERROR_FRAGMENTED_POOL";
+
+            case VK_ERROR_SURFACE_LOST_KHR:
+                return "VK_ERROR_SURFACE_LOST_KHR";
+            case VK_ERROR_NATIVE_WINDOW_IN_USE_KHR:
+                return "VK_ERROR_NATIVE_WINDOW_IN_USE_KHR";
+
             case VK_FAKE_DEVICE_OOM_FOR_TESTING:
                 return "VK_FAKE_DEVICE_OOM_FOR_TESTING";
             case VK_FAKE_ERROR_FOR_TESTING:

src/tests/end2end/SwapChainTests.cpp

@@ -136,9 +136,9 @@ TEST_P(SwapChainTests, DestroySurfaceAfterGet) {
 
 // Test switching between present modes.
 TEST_P(SwapChainTests, SwitchPresentMode) {
-    // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and
+    // Fails with "internal drawable creation failed" on the Windows NVIDIA CQ builders but not
-    // makes the test fail.
+    // locally.
-    DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia());
+    DAWN_SKIP_TEST_IF(IsWindows() && IsVulkan() && IsNvidia());
 
     constexpr wgpu::PresentMode kAllPresentModes[] = {
         wgpu::PresentMode::Immediate,
@@ -165,10 +165,6 @@ TEST_P(SwapChainTests, SwitchPresentMode) {
 
 // Test resizing the swapchain and without resizing the window.
 TEST_P(SwapChainTests, ResizingSwapChainOnly) {
-    // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and
-    // makes the test fail.
-    DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia());
-
     for (int i = 0; i < 10; i++) {
         wgpu::SwapChainDescriptor desc = baseDescriptor;
         desc.width += i * 10;
@@ -195,10 +191,6 @@ TEST_P(SwapChainTests, ResizingWindowOnly) {
 
 // Test resizing both the window and the swapchain at the same time.
 TEST_P(SwapChainTests, ResizingWindowAndSwapChain) {
-    // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and
-    // makes the test fail.
-    DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia());
-
     for (int i = 0; i < 10; i++) {
         glfwSetWindowSize(window, 400 - 10 * i, 400 + 10 * i);
         glfwPollEvents();
@@ -219,9 +211,10 @@ TEST_P(SwapChainTests, ResizingWindowAndSwapChain) {
 
 // Test switching devices on the same adapter.
 TEST_P(SwapChainTests, SwitchingDevice) {
-    // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and
+    // The Vulkan Validation Layers incorrectly disallow gracefully passing a swapchain between two
-    // makes the test fail.
+    // VkDevices using "vkSwapchainCreateInfoKHR::oldSwapchain".
-    DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia());
+    // See https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/2256
+    DAWN_SKIP_TEST_IF(IsVulkan() && IsBackendValidationEnabled());
 
     wgpu::Device device2 = wgpu::Device::Acquire(GetAdapter().CreateDevice());
 

src/tests/end2end/SwapChainValidationTests.cpp

@@ -71,24 +71,38 @@ class SwapChainValidationTests : public DawnTest {
 
     // Checks that an OutputAttachment view is an error by trying to create a render pass on it.
     void CheckTextureViewIsError(wgpu::TextureView view) {
-        utils::ComboRenderPassDescriptor renderPassDesc({view});
+        CheckTextureView(view, true, false);
-
-        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc);
-        pass.EndPass();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
-    // Checks that an OutputAttachment view is an error by trying to create a render pass on it.
+    // Checks that an OutputAttachment view is an error by trying to submit a render pass on it.
     void CheckTextureViewIsDestroyed(wgpu::TextureView view) {
+        CheckTextureView(view, false, true);
+    }
+
+    // Checks that an OutputAttachment view is valid by submitting a render pass on it.
+    void CheckTextureViewIsValid(wgpu::TextureView view) {
+        CheckTextureView(view, false, false);
+    }
+
+  private:
+    void CheckTextureView(wgpu::TextureView view, bool errorAtFinish, bool errorAtSubmit) {
         utils::ComboRenderPassDescriptor renderPassDesc({view});
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
         wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc);
         pass.EndPass();
-        wgpu::CommandBuffer commands = encoder.Finish();
 
-        ASSERT_DEVICE_ERROR(queue.Submit(1, &commands));
+        if (errorAtFinish) {
+            ASSERT_DEVICE_ERROR(encoder.Finish());
+        } else {
+            wgpu::CommandBuffer commands = encoder.Finish();
+
+            if (errorAtSubmit) {
+                ASSERT_DEVICE_ERROR(queue.Submit(1, &commands));
+            } else {
+                queue.Submit(1, &commands);
+            }
+        }
     }
 };
 
@@ -181,12 +195,17 @@ TEST_P(SwapChainValidationTests, PresentWithoutCurrentView) {
     ASSERT_DEVICE_ERROR(swapchain.Present());
 }
 
-// Check that the current view is in the destroyed state after the swapchain is destroyed.
+// Check that the current view isn't destroyed when the ref to the swapchain is lost because the
-TEST_P(SwapChainValidationTests, ViewDestroyedAfterSwapChainDestruction) {
+// swapchain is kept alive by the surface. Also check after we lose all refs to the surface, the
+// texture is destroyed.
+TEST_P(SwapChainValidationTests, ViewValidAfterSwapChainRefLost) {
     wgpu::SwapChain swapchain = device.CreateSwapChain(surface, &goodDescriptor);
     wgpu::TextureView view = swapchain.GetCurrentTextureView();
-    swapchain = nullptr;
 
+    swapchain = nullptr;
+    CheckTextureViewIsValid(view);
+
+    surface = nullptr;
     CheckTextureViewIsDestroyed(view);
 }