From 9f4cc2880d94514ce8b545868e5b3901288eb0dd Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 17 Dec 2015 13:29:24 +0100 Subject: [PATCH] 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 | 20 +++- .../vulkan/wrappers/vk_descriptor_funcs.cpp | 24 +++- .../driver/vulkan/wrappers/vk_misc_funcs.cpp | 111 ++++++++++++++---- .../vulkan/wrappers/vk_resource_funcs.cpp | 48 ++++++-- .../vulkan/wrappers/vk_shader_funcs.cpp | 92 ++++++++++++--- .../driver/vulkan/wrappers/vk_sync_funcs.cpp | 14 +-- 6 files changed, 243 insertions(+), 66 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_resources.h b/renderdoc/driver/vulkan/vk_resources.h index 795099d59..91cb8688e 100644 --- a/renderdoc/driver/vulkan/vk_resources.h +++ b/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 struct UnwrapHelper {}; diff --git a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp index 1143fca85..aad4a13a9 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp +++ b/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); + } } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp index a62af0fb9..5066a140a 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp +++ b/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; + } } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp index 6442c91ea..36a80a6a3 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp +++ b/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); + } } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_shader_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_shader_funcs.cpp index c9cac8644..c614c435b 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_shader_funcs.cpp +++ b/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); + } } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_sync_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_sync_funcs.cpp index 92b5ee6ca..19c23d5ff 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_sync_funcs.cpp +++ b/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); } }