From ada97bf7bd72af8dc6f73f8029ebb7acdb4180fe Mon Sep 17 00:00:00 2001 From: baldurk Date: Tue, 14 May 2019 11:48:34 +0100 Subject: [PATCH] Only reference dirty resource list at capture begin time. Closes #1309 * The problem with trying to keep the list of dirty resources identical at the start and the end of the capture is that resources being deleted or dirtied mid-frame causes problems. For the latter we have 'pending dirty' ugliness, for the former we weren't properly handling it in all cases. Instead just prepare initial contents from dirty resources, then use the list of resources that had initial contents prepared to determine which should be saved. * This means resources freed mid frame are fine, as long as we hold onto their resource records and initial contents which we do. It also means the pending dirty handling can be removed and we can just dirty resources as soon as we want - it will be ignored mid-frame and apply to subsequent frames. --- renderdoc/core/resource_manager.h | 96 +++++++++++++------------- renderdoc/driver/vulkan/vk_manager.cpp | 2 +- 2 files changed, 48 insertions(+), 50 deletions(-) diff --git a/renderdoc/core/resource_manager.h b/renderdoc/core/resource_manager.h index 9f1d3e4d3..5d68cc24f 100644 --- a/renderdoc/core/resource_manager.h +++ b/renderdoc/core/resource_manager.h @@ -604,11 +604,25 @@ protected: set m_DirtyResources; set m_PendingDirtyResources; + struct InitialContentDataOrChunk + { + Chunk *chunk = NULL; + InitialContentData data; + + void Free(ResourceManager *mgr) + { + if(chunk) + { + delete chunk; + chunk = NULL; + } + + data.Free(mgr); + } + }; + // used during capture or replay - holds initial contents - map m_InitialContents; - // on capture, if a chunk was prepared in Prepare_InitialContents and added, don't re-serialise. - // Some initial contents may not need the delayed readback. - map m_InitialChunks; + map m_InitialContents; // used during capture or replay - map of resources currently alive with their real IDs, used in // capture and replay. @@ -757,13 +771,9 @@ void ResourceManager::SetInitialContents(ResourceId id, InitialCo auto it = m_InitialContents.find(id); if(it != m_InitialContents.end()) - { - // free previous contents it->second.Free(this); - m_InitialContents.erase(it); - } - m_InitialContents[id] = contents; + m_InitialContents[id].data = contents; } template @@ -772,19 +782,14 @@ void ResourceManager::SetInitialChunk(ResourceId id, Chunk *chunk SCOPED_LOCK(m_Lock); RDCASSERT(id != ResourceId()); - - auto it = m_InitialChunks.find(id); - RDCASSERT(chunk->GetChunkType() == SystemChunk::InitialContents); - if(it != m_InitialChunks.end()) - { - RDCERR("Initial chunk set for ID %llu twice", id); - delete chunk; - return; - } + InitialContentDataOrChunk &data = m_InitialContents[id]; - m_InitialChunks[id] = chunk; + if(data.chunk) + delete data.chunk; + + data.chunk = chunk; } template @@ -797,7 +802,7 @@ typename Configuration::InitialContentData ResourceManager::GetIn return InitialContentData(); if(m_InitialContents.find(id) != m_InitialContents.end()) - return m_InitialContents[id]; + return m_InitialContents[id].data; return InitialContentData(); } @@ -833,6 +838,8 @@ void ResourceManager::Serialise_InitialContentsNeeded(WriteSerial // reasonable estimate, and these records are small WrittenRecords.reserve(m_FrameReferencedResources.size()); + // all resources that were recorded as being modified should be included in the list of those + // needing initial contents for(auto it = m_FrameReferencedResources.begin(); it != m_FrameReferencedResources.end(); ++it) { RecordType *record = GetResourceRecord(it->first); @@ -844,9 +851,10 @@ void ResourceManager::Serialise_InitialContentsNeeded(WriteSerial } } - for(auto it = m_DirtyResources.begin(); it != m_DirtyResources.end(); ++it) + // any resources that had initial contents generated should also be included + for(auto it = m_InitialContents.begin(); it != m_InitialContents.end(); ++it) { - ResourceId id = *it; + ResourceId id = it->first; auto ref = m_FrameReferencedResources.find(id); if(ref == m_FrameReferencedResources.end() || !IsDirtyFrameRef(ref->second)) { @@ -872,11 +880,6 @@ void ResourceManager::FreeInitialContents() if(!m_InitialContents.empty()) m_InitialContents.erase(m_InitialContents.begin()); } - - for(auto it = m_InitialChunks.begin(); it != m_InitialChunks.end(); ++it) - delete it->second; - - m_InitialChunks.clear(); } template @@ -924,9 +927,9 @@ void ResourceManager::ApplyInitialContents() for(auto it = resources.begin(); it != resources.end(); ++it) { ResourceId id = *it; - InitialContentData data = m_InitialContents[id]; + const InitialContentDataOrChunk &data = m_InitialContents[id]; WrappedResourceType live = GetLiveResource(id); - Apply_InitialState(live, data); + Apply_InitialState(live, data.data); } RDCDEBUG("Applied %d", (uint32_t)resources.size()); } @@ -1026,12 +1029,16 @@ void ResourceManager::PrepareInitialContents() RenderDoc::Inst().SetProgress(CaptureProgress::PrepareInitialStates, idx / num); idx += 1.0f; + // if somehow this resource has been deleted but is still dirty, we can't prepare it. Resources + // deleted prior to beginning the frame capture cannot linger and be needed - we only need to + // care about resources deleted after this point (mid-capture) if(!HasCurrentResource(id)) continue; RecordType *record = GetResourceRecord(id); WrappedResourceType res = GetCurrentResource(id); + // don't prepare internal resources, or those without a record if(record == NULL || record->InternalResource) continue; @@ -1071,14 +1078,14 @@ void ResourceManager::InsertInitialContentsChunks(WriteSerialiser uint32_t dirty = 0; uint32_t skipped = 0; - RDCDEBUG("Checking %u possibly dirty resources", (uint32_t)m_DirtyResources.size()); + RDCDEBUG("Checking %u resources with initial contents", (uint32_t)m_InitialContents.size()); - float num = float(m_DirtyResources.size()); + float num = float(m_InitialContents.size()); float idx = 0.0f; - for(auto it = m_DirtyResources.begin(); it != m_DirtyResources.end(); ++it) + for(auto it = m_InitialContents.begin(); it != m_InitialContents.end(); ++it) { - ResourceId id = *it; + ResourceId id = it->first; RenderDoc::Inst().SetProgress(CaptureProgress::SerialiseInitialStates, idx / num); idx += 1.0f; @@ -1133,16 +1140,14 @@ void ResourceManager::InsertInitialContentsChunks(WriteSerialiser if(!Need_InitialStateChunk(res)) { - // just need to grab data, don't create chunk - Serialise_InitialState(ser, id, res); + // this was handled in ApplyInitialContentsNonChunks(), do nothing as there's no point copying + // the data again (it's already been serialised). continue; } - auto preparedChunk = m_InitialChunks.find(id); - if(preparedChunk != m_InitialChunks.end()) + if(it->second.chunk) { - preparedChunk->second->Write(ser); - m_InitialChunks.erase(preparedChunk); + it->second.chunk->Write(ser); } else { @@ -1154,8 +1159,7 @@ void ResourceManager::InsertInitialContentsChunks(WriteSerialiser } } - RDCDEBUG("Serialised %u dirty resources, skipped %u unreferenced", dirty, skipped); - + RDCDEBUG("Serialised %u resources, skipped %u unreferenced", dirty, skipped); dirty = 0; for(auto it = m_CurrentResourceMap.begin(); it != m_CurrentResourceMap.end(); ++it) @@ -1186,12 +1190,6 @@ void ResourceManager::InsertInitialContentsChunks(WriteSerialiser RDCDEBUG("Force-serialised %u dirty resources", dirty); - // delete/cleanup any chunks that weren't used (maybe the resource was not - // referenced). - for(auto it = m_InitialChunks.begin(); it != m_InitialChunks.end(); ++it) - delete it->second; - - m_InitialChunks.clear(); } template @@ -1199,9 +1197,9 @@ void ResourceManager::ApplyInitialContentsNonChunks(WriteSerialis { SCOPED_LOCK(m_Lock); - for(auto it = m_DirtyResources.begin(); it != m_DirtyResources.end(); ++it) + for(auto it = m_InitialContents.begin(); it != m_InitialContents.end(); ++it) { - ResourceId id = *it; + ResourceId id = it->first; if(m_FrameReferencedResources.find(id) == m_FrameReferencedResources.end() && !RenderDoc::Inst().GetCaptureOptions().refAllResources) diff --git a/renderdoc/driver/vulkan/vk_manager.cpp b/renderdoc/driver/vulkan/vk_manager.cpp index 18b2eede8..7548c4a20 100644 --- a/renderdoc/driver/vulkan/vk_manager.cpp +++ b/renderdoc/driver/vulkan/vk_manager.cpp @@ -764,7 +764,7 @@ std::vector VulkanResourceManager::InitialContentResources() std::vector resources = ResourceManager::InitialContentResources(); std::sort(resources.begin(), resources.end(), [this](ResourceId a, ResourceId b) { - return m_InitialContents[a].type < m_InitialContents[b].type; + return m_InitialContents[a].data.type < m_InitialContents[b].data.type; }); return resources; }