Fix retention of MVKSwapchain for future drawable presentations.
When presenting drawables in the future, the MVKPresentableSwapchainImage
and MVKSwapchain were both being retained by the drawable-presented callback,
so that the presentation timing info can be recorded on the swapchain.
Unfortunately, in the case where the presentation timing is set far enough
into the future (I'm looking at you CTS), the swapchain, and even the
CAMetalLayer's view may be destroyed, causing occasional bad access crashes.
- MVKSwapchainImage don't retain() the swapchain, and move clearing the
swapchain from the destructor to destroy(), so the MVKSwapchain is not
necessarily retained by the MVKSwapchainImage, for a drawable being
presented in the future.
- MVKSwapchainImage add a lock around clearing swapchain and accessing
it from callbacks.
- Add lockable releaseLayer() function in both MVKSwapchain and MVKSurface,
which is called from both layer observer and destructor, to handle race
conditions better.
- MVKSwapchain::initCAMetalLayer() call MVKSurface::getCAMetalLayer() only once.
- Update MoltenVK version to 1.1.12.
- Update What's New document.
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();
}
