Merge "Fix crash when VulkanSurface is no longer valid"

5 files changed, 55 insertions, 73 deletions

diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
index d0fe022616c5..15f53f286261 100644
--- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
+++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
@@ -42,7 +42,13 @@ namespace uirenderer {
 namespace skiapipeline {
 
 SkiaVulkanPipeline::SkiaVulkanPipeline(renderthread::RenderThread& thread)
-        : SkiaPipeline(thread), mVkManager(thread.vulkanManager()) {}
+        : SkiaPipeline(thread), mVkManager(thread.vulkanManager()) {
+    thread.renderState().registerContextCallback(this);
+}
+
+SkiaVulkanPipeline::~SkiaVulkanPipeline() {
+    mRenderThread.renderState().removeContextCallback(this);
+}
 
 MakeCurrentResult SkiaVulkanPipeline::makeCurrent() {
     return MakeCurrentResult::AlreadyCurrent;
@@ -53,6 +59,8 @@ Frame SkiaVulkanPipeline::getFrame() {
                         "drawRenderNode called on a context with no surface!");
 
     SkSurface* backBuffer = mVkManager.getBackbufferSurface(&mVkSurface);
+    LOG_ALWAYS_FATAL_IF(mVkSurface == nullptr,
+                        "drawRenderNode called on a context with an invalid surface");
     if (backBuffer == nullptr) {
         SkDebugf("failed to get backbuffer");
         return Frame(-1, -1, 0);
@@ -153,6 +161,13 @@ sk_sp<Bitmap> SkiaVulkanPipeline::allocateHardwareBitmap(renderthread::RenderThr
     return nullptr;
 }
 
+void SkiaVulkanPipeline::onContextDestroyed() {
+    if (mVkSurface) {
+        mVkManager.destroySurface(mVkSurface);
+        mVkSurface = nullptr;
+    }
+}
+
 } /* namespace skiapipeline */
 } /* namespace uirenderer */
 } /* namespace android */
diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
index 934307636913..2c24edddb7e0 100644
--- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
+++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
@@ -19,14 +19,16 @@
 #include "SkiaPipeline.h"
 #include "renderthread/VulkanManager.h"
 
+#include "renderstate/RenderState.h"
+
 namespace android {
 namespace uirenderer {
 namespace skiapipeline {
 
-class SkiaVulkanPipeline : public SkiaPipeline {
+class SkiaVulkanPipeline : public SkiaPipeline, public IGpuContextCallback {
 public:
     explicit SkiaVulkanPipeline(renderthread::RenderThread& thread);
-    virtual ~SkiaVulkanPipeline() {}
+    virtual ~SkiaVulkanPipeline();
 
     renderthread::MakeCurrentResult makeCurrent() override;
     renderthread::Frame getFrame() override;
@@ -49,6 +51,9 @@ public:
     static sk_sp<Bitmap> allocateHardwareBitmap(renderthread::RenderThread& thread,
                                                 SkBitmap& skBitmap);
 
+protected:
+    void onContextDestroyed() override;
+
 private:
     renderthread::VulkanManager& mVkManager;
     renderthread::VulkanSurface* mVkSurface = nullptr;
diff --git a/libs/hwui/renderthread/RenderThread.cpp b/libs/hwui/renderthread/RenderThread.cpp
index 8bef35915c4d..3b37c83a6acc 100644
--- a/libs/hwui/renderthread/RenderThread.cpp
+++ b/libs/hwui/renderthread/RenderThread.cpp
@@ -203,11 +203,17 @@ void RenderThread::requireGlContext() {
 
 void RenderThread::destroyRenderingContext() {
     mFunctorManager.onContextDestroyed();
-    if (mEglManager->hasEglContext()) {
-        setGrContext(nullptr);
-        mEglManager->destroy();
+    if (Properties::getRenderPipelineType() == RenderPipelineType::SkiaGL) {
+        if (mEglManager->hasEglContext()) {
+            setGrContext(nullptr);
+            mEglManager->destroy();
+        }
+    } else {
+        if (vulkanManager().hasVkContext()) {
+            setGrContext(nullptr);
+            vulkanManager().destroy();
+        }
     }
-    vulkanManager().destroy();
 }
 
 void RenderThread::dumpGraphicsMemory(int fd) {
diff --git a/libs/hwui/renderthread/VulkanManager.cpp b/libs/hwui/renderthread/VulkanManager.cpp
index 582d51e6af94..90397fddf618 100644
--- a/libs/hwui/renderthread/VulkanManager.cpp
+++ b/libs/hwui/renderthread/VulkanManager.cpp
@@ -89,7 +89,7 @@ void VulkanManager::destroy() {
     mPhysicalDeviceFeatures2 = {};
 }
 
-bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFeatures2& features) {
+void VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFeatures2& features) {
     VkResult err;
 
     constexpr VkApplicationInfo app_info = {
@@ -107,15 +107,11 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
 
         uint32_t extensionCount = 0;
         err = mEnumerateInstanceExtensionProperties(nullptr, &extensionCount, nullptr);
-        if (VK_SUCCESS != err) {
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(VK_SUCCESS != err);
         std::unique_ptr<VkExtensionProperties[]> extensions(
                 new VkExtensionProperties[extensionCount]);
         err = mEnumerateInstanceExtensionProperties(nullptr, &extensionCount, extensions.get());
-        if (VK_SUCCESS != err) {
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(VK_SUCCESS != err);
         bool hasKHRSurfaceExtension = false;
         bool hasKHRAndroidSurfaceExtension = false;
         for (uint32_t i = 0; i < extensionCount; ++i) {
@@ -127,10 +123,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
                 hasKHRAndroidSurfaceExtension = true;
             }
         }
-        if (!hasKHRSurfaceExtension || !hasKHRAndroidSurfaceExtension) {
-            this->destroy();
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(!hasKHRSurfaceExtension || !hasKHRAndroidSurfaceExtension);
     }
 
     const VkInstanceCreateInfo instance_create = {
@@ -146,10 +139,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
 
     GET_PROC(CreateInstance);
     err = mCreateInstance(&instance_create, nullptr, &mInstance);
-    if (err < 0) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(err < 0);
 
     GET_INST_PROC(DestroyInstance);
     GET_INST_PROC(EnumeratePhysicalDevices);
@@ -166,39 +156,23 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
     GET_INST_PROC(GetPhysicalDeviceSurfacePresentModesKHR);
 
     uint32_t gpuCount;
-    err = mEnumeratePhysicalDevices(mInstance, &gpuCount, nullptr);
-    if (err) {
-        this->destroy();
-        return false;
-    }
-    if (!gpuCount) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(mEnumeratePhysicalDevices(mInstance, &gpuCount, nullptr));
+    LOG_ALWAYS_FATAL_IF(!gpuCount);
     // Just returning the first physical device instead of getting the whole array. Since there
     // should only be one device on android.
     gpuCount = 1;
     err = mEnumeratePhysicalDevices(mInstance, &gpuCount, &mPhysicalDevice);
     // VK_INCOMPLETE is returned when the count we provide is less than the total device count.
-    if (err && VK_INCOMPLETE != err) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(err && VK_INCOMPLETE != err);
 
     VkPhysicalDeviceProperties physDeviceProperties;
     mGetPhysicalDeviceProperties(mPhysicalDevice, &physDeviceProperties);
-    if (physDeviceProperties.apiVersion < VK_MAKE_VERSION(1, 1, 0)) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(physDeviceProperties.apiVersion < VK_MAKE_VERSION(1, 1, 0));
 
     // query to get the initial queue props size
     uint32_t queueCount;
     mGetPhysicalDeviceQueueFamilyProperties(mPhysicalDevice, &queueCount, nullptr);
-    if (!queueCount) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(!queueCount);
 
     // now get the actual queue props
     std::unique_ptr<VkQueueFamilyProperties[]> queueProps(new VkQueueFamilyProperties[queueCount]);
@@ -212,10 +186,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
             break;
         }
     }
-    if (mGraphicsQueueIndex == queueCount) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(mGraphicsQueueIndex == queueCount);
 
     // All physical devices and queue families on Android must be capable of
     // presentation with any native window. So just use the first one.
@@ -225,18 +196,12 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
         uint32_t extensionCount = 0;
         err = mEnumerateDeviceExtensionProperties(mPhysicalDevice, nullptr, &extensionCount,
                 nullptr);
-        if (VK_SUCCESS != err) {
-            this->destroy();
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(VK_SUCCESS != err);
         std::unique_ptr<VkExtensionProperties[]> extensions(
                 new VkExtensionProperties[extensionCount]);
         err = mEnumerateDeviceExtensionProperties(mPhysicalDevice, nullptr, &extensionCount,
                 extensions.get());
-        if (VK_SUCCESS != err) {
-            this->destroy();
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(VK_SUCCESS != err);
         bool hasKHRSwapchainExtension = false;
         for (uint32_t i = 0; i < extensionCount; ++i) {
             mDeviceExtensions.push_back(extensions[i].extensionName);
@@ -244,10 +209,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
                 hasKHRSwapchainExtension = true;
             }
         }
-        if (!hasKHRSwapchainExtension) {
-            this->destroy();
-            return false;
-        }
+        LOG_ALWAYS_FATAL_IF(!hasKHRSwapchainExtension);
     }
 
     auto getProc = [] (const char* proc_name, VkInstance instance, VkDevice device) {
@@ -259,10 +221,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
     grExtensions.init(getProc, mInstance, mPhysicalDevice, mInstanceExtensions.size(),
             mInstanceExtensions.data(), mDeviceExtensions.size(), mDeviceExtensions.data());
 
-    if (!grExtensions.hasExtension(VK_KHR_EXTERNAL_SEMAPHORE_FD_EXTENSION_NAME, 1)) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(!grExtensions.hasExtension(VK_KHR_EXTERNAL_SEMAPHORE_FD_EXTENSION_NAME, 1));
 
     memset(&features, 0, sizeof(VkPhysicalDeviceFeatures2));
     features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2;
@@ -332,11 +291,7 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
         nullptr,                                 // ppEnabledFeatures
     };
 
-    err = mCreateDevice(mPhysicalDevice, &deviceInfo, nullptr, &mDevice);
-    if (err) {
-        this->destroy();
-        return false;
-    }
+    LOG_ALWAYS_FATAL_IF(mCreateDevice(mPhysicalDevice, &deviceInfo, nullptr, &mDevice));
 
     GET_DEV_PROC(GetDeviceQueue);
     GET_DEV_PROC(DeviceWaitIdle);
@@ -366,8 +321,6 @@ bool VulkanManager::setupDevice(GrVkExtensions& grExtensions, VkPhysicalDeviceFe
     GET_DEV_PROC(DestroyFence);
     GET_DEV_PROC(WaitForFences);
     GET_DEV_PROC(ResetFences);
-
-    return true;
 }
 
 void VulkanManager::initialize() {
@@ -381,7 +334,7 @@ void VulkanManager::initialize() {
     LOG_ALWAYS_FATAL_IF(instanceVersion < VK_MAKE_VERSION(1, 1, 0));
 
     GrVkExtensions extensions;
-    LOG_ALWAYS_FATAL_IF(!this->setupDevice(extensions, mPhysicalDeviceFeatures2));
+    this->setupDevice(extensions, mPhysicalDeviceFeatures2);
 
     mGetDeviceQueue(mDevice, mGraphicsQueueIndex, 0, &mGraphicsQueue);
 
@@ -419,7 +372,7 @@ void VulkanManager::initialize() {
 
     if (!setupDummyCommandBuffer()) {
         this->destroy();
-        return;
+        // Pass through will crash on next line.
     }
     LOG_ALWAYS_FATAL_IF(mDummyCB == VK_NULL_HANDLE);
 
@@ -520,6 +473,9 @@ SkSurface* VulkanManager::getBackbufferSurface(VulkanSurface** surfaceOut) {
         destroySurface(surface);
         *surfaceOut = createSurface(window, colorMode, colorSpace, colorType);
         surface = *surfaceOut;
+        if (!surface) {
+            return nullptr;
+        }
     }
 
     VulkanSurface::BackbufferInfo* backbuffer = getAvailableBackbuffer(surface);
diff --git a/libs/hwui/renderthread/VulkanManager.h b/libs/hwui/renderthread/VulkanManager.h
index 6426fe2808b7..1fe6c65b35b8 100644
--- a/libs/hwui/renderthread/VulkanManager.h
+++ b/libs/hwui/renderthread/VulkanManager.h
@@ -151,7 +151,7 @@ private:
 
     // Sets up the VkInstance and VkDevice objects. Also fills out the passed in
     // VkPhysicalDeviceFeatures struct.
-    bool setupDevice(GrVkExtensions&, VkPhysicalDeviceFeatures2&);
+    void setupDevice(GrVkExtensions&, VkPhysicalDeviceFeatures2&);
 
     void destroyBuffers(VulkanSurface* surface);