Merge pull request #1683 from billhollings/fix-swapchain-retention
Fix retention of MVKSwapchain for future drawable presentations.
diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md
index a2efc7d..b54e5ab 100644
--- a/Docs/Whats_New.md
+++ b/Docs/Whats_New.md
@@ -13,6 +13,16 @@
+MoltenVK 1.1.12
+--------------
+
+Released TBD
+
+- Fix occassional crash from retention of `MVKSwapchain` for future drawable presentations.
+- Add `MVK_USE_CEREAL` build setting to avoid use of Cereal external library (for pipeline caching).
+
+
+
MoltenVK 1.1.11
--------------
diff --git a/MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h b/MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h
index 70d6030..080bb02 100644
--- a/MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h
+++ b/MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h
@@ -51,7 +51,7 @@
*/
#define MVK_VERSION_MAJOR 1
#define MVK_VERSION_MINOR 1
-#define MVK_VERSION_PATCH 11
+#define MVK_VERSION_PATCH 12
#define MVK_MAKE_VERSION(major, minor, patch) (((major) * 10000) + ((minor) * 100) + (patch))
#define MVK_VERSION MVK_MAKE_VERSION(MVK_VERSION_MAJOR, MVK_VERSION_MINOR, MVK_VERSION_PATCH)
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h
index b56eeb0..fc9e5fd 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h
@@ -393,20 +393,21 @@
#pragma mark Construction
- /** Constructs an instance for the specified device and swapchain. */
MVKSwapchainImage(MVKDevice* device,
const VkImageCreateInfo* pCreateInfo,
MVKSwapchain* swapchain,
uint32_t swapchainIndex);
- ~MVKSwapchainImage() override;
+ void destroy() override;
protected:
friend class MVKPeerSwapchainImage;
virtual id<CAMetalDrawable> getCAMetalDrawable() = 0;
+ void detachSwapchain();
MVKSwapchain* _swapchain;
+ std::mutex _swapchainLock;
uint32_t _swapchainIndex;
};
@@ -452,7 +453,6 @@
#pragma mark Construction
- /** Constructs an instance for the specified device and swapchain. */
MVKPresentableSwapchainImage(MVKDevice* device,
const VkImageCreateInfo* pCreateInfo,
MVKSwapchain* swapchain,
@@ -464,7 +464,7 @@
friend MVKSwapchain;
id<CAMetalDrawable> getCAMetalDrawable() override;
- void presentCAMetalDrawable(id<CAMetalDrawable> mtlDrawable, MVKPresentTimingInfo presentTimingInfo);
+ void addPresentedHandler(id<CAMetalDrawable> mtlDrawable, MVKPresentTimingInfo presentTimingInfo);
void releaseMetalDrawable();
MVKSwapchainImageAvailability getAvailability();
void makeAvailable(const MVKSwapchainSignaler& signaler);
@@ -498,7 +498,6 @@
#pragma mark Construction
- /** Constructs an instance for the specified device and swapchain. */
MVKPeerSwapchainImage(MVKDevice* device,
const VkImageCreateInfo* pCreateInfo,
MVKSwapchain* swapchain,
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm
index 8340095..dc85fd4 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm
@@ -1190,12 +1190,16 @@
uint32_t swapchainIndex) : MVKImage(device, pCreateInfo) {
_swapchain = swapchain;
_swapchainIndex = swapchainIndex;
-
- _swapchain->retain();
}
-MVKSwapchainImage::~MVKSwapchainImage() {
- if (_swapchain) { _swapchain->release(); }
+void MVKSwapchainImage::detachSwapchain() {
+ lock_guard<mutex> lock(_swapchainLock);
+ _swapchain = nullptr;
+}
+
+void MVKSwapchainImage::destroy() {
+ detachSwapchain();
+ MVKImage::destroy();
}
@@ -1308,10 +1312,19 @@
_swapchain->willPresentSurface(getMTLTexture(0), mtlCmdBuff);
- // Get current drawable now. Don't retrieve in handler, because a new drawable might be acquired by then.
+ // According to Apple, it is more performant to call MTLDrawable present from within a
+ // MTLCommandBuffer scheduled-handler than it is to call MTLCommandBuffer presentDrawable:.
+ // But get current drawable now, intead of in handler, because a new drawable might be acquired by then.
+ // Attach present handler before presenting to avoid race condition.
id<CAMetalDrawable> mtlDrwbl = getCAMetalDrawable();
[mtlCmdBuff addScheduledHandler: ^(id<MTLCommandBuffer> mcb) {
- presentCAMetalDrawable(mtlDrwbl, presentTimingInfo);
+ if (presentTimingInfo.hasPresentTime) {
+ // Convert from nsecs to seconds for Metal
+ addPresentedHandler(mtlDrwbl, presentTimingInfo);
+ [mtlDrwbl presentAtTime: (double)presentTimingInfo.desiredPresentTime * 1.0e-9];
+ } else {
+ [mtlDrwbl present];
+ }
}];
MVKSwapchainSignaler signaler;
@@ -1342,33 +1355,29 @@
signalPresentationSemaphore(signaler, mtlCmdBuff);
}
-void MVKPresentableSwapchainImage::presentCAMetalDrawable(id<CAMetalDrawable> mtlDrawable,
- MVKPresentTimingInfo presentTimingInfo) {
-
- if (presentTimingInfo.hasPresentTime) {
-
- // Attach present handler before presenting to avoid race condition.
- // If MTLDrawable.presentedTime/addPresentedHandler isn't supported,
- // treat it as if the present happened when requested.
-#if MVK_OS_SIMULATOR
- _swapchain->recordPresentTime(presentTimingInfo);
-#else
- if ([mtlDrawable respondsToSelector: @selector(addPresentedHandler:)]) {
- // Ensure this image is not destroyed while awaiting presentation
- retain();
- [mtlDrawable addPresentedHandler: ^(id<MTLDrawable> drawable) {
- _swapchain->recordPresentTime(presentTimingInfo, drawable.presentedTime * 1.0e9);
- release();
- }];
- } else {
- _swapchain->recordPresentTime(presentTimingInfo);
- }
-#endif
- // Convert from nsecs to seconds for Metal
- [mtlDrawable presentAtTime: (double)presentTimingInfo.desiredPresentTime * 1.0e-9];
- } else {
- [mtlDrawable present];
+void MVKPresentableSwapchainImage::addPresentedHandler(id<CAMetalDrawable> mtlDrawable,
+ MVKPresentTimingInfo presentTimingInfo) {
+#if !MVK_OS_SIMULATOR
+ if ([mtlDrawable respondsToSelector: @selector(addPresentedHandler:)]) {
+ retain(); // Ensure this image is not destroyed while awaiting presentation
+ [mtlDrawable addPresentedHandler: ^(id<MTLDrawable> drawable) {
+ // Since we're in a callback, it's possible that the swapchain has been released by now.
+ // Lock the swapchain, and test if it is present before doing anything with it.
+ lock_guard<mutex> cblock(_swapchainLock);
+ if (_swapchain) { _swapchain->recordPresentTime(presentTimingInfo, drawable.presentedTime * 1.0e9); }
+ release();
+ }];
+ return;
}
+#endif
+
+ // If MTLDrawable.presentedTime/addPresentedHandler isn't supported,
+ // treat it as if the present happened when requested.
+ // Since this function may be called in a callback, it's possible that
+ // the swapchain has been released by the time this function runs.
+ // Lock the swapchain, and test if it is present before doing anything with it.
+ lock_guard<mutex> lock(_swapchainLock);
+ if (_swapchain) {_swapchain->recordPresentTime(presentTimingInfo); }
}
// Resets the MTLTexture and CAMetalDrawable underlying this image.
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSurface.h b/MoltenVK/MoltenVK/GPUObjects/MVKSurface.h
index bcb69fd..8ec3eb1 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKSurface.h
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKSurface.h
@@ -58,7 +58,7 @@
/** Returns the CAMetalLayer underlying this surface. */
inline CAMetalLayer* getCAMetalLayer() {
- std::lock_guard<std::mutex> lock(_lock);
+ std::lock_guard<std::mutex> lock(_layerLock);
return _mtlCAMetalLayer;
}
@@ -78,10 +78,11 @@
protected:
void propagateDebugName() override {}
void initLayerObserver();
+ void releaseLayer();
MVKInstance* _mvkInstance;
CAMetalLayer* _mtlCAMetalLayer;
- std::mutex _lock;
MVKBlockObserver* _layerObserver;
+ std::mutex _layerLock;
};
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSurface.mm b/MoltenVK/MoltenVK/GPUObjects/MVKSurface.mm
index a70537c..a355177 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKSurface.mm
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKSurface.mm
@@ -77,18 +77,20 @@
_layerObserver = [MVKBlockObserver observerWithBlock: ^(NSString* path, id, NSDictionary*, void*) {
if ( ![path isEqualToString: @"layer"] ) { return; }
- std::lock_guard<std::mutex> lock(this->_lock);
- [this->_mtlCAMetalLayer release];
- this->_mtlCAMetalLayer = nil;
- this->setConfigurationResult(VK_ERROR_SURFACE_LOST_KHR);
- [this->_layerObserver release];
- this->_layerObserver = nil;
+ this->releaseLayer();
} forObject: _mtlCAMetalLayer.delegate atKeyPath: @"layer"];
}
-MVKSurface::~MVKSurface() {
- std::lock_guard<std::mutex> lock(_lock);
+void MVKSurface::releaseLayer() {
+ std::lock_guard<std::mutex> lock(_layerLock);
+ setConfigurationResult(VK_ERROR_SURFACE_LOST_KHR);
[_mtlCAMetalLayer release];
+ _mtlCAMetalLayer = nil;
[_layerObserver release];
+ _layerObserver = nil;
+}
+
+MVKSurface::~MVKSurface() {
+ releaseLayer();
}
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.h b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.h
index 7521e65..76ae1ce 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.h
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.h
@@ -21,6 +21,7 @@
#include "MVKDevice.h"
#include "MVKImage.h"
#include "MVKSmallVector.h"
+#include <mutex>
#import "CAMetalLayer+MoltenVK.h"
#import <Metal/Metal.h>
@@ -111,6 +112,7 @@
void propagateDebugName() override;
void initCAMetalLayer(const VkSwapchainCreateInfoKHR* pCreateInfo, uint32_t imgCnt);
void initSurfaceImages(const VkSwapchainCreateInfoKHR* pCreateInfo, uint32_t imgCnt);
+ void releaseLayer();
void releaseUndisplayedSurfaces();
uint64_t getNextAcquisitionID();
void willPresentSurface(id<MTLTexture> mtlTexture, id<MTLCommandBuffer> mtlCmdBuff);
@@ -126,6 +128,7 @@
uint32_t _currentPerfLogFrameCount;
std::atomic<bool> _surfaceLost;
MVKBlockObserver* _layerObserver;
+ std::mutex _layerLock;
static const int kMaxPresentationHistory = 60;
VkPastPresentationTimingGOOGLE _presentTimingHistory[kMaxPresentationHistory];
uint32_t _presentHistoryCount;
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm
index d614790..781f72b 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm
@@ -255,13 +255,13 @@
void MVKSwapchain::initCAMetalLayer(const VkSwapchainCreateInfoKHR* pCreateInfo, uint32_t imgCnt) {
MVKSurface* mvkSrfc = (MVKSurface*)pCreateInfo->surface;
- if ( !mvkSrfc->getCAMetalLayer() ) {
+ _mtlLayer = mvkSrfc->getCAMetalLayer();
+ if ( !_mtlLayer ) {
setConfigurationResult(mvkSrfc->getConfigurationResult());
_surfaceLost = true;
return;
}
- _mtlLayer = mvkSrfc->getCAMetalLayer();
_mtlLayer.device = getMTLDevice();
_mtlLayer.pixelFormat = getPixelFormats()->getMTLPixelFormat(pCreateInfo->imageFormat);
_mtlLayer.maximumDrawableCountMVK = imgCnt;
@@ -341,13 +341,18 @@
// needs to recreate the swapchain, or no content will be displayed.
_layerObserver = [MVKBlockObserver observerWithBlock: ^(NSString* path, id, NSDictionary*, void*) {
if ( ![path isEqualToString: @"layer"] ) { return; }
- this->_surfaceLost = true;
- [this->_layerObserver release];
- this->_layerObserver = nil;
+ this->releaseLayer();
} forObject: _mtlLayer.delegate atKeyPath: @"layer"];
}
}
+void MVKSwapchain::releaseLayer() {
+ std::lock_guard<std::mutex> lock(_layerLock);
+ _surfaceLost = true;
+ [_layerObserver release];
+ _layerObserver = nil;
+}
+
// Initializes the array of images used for the surface of this swapchain.
// The CAMetalLayer should already be initialized when this is called.
void MVKSwapchain::initSurfaceImages(const VkSwapchainCreateInfoKHR* pCreateInfo, uint32_t imgCnt) {
@@ -494,6 +499,6 @@
MVKSwapchain::~MVKSwapchain() {
if (_licenseWatermark) { _licenseWatermark->destroy(); }
- [this->_layerObserver release];
+ releaseLayer();
}
