Fixed crashes when using VK_EXT_metal_objects from ARC enabled apps

[MennoVink]

Fixing #2151

While this fixes ARC enabled apps, it will require non-ARC apps to manually release. I think that is to be expected, how else can the pointers be usable for an app-determined amount of time. Should i add some documentation for this somewhere?

Could you review if there need to be checks for making sure there is any output? For example:

auto* pBuffInfo = (VkExportMetalBufferInfoEXT*)next;
auto* mvkDevMem = (MVKDeviceMemory*)pBuffInfo->memory;
pBuffInfo->mtlBuffer = mvkDevMem->getMTLBuffer();
[pBuffInfo->mtlBuffer retain];

It's not checking if mvkDevMem is even valid before calling getMTLBuffer on it. Consequently i didn't check if getMTLBuffer's result is valid before calling retain on it. I'm assuming this is part of Vulkan's VU specs.

[cdavis5e]

Fixing #2151

While this fixes ARC enabled apps, it will require non-ARC apps to manually release. I think that is to be expected, how else can the pointers be usable for an app-determined amount of time. Should i add some documentation for this somewhere?

Probably. Really, it ought to be documented as part of the VK_EXT_metal_objects extension itself. But since AFAICT you're not a member of Khronos, we'll have to get someone else to update the extension...

Could you review if there need to be checks for making sure there is any output? For example:

auto* pBuffInfo = (VkExportMetalBufferInfoEXT*)next;
auto* mvkDevMem = (MVKDeviceMemory*)pBuffInfo->memory;
pBuffInfo->mtlBuffer = mvkDevMem->getMTLBuffer();
[pBuffInfo->mtlBuffer retain];

It's not checking if mvkDevMem is even valid before calling getMTLBuffer on it. Consequently i didn't check if getMTLBuffer's result is valid before calling retain on it. I'm assuming this is part of Vulkan's VU specs.

Generally speaking, if you were to pass a null handle to a Vulkan function where the VU says that the handle must [original emphasis] be valid, the behavior from that point is undefined, and literally anything is allowed to happen--from the program crashing to the program working fine to the command rm -rf / being executed as root to (as compiler engineers like to say) demons spontaneously flying out of your nose. So if there's a VU saying that VkExportMetalBufferInfoEXT::memory must be valid--which, of course, there is--you can avoid checking for null. Not requiring all these null checks allows Vulkan drivers to run faster--the constant validation that OpenGL implementations were required to perform was a notorious performance bottleneck.

[cdavis5e]

This is a curious case here. IOSurface was originally a Core Foundation-style object, which means that you had to use manual retain/release even from ARC-enabled Objective-C. But now there's an actual IOSurface class. Further, Swift is capable of automatically managing Core Foundation objects, and I believe it will actually wind up importing any IOSurfaceRef as an instance of the Objective-C IOSurface class. So, you really do need the CFRetain() here for Swift. (From Objective-C that means that the caller would either have to CFRelease() it manually, or with ARC use a __bridge_transfer cast or a call to CFBridgingRelease() to tell ARC about it.)

[billhollings]

Sorry for the delay in addressing this PR. I wanted to wait until I had a some time to dig into, and better understand, how ARC is handling these objects under the covers. I didn't want to replace the ARC break with the non-ARC break, without a better understanding of why this is happening in the first place.

Rather than add the additional retains, if we add __unsafe_unretained to the declarations of the struct members, it works for both ARC and non-ARC callers. I've requested some follow-up on issue #2151, to see if this will fix the problem you were encountering.

[billhollings]

Replaced by #2200.