Treat duplicate handles correctly while in replay

* For those types that I think it's valid for them to be duplicated, we
  look up if an existing wrapper is present and if so use that - and
  just use ReplaceResource.
* If it turns out some other types are duplicated, the handling can be
  added easily.

renderdoc/driver/vulkan/vk_resources.h

@@ -380,10 +380,22 @@ struct WrappedVkSurfaceKHR : WrappedVkNonDispRes
 	enum { TypeEnum = eResSurface, };
 };
 
-// VKTODOMED Need to find out which resources can validly return duplicate
-// handles for unique creates. E.g. if there are the same input parameters
-// to multiple create calls - perhaps it is valid for any handle to be
-// returned twice.
+// Note: we assume only the following resources can return duplicate handles (and so
+// on replay we need to handle two distinct ids with the same handle.
+// Other resources are discounted because they have 'state' or otherwise wouldn't make
+// sense to alias. Some of these are very *unlikely* to alias/duplicate, but they could
+// in theory do so e.g. if the inputs to the create function were hashed.
+// Listed in order from least likely to most likely to alias:
+//
+// * VkPipeline
+// * VkShaderModule
+// * VkRenderPass
+// * VkPipelineLayout
+// * VkDescriptorSetLayout
+// * VkFramebuffer
+// * VkBufferView
+// * VkImageView
+// * VkSampler
 
 // template magic voodoo to unwrap types
 template<typename inner> struct UnwrapHelper {};

renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp

@@ -119,10 +119,26 @@ bool WrappedVulkan::Serialise_vkCreateDescriptorSetLayout(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), layout);
-			GetResourceManager()->AddLiveResource(id, layout);
-		
-			m_CreationInfo.m_DescSetLayout[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(layout)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(layout)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyDescriptorSetLayout(Unwrap(device), layout, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), layout);
+				GetResourceManager()->AddLiveResource(id, layout);
+			
+				m_CreationInfo.m_DescSetLayout[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 

renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp

@@ -351,10 +351,26 @@ bool WrappedVulkan::Serialise_vkCreateSampler(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), samp);
-			GetResourceManager()->AddLiveResource(id, samp);
-		
-			m_CreationInfo.m_Sampler[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(samp)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(samp)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroySampler(Unwrap(device), samp, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), samp);
+				GetResourceManager()->AddLiveResource(id, samp);
+			
+				m_CreationInfo.m_Sampler[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -424,10 +440,26 @@ bool WrappedVulkan::Serialise_vkCreateFramebuffer(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), fb);
-			GetResourceManager()->AddLiveResource(id, fb);
-		
-			m_CreationInfo.m_Framebuffer[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(fb)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(fb)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyFramebuffer(Unwrap(device), fb, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), fb);
+				GetResourceManager()->AddLiveResource(id, fb);
+			
+				m_CreationInfo.m_Framebuffer[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -532,27 +564,58 @@ bool WrappedVulkan::Serialise_vkCreateRenderPass(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), rp);
-			GetResourceManager()->AddLiveResource(id, rp);
+			ResourceId live;
 
-			// make a version of the render pass that loads from its attachments,
-			// so it can be used for replaying a single draw after a render pass
-			// without doing a clear or a DONT_CARE load.
-			for(uint32_t i=0; i < info.attachmentCount; i++)
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(rp)))
 			{
-				att[i].loadOp = VK_ATTACHMENT_LOAD_OP_LOAD;
-				att[i].stencilLoadOp = VK_ATTACHMENT_LOAD_OP_LOAD;
+				live = GetResourceManager()->GetNonDispWrapper(rp)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyRenderPass(Unwrap(device), rp, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
 			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), rp);
+				GetResourceManager()->AddLiveResource(id, rp);
 
-			ret = ObjDisp(device)->CreateRenderPass(Unwrap(device), &info, NULL, &rpinfo.loadRP);
-			RDCASSERT(ret == VK_SUCCESS);
+				// make a version of the render pass that loads from its attachments,
+				// so it can be used for replaying a single draw after a render pass
+				// without doing a clear or a DONT_CARE load.
+				for(uint32_t i=0; i < info.attachmentCount; i++)
+				{
+					att[i].loadOp = VK_ATTACHMENT_LOAD_OP_LOAD;
+					att[i].stencilLoadOp = VK_ATTACHMENT_LOAD_OP_LOAD;
+				}
 
-			ResourceId loadRPid = GetResourceManager()->WrapResource(Unwrap(device), rpinfo.loadRP);
-			
-			// register as a live-only resource, so it is cleaned up properly
-			GetResourceManager()->AddLiveResource(loadRPid, rpinfo.loadRP);
-		
-			m_CreationInfo.m_RenderPass[live] = rpinfo;
+				ret = ObjDisp(device)->CreateRenderPass(Unwrap(device), &info, NULL, &rpinfo.loadRP);
+				RDCASSERT(ret == VK_SUCCESS);
+				
+				// handle the loadRP being a duplicate
+				if(GetResourceManager()->HasWrapper(ToTypedHandle(rpinfo.loadRP)))
+				{
+					// just fetch the existing wrapped object
+					rpinfo.loadRP = (VkRenderPass)(uint64_t)GetResourceManager()->GetNonDispWrapper(rpinfo.loadRP);
+
+					// destroy this instance of the duplicate, as we must have matching create/destroy
+					// calls and there won't be a wrapped resource hanging around to destroy this one.
+					ObjDisp(device)->DestroyRenderPass(Unwrap(device), rpinfo.loadRP, NULL);
+
+					// don't need to ReplaceResource as no IDs are involved
+				}
+				else
+				{
+					ResourceId loadRPid = GetResourceManager()->WrapResource(Unwrap(device), rpinfo.loadRP);
+
+					// register as a live-only resource, so it is cleaned up properly
+					GetResourceManager()->AddLiveResource(loadRPid, rpinfo.loadRP);
+				}
+				
+				m_CreationInfo.m_RenderPass[live] = rpinfo;
+			}
 		}
 	}
 

renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp

@@ -822,10 +822,26 @@ bool WrappedVulkan::Serialise_vkCreateBufferView(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), view);
-			GetResourceManager()->AddLiveResource(id, view);
-		
-			m_CreationInfo.m_BufferView[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(view)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(view)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyBufferView(Unwrap(device), view, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), view);
+				GetResourceManager()->AddLiveResource(id, view);
+			
+				m_CreationInfo.m_BufferView[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -1093,10 +1109,26 @@ bool WrappedVulkan::Serialise_vkCreateImageView(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), view);
-			GetResourceManager()->AddLiveResource(id, view);
-		
-			m_CreationInfo.m_ImageView[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(view)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(view)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyImageView(Unwrap(device), view, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), view);
+				GetResourceManager()->AddLiveResource(id, view);
+			
+				m_CreationInfo.m_ImageView[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 

renderdoc/driver/vulkan/wrappers/vk_shader_funcs.cpp

@@ -52,10 +52,26 @@ bool WrappedVulkan::Serialise_vkCreatePipelineLayout(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), layout);
-			GetResourceManager()->AddLiveResource(id, layout);
+			ResourceId live;
 
-			m_CreationInfo.m_PipelineLayout[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(layout)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(layout)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyPipelineLayout(Unwrap(device), layout, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), layout);
+				GetResourceManager()->AddLiveResource(id, layout);
+			
+				m_CreationInfo.m_PipelineLayout[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -137,10 +153,26 @@ bool WrappedVulkan::Serialise_vkCreateShaderModule(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), sh);
-			GetResourceManager()->AddLiveResource(id, sh);
+			ResourceId live;
 
-			m_CreationInfo.m_ShaderModule[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(sh)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(sh)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyShaderModule(Unwrap(device), sh, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), sh);
+				GetResourceManager()->AddLiveResource(id, sh);
+			
+				m_CreationInfo.m_ShaderModule[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -286,10 +318,26 @@ bool WrappedVulkan::Serialise_vkCreateGraphicsPipelines(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), pipe);
-			GetResourceManager()->AddLiveResource(id, pipe);
-		
-			m_CreationInfo.m_Pipeline[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(pipe)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(pipe)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyPipeline(Unwrap(device), pipe, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), pipe);
+				GetResourceManager()->AddLiveResource(id, pipe);
+			
+				m_CreationInfo.m_Pipeline[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 
@@ -411,10 +459,26 @@ bool WrappedVulkan::Serialise_vkCreateComputePipelines(
 		}
 		else
 		{
-			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), pipe);
-			GetResourceManager()->AddLiveResource(id, pipe);
-		
-			m_CreationInfo.m_Pipeline[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			ResourceId live;
+
+			if(GetResourceManager()->HasWrapper(ToTypedHandle(pipe)))
+			{
+				live = GetResourceManager()->GetNonDispWrapper(pipe)->id;
+
+				// destroy this instance of the duplicate, as we must have matching create/destroy
+				// calls and there won't be a wrapped resource hanging around to destroy this one.
+				ObjDisp(device)->DestroyPipeline(Unwrap(device), pipe, NULL);
+
+				// whenever the new ID is requested, return the old ID, via replacements.
+				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
+			}
+			else
+			{
+				live = GetResourceManager()->WrapResource(Unwrap(device), pipe);
+				GetResourceManager()->AddLiveResource(id, pipe);
+			
+				m_CreationInfo.m_Pipeline[live].Init(GetResourceManager(), m_CreationInfo, &info);
+			}
 		}
 	}
 

renderdoc/driver/vulkan/wrappers/vk_sync_funcs.cpp

@@ -495,18 +495,8 @@ bool WrappedVulkan::Serialise_vkCreateSemaphore(
 		}
 		else
 		{
-			if(GetResourceManager()->HasWrapper(ToTypedHandle(sem)))
-			{
-				// VKTODOMED need to handle duplicate objects better than this, perhaps
-				ResourceId live = GetResourceManager()->GetNonDispWrapper(sem)->id;
-				RDCDEBUG("Doing hack for duplicate objects that replay expects to be distinct - %llu -> %llu", id, live);
-				GetResourceManager()->ReplaceResource(id, GetResourceManager()->GetOriginalID(live));
-			}
-			else
-			{
-				ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), sem);
-				GetResourceManager()->AddLiveResource(id, sem);
-			}
+			ResourceId live = GetResourceManager()->WrapResource(Unwrap(device), sem);
+			GetResourceManager()->AddLiveResource(id, sem);
 		}
 	}