Merge pull request #2012 from billhollings/fix-drwbl-invalid-format

Guard against CAMetalDrawable with invalid pixel format.

Common/MVKOSExtensions.h

@@ -39,27 +39,30 @@ static const MVKOSVersion kMVKOSVersionUnsupported = std::numeric_limits<MVKOSVe
 MVKOSVersion mvkOSVersion();
 
 /** Returns a MVKOSVersion built from the version components. */
-inline MVKOSVersion mvkMakeOSVersion(uint32_t major, uint32_t minor, uint32_t patch) {
+static inline MVKOSVersion mvkMakeOSVersion(uint32_t major, uint32_t minor, uint32_t patch) {
 	return (float)major + ((float)minor / 100.0f) + ((float)patch / 10000.0f);
 }
 
 /** Returns whether the operating system version is at least minVer. */
-inline bool mvkOSVersionIsAtLeast(MVKOSVersion minVer) { return mvkOSVersion() >= minVer; }
+static inline bool mvkOSVersionIsAtLeast(MVKOSVersion minVer) { return mvkOSVersion() >= minVer; }
 
 /**
  * Returns whether the operating system version is at least the appropriate min version.
- * The constant kMVKOSVersionUnsupported can be used for either value to cause the test
- * to always fail on that OS, which is useful for indidicating functionalty guarded by
+ * The constant kMVKOSVersionUnsupported can be used for any of the values to cause the test
+ * to always fail on that OS, which is useful for indicating that functionalty guarded by
  * this test is not supported on that OS.
  */
-inline bool mvkOSVersionIsAtLeast(MVKOSVersion macOSMinVer, MVKOSVersion iOSMinVer, MVKOSVersion visionOSMinVer) {
+static inline bool mvkOSVersionIsAtLeast(MVKOSVersion macOSMinVer,
+										 MVKOSVersion iOSMinVer,
+										 MVKOSVersion visionOSMinVer) {
 #if MVK_MACOS
 	return mvkOSVersionIsAtLeast(macOSMinVer);
 #endif
+#if MVK_IOS_OR_TVOS
+	return mvkOSVersionIsAtLeast(iOSMinVer);
+#endif
 #if MVK_VISIONOS
 	return mvkOSVersionIsAtLeast(visionOSMinVer);
-#elif MVK_IOS_OR_TVOS
-	return mvkOSVersionIsAtLeast(iOSMinVer);
 #endif
 }
 

Docs/Whats_New.md

@@ -20,6 +20,7 @@ Released TBD
 
 - Fix rare case where vertex attribute buffers are not bound to Metal 
   when no other bindings change between pipelines.
+- Fix case where a `CAMetalDrawable` with invalid pixel format causes onscreen flickering.
 - Improve behavior of swapchain image presentation stalls caused by Metal regression.
 - Add several additional performance trackers, available via logging, or the `mvk_private_api.h` API.
 - Update `MVK_CONFIGURATION_API_VERSION` and `MVK_PRIVATE_API_VERSION` to `38`.

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

@@ -454,7 +454,7 @@ public:
 #pragma mark Metal
 
 	/** Presents the contained drawable to the OS. */
-	void presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff, MVKImagePresentInfo presentInfo);
+	VkResult presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff, MVKImagePresentInfo presentInfo);
 
 	/** Called when the presentation begins. */
 	void beginPresentation(const MVKImagePresentInfo& presentInfo);

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

@@ -1258,7 +1258,6 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor
 	// This is not done earlier so the texture is retained for any post-processing such as screen captures, etc.
 	releaseMetalDrawable();
 
-	VkResult rslt = VK_SUCCESS;
 	auto signaler = MVKSwapchainSignaler{fence, semaphore, semaphore ? semaphore->deferSignal() : 0};
 	if (_availability.isAvailable) {
 		_availability.isAvailable = false;
@@ -1271,7 +1270,7 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor
 			id<MTLCommandBuffer> mtlCmdBuff = nil;
 			if (mvkSem && mvkSem->isUsingCommandEncoding()) {
 				mtlCmdBuff = _device->getAnyQueue()->getMTLCommandBuffer(kMVKCommandUseAcquireNextImage);
-				if ( !mtlCmdBuff ) { rslt = VK_ERROR_OUT_OF_POOL_MEMORY; }
+				if ( !mtlCmdBuff ) { setConfigurationResult(VK_ERROR_OUT_OF_POOL_MEMORY); }
 			}
 			signal(signaler, mtlCmdBuff);
 			[mtlCmdBuff commit];
@@ -1283,19 +1282,29 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor
 	}
 	markAsTracked(signaler);
 
-	return rslt;
+	return getConfigurationResult();
 }
 
+// Calling nextDrawable may result in a nil drawable, or a drawable with no pixel format.
+// Attempt several times to retrieve a good drawable, and set an error to trigger the
+// swapchain to be re-established if one cannot be retrieved.
 id<CAMetalDrawable> MVKPresentableSwapchainImage::getCAMetalDrawable() {
 	if ( !_mtlDrawable ) {
 		@autoreleasepool {
+			bool hasInvalidFormat = false;
 			uint32_t attemptCnt = _swapchain->getImageCount() * 2;	// Attempt a resonable number of times
 			for (uint32_t attemptIdx = 0; !_mtlDrawable && attemptIdx < attemptCnt; attemptIdx++) {
 				uint64_t startTime = _device->getPerformanceTimestamp();
 				_mtlDrawable = [_swapchain->_surface->getCAMetalLayer().nextDrawable retain];	// retained
 				_device->addPerformanceInterval(_device->_performanceStatistics.queue.retrieveCAMetalDrawable, startTime);
+				hasInvalidFormat = _mtlDrawable && !_mtlDrawable.texture.pixelFormat;
+				if (hasInvalidFormat) { releaseMetalDrawable(); }
+			}
+			if (hasInvalidFormat) {
+				setConfigurationResult(reportError(VK_ERROR_OUT_OF_DATE_KHR, "CAMetalDrawable with valid format could not be acquired after %d attempts.", attemptCnt));
+			} else if ( !_mtlDrawable ) {
+				setConfigurationResult(reportError(VK_ERROR_OUT_OF_POOL_MEMORY, "CAMetalDrawable could not be acquired after %d attempts.", attemptCnt));
 			}
-			if ( !_mtlDrawable ) { reportError(VK_ERROR_OUT_OF_POOL_MEMORY, "CAMetalDrawable could not be acquired after %d attempts.", attemptCnt); }
 		}
 	}
 	return _mtlDrawable;
@@ -1303,8 +1312,8 @@ id<CAMetalDrawable> MVKPresentableSwapchainImage::getCAMetalDrawable() {
 
 // Present the drawable and make myself available only once the command buffer has completed.
 // Pass MVKImagePresentInfo by value because it may not exist when the callback runs.
-void MVKPresentableSwapchainImage::presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff,
-														  MVKImagePresentInfo presentInfo) {
+VkResult MVKPresentableSwapchainImage::presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff,
+															  MVKImagePresentInfo presentInfo) {
 	lock_guard<mutex> lock(_availabilityLock);
 
 	_swapchain->renderWatermark(getMTLTexture(0), mtlCmdBuff);
@@ -1363,6 +1372,8 @@ void MVKPresentableSwapchainImage::presentCAMetalDrawable(id<MTLCommandBuffer> m
 	}];
 
 	signalPresentationSemaphore(signaler, mtlCmdBuff);
+
+	return getConfigurationResult();
 }
 
 // Pass MVKImagePresentInfo by value because it may not exist when the callback runs.

MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm

@@ -623,12 +623,6 @@ MVKQueueFullCommandBufferSubmission<N>::MVKQueueFullCommandBufferSubmission(MVKQ
 // The semaphores know what to do.
 VkResult MVKQueuePresentSurfaceSubmission::execute() {
 	id<MTLCommandBuffer> mtlCmdBuff = _queue->getMTLCommandBuffer(kMVKCommandUseQueuePresent);
-	[mtlCmdBuff enqueue];
-
-	// Add completion handler that will destroy this submission only once the MTLCommandBuffer
-	// is finished with the resources retained here, including the wait semaphores.
-	// Completion handlers are also added in presentCAMetalDrawable() to retain the swapchain images.
-	[mtlCmdBuff addCompletedHandler: ^(id<MTLCommandBuffer> mtlCB) { this->finish(); }];
 
 	for (auto& ws : _waitSemaphores) {
 		auto& sem4 = ws.first;
@@ -637,15 +631,22 @@ VkResult MVKQueuePresentSurfaceSubmission::execute() {
 	}
 
 	for (int i = 0; i < _presentInfo.size(); i++ ) {
-		_presentInfo[i].presentableImage->presentCAMetalDrawable(mtlCmdBuff, _presentInfo[i]);
+		setConfigurationResult(_presentInfo[i].presentableImage->presentCAMetalDrawable(mtlCmdBuff, _presentInfo[i]));
 	}
 
-	[mtlCmdBuff commit];
+	if ( !mtlCmdBuff ) { setConfigurationResult(VK_ERROR_OUT_OF_POOL_MEMORY); }	// Check after images may set error.
 
-	// If an error occurred and the MTLCommandBuffer was not created, call finish() directly.
-	if ( !mtlCmdBuff ) { finish(); }
-
-	return mtlCmdBuff ? VK_SUCCESS : VK_ERROR_OUT_OF_POOL_MEMORY;
+	// Add completion callback to the MTLCommandBuffer to call finish(), 
+	// or if the MTLCommandBuffer could not be created, call finish() directly.
+	// Retrieve the result first, because finish() will destroy this instance.
+	VkResult rslt = getConfigurationResult();
+	if (mtlCmdBuff) {
+		[mtlCmdBuff addCompletedHandler: ^(id<MTLCommandBuffer> mtlCB) { this->finish(); }];
+		[mtlCmdBuff commit];
+	} else {
+		finish();
+	}
+	return rslt;
 }
 
 void MVKQueuePresentSurfaceSubmission::finish() {