From ecd8dd0ade7ecc40286cc3cf46a5a9490ee5a5a5 Mon Sep 17 00:00:00 2001 From: baldurk Date: Fri, 3 Apr 2020 12:43:12 +0100 Subject: [PATCH] Don't report replaced IDs in pipeline state * If we return back (accurate) replaced IDs in current pipeline state, the UI won't recognise them since the list of resources is cached and fixed at load time. * The replaced ID is still present in the shader reflection info as we return the replaced shader's reflection and not the original's. * We also change how we replace a programshader (glCreateShaderProgramv) on GL. Instead of creating a program and a shader - resulting in two objects replacing one - we instead replace the shader that got built with a program. We can do this safely since we know the shader won't be used as an actual real shader (since it was a program in the original capture too). --- renderdoc/core/resource_manager.h | 25 +++++++ renderdoc/driver/d3d11/d3d11_replay.cpp | 2 +- renderdoc/driver/d3d12/d3d12_replay.cpp | 6 +- renderdoc/driver/gl/gl_driver.cpp | 99 ++++++++++++------------- renderdoc/driver/gl/gl_manager.h | 7 +- renderdoc/driver/gl/gl_replay.cpp | 10 +-- renderdoc/driver/vulkan/vk_replay.cpp | 10 ++- 7 files changed, 92 insertions(+), 67 deletions(-) diff --git a/renderdoc/core/resource_manager.h b/renderdoc/core/resource_manager.h index e2a9b9a81..502440521 100644 --- a/renderdoc/core/resource_manager.h +++ b/renderdoc/core/resource_manager.h @@ -621,6 +621,11 @@ public: bool HasReplacement(ResourceId from); void RemoveReplacement(ResourceId id); + // get the original ID for a real ID that may be a replacement. i.e. if ID 123 is ID 10000005 + // live, and 10000005 live is replaced with 10000839, then calling this function with either ID + // 10000005 or ID 10000839 will return ID 123. + ResourceId GetUnreplacedOriginalID(ResourceId id); + // fetch original ID for a real ID or vice-versa. ResourceId GetOriginalID(ResourceId id); ResourceId GetLiveID(ResourceId id); @@ -728,7 +733,10 @@ protected: std::map m_ResourceRecords; // used during replay - holds current resource replacements + // replaced -> replacement std::map m_Replacements; + // replacement -> replaced (for looking up original IDs) + std::map m_Replaced; // During initial resources preparation, persistent resources are // postponed until serializing to RDC file. @@ -1371,7 +1379,10 @@ void ResourceManager::ReplaceResource(ResourceId from, ResourceId SCOPED_LOCK(m_Lock); if(HasLiveResource(to)) + { m_Replacements[from] = to; + m_Replaced[to] = from; + } } template @@ -1392,6 +1403,7 @@ void ResourceManager::RemoveReplacement(ResourceId id) if(it == m_Replacements.end()) return; + m_Replaced.erase(it->second); m_Replacements.erase(it); } @@ -1641,6 +1653,19 @@ ResourceId ResourceManager::GetOriginalID(ResourceId id) return m_OriginalIDs[id]; } +template +ResourceId ResourceManager::GetUnreplacedOriginalID(ResourceId id) +{ + if(id == ResourceId()) + return id; + + if(m_Replaced.find(id) != m_Replaced.end()) + return m_Replaced[id]; + + RDCASSERT(m_OriginalIDs.find(id) != m_OriginalIDs.end(), id); + return m_OriginalIDs[id]; +} + template ResourceId ResourceManager::GetLiveID(ResourceId id) { diff --git a/renderdoc/driver/d3d11/d3d11_replay.cpp b/renderdoc/driver/d3d11/d3d11_replay.cpp index ae4d155e5..946d0a1a0 100644 --- a/renderdoc/driver/d3d11/d3d11_replay.cpp +++ b/renderdoc/driver/d3d11/d3d11_replay.cpp @@ -772,7 +772,7 @@ void D3D11Replay::SavePipelineState(uint32_t eventId) dst.bindpointMapping = ShaderBindpointMapping(); } - dst.resourceId = rm->GetOriginalID(id); + dst.resourceId = rm->GetUnreplacedOriginalID(id); dst.reflection = refl; dst.constantBuffers.resize(D3D11_COMMONSHADER_CONSTANT_BUFFER_API_SLOT_COUNT); diff --git a/renderdoc/driver/d3d12/d3d12_replay.cpp b/renderdoc/driver/d3d12/d3d12_replay.cpp index 73612fd4c..dec73a8f8 100644 --- a/renderdoc/driver/d3d12/d3d12_replay.cpp +++ b/renderdoc/driver/d3d12/d3d12_replay.cpp @@ -1276,7 +1276,7 @@ void D3D12Replay::SavePipelineState(uint32_t eventId) D3D12ResourceManager *rm = m_pDevice->GetResourceManager(); - state.pipelineResourceId = rm->GetOriginalID(rs.pipe); + state.pipelineResourceId = rm->GetUnreplacedOriginalID(rs.pipe); WrappedID3D12PipelineState *pipe = NULL; @@ -1338,7 +1338,7 @@ void D3D12Replay::SavePipelineState(uint32_t eventId) { WrappedID3D12Shader *sh = (WrappedID3D12Shader *)pipe->compute->CS.pShaderBytecode; - state.computeShader.resourceId = sh->GetResourceID(); + state.computeShader.resourceId = rm->GetUnreplacedOriginalID(sh->GetResourceID()); state.computeShader.stage = ShaderStage::Compute; state.computeShader.reflection = &sh->GetDetails(); state.computeShader.bindpointMapping = sh->GetMapping(); @@ -1372,7 +1372,7 @@ void D3D12Replay::SavePipelineState(uint32_t eventId) if(sh) { - dst.resourceId = sh->GetResourceID(); + dst.resourceId = rm->GetUnreplacedOriginalID(sh->GetResourceID()); dst.bindpointMapping = sh->GetMapping(); dst.reflection = &sh->GetDetails(); } diff --git a/renderdoc/driver/gl/gl_driver.cpp b/renderdoc/driver/gl/gl_driver.cpp index fdd2c1f05..42b800155 100644 --- a/renderdoc/driver/gl/gl_driver.cpp +++ b/renderdoc/driver/gl/gl_driver.cpp @@ -1638,71 +1638,61 @@ void WrappedOpenGL::ReplaceResource(ResourceId from, ResourceId to) else if(fromresource.Namespace == eResProgram && toresource.Namespace == eResShader) { // if we want to replace a program with a shader, this is a glCreateShaderProgramv so we need - // to handle it specially + // to handle it specially. We take the source from the shader, delete the shader, and steal + // its ID to create a glCreateShaderProgramv. This avoids the awkward problem where we have + // two replacements (program and shader) for one resource. - ResourceId progsrcid = GetResourceManager()->GetLiveID(from); - const ProgramData &progdata = m_Programs[progsrcid]; + ResourceId targetId = GetResourceManager()->GetID(toresource); - // if this program has a replacement, remove it and delete the program generated for it - if(GetResourceManager()->HasReplacement(from)) - { - glDeleteProgram(GetResourceManager()->GetLiveResource(from).name); - GetResourceManager()->RemoveReplacement(from); - } + // copy the shader data + ShaderData shaddata = m_Shaders[targetId]; - GLuint progsrc = GetResourceManager()->GetLiveResource(from).name; + // delete the shader completely + glDeleteShader(toresource.name); + m_Shaders.erase(targetId); - // make a new program - GLuint progdst = glCreateProgram(); + // create a new unwrapped/unregistered programshader. This must be created unwrapped so we can + // assign the existing ID to it. + const char *str = shaddata.sources[0].c_str(); + toresource = ProgramRes(GetCtx(), GL.glCreateShaderProgramv(shaddata.type, 1, &str)); - ResourceId progdstid = GetResourceManager()->GetID(ProgramRes(GetCtx(), progdst)); + // re-register the programshader in the place of where the shader used to be + GetResourceManager()->RegisterResource(toresource, targetId); - // attach the shader - glAttachShader(progdst, GetResourceManager()->GetCurrentResource(to).name); + ProgramData &progDetails = m_Programs[targetId]; - // mark separable - glProgramParameteri(progdst, eGL_PROGRAM_SEPARABLE, GL_TRUE); + progDetails.linked = true; + progDetails.shaders.push_back(targetId); + progDetails.stageShaders[ShaderIdx(shaddata.type)] = targetId; + progDetails.shaderProgramUnlinkable = true; - // copy VS or FS bindings as necessary - ResourceId vs = progdata.stageShaders[0]; - ResourceId fs = progdata.stageShaders[4]; + ShaderData &shadDetails = m_Shaders[targetId]; - if(vs != ResourceId()) - CopyProgramAttribBindings(progsrc, progdst, &m_Shaders[vs].reflection); + shadDetails.type = shaddata.type; + shadDetails.sources = shaddata.sources; - if(fs != ResourceId()) - CopyProgramFragDataBindings(progsrc, progdst, &m_Shaders[fs].reflection); + shadDetails.ProcessCompilation(*this, targetId, 0); - // link new program - glLinkProgram(progdst); + GetResourceManager()->AddLiveResource(targetId, toresource); - GLint status = 0; - glGetProgramiv(progdst, eGL_LINK_STATUS, &status); + // finally since programs have state (sigh) we have to copy that across as well. + GLuint progsrc = fromresource.name; + GLuint progdst = toresource.name; - if(status == 0) - { - GLint len = 1024; - glGetProgramiv(progdst, eGL_INFO_LOG_LENGTH, &len); - char *buffer = new char[len + 1]; - glGetProgramInfoLog(progdst, len, NULL, buffer); - buffer[len] = 0; + if(shaddata.type == eGL_VERTEX_SHADER) + CopyProgramAttribBindings(progsrc, progdst, &shadDetails.reflection); - RDCWARN( - "When making program replacement for glCreateShaderProgramv shader, program failed " - "to link. Skipping replacement:\n%s", - buffer); + if(shaddata.type == eGL_FRAGMENT_SHADER) + CopyProgramFragDataBindings(progsrc, progdst, &shadDetails.reflection); - delete[] buffer; - - glDeleteProgram(progdst); - } - else { PerStageReflections dstStages; - FillReflectionArray(progdstid, dstStages); + FillReflectionArray(targetId, dstStages); std::map translate; + ResourceId progsrcid = GetResourceManager()->GetID(fromresource); + PerStageReflections stages; FillReflectionArray(progsrcid, stages); @@ -1711,11 +1701,11 @@ void WrappedOpenGL::ReplaceResource(ResourceId from, ResourceId to) // start with the original location translation table, to account for any // capture-replay translation - m_Programs[progdstid].locationTranslate = m_Programs[progsrcid].locationTranslate; + m_Programs[targetId].locationTranslate = m_Programs[progsrcid].locationTranslate; // compose on the one from editing. - for(auto lit = m_Programs[progdstid].locationTranslate.begin(); - lit != m_Programs[progdstid].locationTranslate.end(); lit++) + for(auto lit = m_Programs[targetId].locationTranslate.begin(); + lit != m_Programs[targetId].locationTranslate.end(); lit++) { auto lit2 = translate.find(lit->second); if(lit2 != translate.end()) @@ -1723,10 +1713,11 @@ void WrappedOpenGL::ReplaceResource(ResourceId from, ResourceId to) else lit->second = -1; } - - // replace the program - GetResourceManager()->ReplaceResource(from, progdstid); } + + // now finally we can do the replacement as normal + GetResourceManager()->RemoveReplacement(from); + GetResourceManager()->ReplaceResource(from, to); } else { @@ -1758,7 +1749,11 @@ void WrappedOpenGL::FreeTargetResource(ResourceId id) switch(resource.Namespace) { - case eResShader: glDeleteShader(resource.name); break; + case eResShader: + glDeleteShader(resource.name); + break; + // a compiled shader could have been promoted to a program if it were a glCreateShaderProgramv + case eResProgram: glDeleteProgram(resource.name); break; default: RDCERR("Unexpected resource type to be freed"); break; } } diff --git a/renderdoc/driver/gl/gl_manager.h b/renderdoc/driver/gl/gl_manager.h index 0ce186e16..cb2393b02 100644 --- a/renderdoc/driver/gl/gl_manager.h +++ b/renderdoc/driver/gl/gl_manager.h @@ -138,9 +138,10 @@ public: ResourceManager::RemoveResourceRecord(id); } - ResourceId RegisterResource(GLResource res) + ResourceId RegisterResource(GLResource res, ResourceId id = ResourceId()) { - ResourceId id = ResourceIDGen::GetNewUniqueID(); + if(id == ResourceId()) + id = ResourceIDGen::GetNewUniqueID(); m_CurrentResourceIds[res] = id; AddCurrentResource(id, res); return id; @@ -164,6 +165,8 @@ public: { m_Names.erase(it->second); + if(IsReplayMode(m_State) && HasLiveResource(it->second)) + EraseLiveResource(it->second); ReleaseCurrentResource(it->second); m_CurrentResourceIds.erase(res); } diff --git a/renderdoc/driver/gl/gl_replay.cpp b/renderdoc/driver/gl/gl_replay.cpp index be608734c..087511938 100644 --- a/renderdoc/driver/gl/gl_replay.cpp +++ b/renderdoc/driver/gl/gl_replay.cpp @@ -1019,7 +1019,7 @@ void GLReplay::SavePipelineState(uint32_t eventId) ResourceId id = rm->GetID(ProgramPipeRes(ctx, curProg)); auto &pipeDetails = m_pDriver->m_Pipelines[id]; - pipe.pipelineResourceId = rm->GetOriginalID(id); + pipe.pipelineResourceId = rm->GetUnreplacedOriginalID(id); for(size_t i = 0; i < ARRAY_COUNT(pipeDetails.stageShaders); i++) { @@ -1048,8 +1048,8 @@ void GLReplay::SavePipelineState(uint32_t eventId) mappings[i] = &stages[i]->bindpointMapping; - stages[i]->programResourceId = rm->GetOriginalID(pipeDetails.stagePrograms[i]); - stages[i]->shaderResourceId = rm->GetOriginalID(pipeDetails.stageShaders[i]); + stages[i]->programResourceId = rm->GetUnreplacedOriginalID(pipeDetails.stagePrograms[i]); + stages[i]->shaderResourceId = rm->GetUnreplacedOriginalID(pipeDetails.stageShaders[i]); } else { @@ -1090,8 +1090,8 @@ void GLReplay::SavePipelineState(uint32_t eventId) mappings[i] = &stages[i]->bindpointMapping; - stages[i]->programResourceId = rm->GetOriginalID(id); - stages[i]->shaderResourceId = rm->GetOriginalID(progDetails.stageShaders[i]); + stages[i]->programResourceId = rm->GetUnreplacedOriginalID(id); + stages[i]->shaderResourceId = rm->GetUnreplacedOriginalID(progDetails.stageShaders[i]); } else { diff --git a/renderdoc/driver/vulkan/vk_replay.cpp b/renderdoc/driver/vulkan/vk_replay.cpp index 87b7059ca..4cabb709c 100644 --- a/renderdoc/driver/vulkan/vk_replay.cpp +++ b/renderdoc/driver/vulkan/vk_replay.cpp @@ -1034,8 +1034,10 @@ void VulkanReplay::SavePipelineState(uint32_t eventId) memcpy(m_VulkanPipelineState.pushconsts.data(), state.pushconsts, state.pushConstSize); // General pipeline properties - m_VulkanPipelineState.compute.pipelineResourceId = rm->GetOriginalID(state.compute.pipeline); - m_VulkanPipelineState.graphics.pipelineResourceId = rm->GetOriginalID(state.graphics.pipeline); + m_VulkanPipelineState.compute.pipelineResourceId = + rm->GetUnreplacedOriginalID(state.compute.pipeline); + m_VulkanPipelineState.graphics.pipelineResourceId = + rm->GetUnreplacedOriginalID(state.graphics.pipeline); if(state.compute.pipeline != ResourceId()) { @@ -1049,7 +1051,7 @@ void VulkanReplay::SavePipelineState(uint32_t eventId) int i = 5; // 5 is the CS idx (VS, TCS, TES, GS, FS, CS) { - stage.resourceId = rm->GetOriginalID(p.shaders[i].module); + stage.resourceId = rm->GetUnreplacedOriginalID(p.shaders[i].module); stage.entryPoint = p.shaders[i].entryPoint; stage.stage = ShaderStage::Compute; @@ -1127,7 +1129,7 @@ void VulkanReplay::SavePipelineState(uint32_t eventId) for(size_t i = 0; i < ARRAY_COUNT(stages); i++) { - stages[i]->resourceId = rm->GetOriginalID(p.shaders[i].module); + stages[i]->resourceId = rm->GetUnreplacedOriginalID(p.shaders[i].module); stages[i]->entryPoint = p.shaders[i].entryPoint; stages[i]->stage = StageFromIndex(i);