Merge pull request #652 from billhollings/master

Fix race condition between swapchain image destruction and presentation completion callback

Docs/Whats_New.md

@@ -27,7 +27,8 @@ Released TBD
 - Skip `SPIRV-Tools` build in Travis because Travis does not support the required Python 3.
 - Separate `SPIRVToMSLConverterContext` into input config and output results.
 - Fix pipeline cache lookups.
-- Doument that the functions in `vk_mvk_moltenvk.h` cannot be used with objects 
+- Fix race condition between swapchain image destruction and presentation completion callback.
+- Document that the functions in `vk_mvk_moltenvk.h` cannot be used with objects 
   retrieved through the *Vulkan SDK Loader and Layers* framework.
 - Update `VK_MVK_MOLTENVK_SPEC_VERSION` to 21.
 - Update to latest SPIRV-Cross version:

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

@@ -1323,7 +1323,12 @@ void MVKSwapchainImage::presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff)
             // Signal the semaphore device-side.
             _availabilitySignalers.front().first->encodeSignal(mtlCmdBuff);
         }
-        [mtlCmdBuff addCompletedHandler: ^(id<MTLCommandBuffer> mcb) { makeAvailable(); }];
+
+		retain();	// Ensure this image is not destroyed while awaiting MTLCommandBuffer completion
+        [mtlCmdBuff addCompletedHandler: ^(id<MTLCommandBuffer> mcb) {
+			makeAvailable();
+			release();
+		}];
     } else {
         [mtlDrawable present];
         resetMetalSurface();

MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.h

@@ -21,7 +21,7 @@
 #include "MVKBaseObject.h"
 #include <vulkan/vk_icd.h>
 #include <string>
-#include <mutex>
+#include <atomic>
 
 #import <Foundation/NSString.h>
 
@@ -93,25 +93,18 @@ public:
 	static MVKVulkanAPIObject* getMVKVulkanAPIObject(VkObjectType objType, uint64_t objectHandle);
 
 	/** Construct an empty instance. Declared here to support copy constructor. */
-	MVKVulkanAPIObject() {}
+	MVKVulkanAPIObject() : _refCount(1) {}
 
-	/**
-	 * Construct an instance from a copy. Default copy constructor disallowed due to mutex.
-	 * Copies start with fresh reference counts.
-	 */
-	MVKVulkanAPIObject(const MVKVulkanAPIObject& other) {}
+	/** Default copy constructor disallowed due to mutex. Copy starts with fresh reference counts. */
+	MVKVulkanAPIObject(const MVKVulkanAPIObject& other) : _refCount(1) {}
 
 	~MVKVulkanAPIObject() override;
 
 protected:
-	bool decrementRetainCount();
-	bool markDestroyed();
 	virtual void propogateDebugName() = 0;
 
+	std::atomic<uint32_t> _refCount;
 	NSString* _debugName = nil;
-	std::mutex _refLock;
-	unsigned _refCount = 0;
-	bool _isDestroyed = false;
 };
 
 

MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm

@@ -25,33 +25,15 @@ using namespace std;
 #pragma mark MVKVulkanAPIObject
 
 void MVKVulkanAPIObject::retain() {
-	lock_guard<mutex> lock(_refLock);
-
 	_refCount++;
 }
 
 void MVKVulkanAPIObject::release() {
-	if (decrementRetainCount()) { destroy(); }
+	if (--_refCount == 0) { MVKConfigurableObject::destroy(); }
 }
 
 void MVKVulkanAPIObject::destroy() {
-	if (markDestroyed()) { MVKConfigurableObject::destroy(); }
-}
-
-// Decrements the reference count, and returns whether it's time to destroy this object.
-bool MVKVulkanAPIObject::decrementRetainCount() {
-	lock_guard<mutex> lock(_refLock);
-
-	if (_refCount > 0) { _refCount--; }
-	return (_isDestroyed && _refCount == 0);
-}
-
-// Marks this object as destroyed, and returns whether no references are left outstanding.
-bool MVKVulkanAPIObject::markDestroyed() {
-	lock_guard<mutex> lock(_refLock);
-
-	_isDestroyed = true;
-	return _refCount == 0;
+	release();
 }
 
 VkResult MVKVulkanAPIObject::setDebugName(const char* pObjectName) {