From d7976326fa07011be2d3e8801f9460c8ddea54f7 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Thu, 6 Aug 2026 21:55:41 -0400 Subject: [PATCH] [Fix] (MG_Backend): two DirectVulkan draw memos trusted more than they proved Two correctness holes from the round-7/8 fast-path work, found by bisecting the retrace matrix after corruption reports on device. Cross-frame slice trust: the vertex-binding and EBO memos skipped the acquire - the frame's content-sync point - whenever their recorded slice epochs still matched, trusting the BumpSliceEpoch inventory to cover every way a buffer's GPU copy can go stale. At least one mutation path escapes it: journeymap and common-mods retraces shipped visibly corrupted, and Sodium on an Adreno device rendered random triangles from stale vertex data. A memo recorded in an earlier frame now declines, so the first draw of each (VAO, frame) re-runs the full acquire; the same-frame paths (layout memo, factory-chase elimination, one-compare rescue) are untouched. The cross-frame idea can return once the bump-site inventory is proven complete against exactly these traces. Transform-flags memo key: GetShaderTransformFlags reads the swapchain pre-transform AND whether the bound draw framebuffer is the default one - only a presenting pass gets the Y-flip/rotation bits. The memo declared it pure in the pre-transform, so after any render-to-texture pass the next default-framebuffer pass inherited the FBO's unflipped flags: 1.17-main-menu retraced as a perfectly rendered, perfectly upside-down frame (SSIM 0.052, deterministic), and cloud passes flickered on device. The memo now keys on (preTransform, isDefaultFbo). DirectVulkan retraces for 1.17-main-menu, journeymap, common-mods, sodium and xaero-world-map all pass on lavapipe; unit tests 421/421. --- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 81 ++++++------------- .../DirectVulkan/Renderer/VulkanRenderer.h | 11 ++- 2 files changed, 33 insertions(+), 59 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 0644cfd9..df2803d5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3114,47 +3114,16 @@ void main() { return true; } - // Cross-frame revalidation. Only all-resident, unmapped layouts qualify: - // - a streamed binding's slice moves to a new arena block every frame BY DESIGN, - // but every such move funnels through BumpSliceEpoch, so the per-binding epoch - // compares below catch it (as they do a respecify, a sub-data update, a - // resident<->streamed promotion and a buffer becoming persistently mapped); - // - a mapped buffer mutates its shadow with no API call and must re-run the - // Sync/acquire path every frame, so it is excluded outright; - // - epochs are minted from a process-lifetime counter, so a deleted buffer (or - // resource) recycled at the same address can never reproduce a recorded epoch. - if (entry.anyBufferMapped) { - return false; - } - const auto& attributes = vao.GetAllAttributes(); - VkBufferResource* resources[ResolvedVertexBindings::kMaxBindings] = {}; - for (Uint32 binding = 0; binding < entry.bindingCount; ++binding) { - // Compare against the LIVE pointer before any dereference: entry.buffers may - // dangle if the app deleted a buffer since (the VAO unbind that deletion - // performs bumps the config version, so the hash compare above already - // declined - this is defense in depth for the aliased-address case). - auto* bufferObject = attributes[entry.attributeLocations[binding]].Buffer.get(); - if (bufferObject != entry.buffers[binding]) { - return false; - } - auto* resource = static_cast(bufferObject->GetBackendResource().get()); - if (resource == nullptr || resource->sliceEpoch != entry.sliceEpochs[binding]) { - return false; - } - resources[binding] = resource; - } - // Every binding still resolves to the recorded slice. The per-frame resolve this - // replaces had one side effect the busy tracking depends on (glBufferSubData's - // host-write-vs-staged-copy choice): stamping each resource's GPU-use serial. - // Do exactly that, then re-arm the entry so the rest of the frame's draws take - // the one-compare path above. - for (Uint32 binding = 0; binding < entry.bindingCount; ++binding) { - resources[binding]->lastUseSerial = frameSerial; - } - entry.frameSerial = frameSerial; - entry.sliceEpochCounter = m_bufferManager.GetSliceEpochCounter(); - ShadowedBindVertexBuffers(commandBuffer, entry.vkBuffers, entry.vkOffsets, entry.bindingCount); - return true; + // NO cross-frame trust: a memo recorded in an earlier frame declines here and + // the draw re-resolves through the full acquire path. The epoch-compare + // revalidation that used to sit here shipped visible corruption (journeymap / + // common-mods retraces, vertex anomalies on Adreno): the acquire path is the + // frame's content-sync point, and skipping it across frames trusted the + // BumpSliceEpoch inventory to cover every way a buffer's GPU copy can go stale. + // At least one path escapes it. Until that inventory is proven complete the + // hot layout memo above (same-frame) keeps the factory-chase win, and the + // first draw of each (VAO, frame) pays one full resolve. + return false; } VulkanRenderer::VaoDrawMemo* VulkanRenderer::LookupVaoDrawMemo( @@ -3709,18 +3678,10 @@ void main() { if (indexMemo->indexFrameSerial == frameSerial && indexMemo->indexSliceEpochCounter == m_bufferManager.GetSliceEpochCounter()) { sliceStillValid = true; - } else { - auto* resource = static_cast( - indexBufferShared->GetBackendResource().get()); - if (resource != nullptr && resource->sliceEpoch == indexMemo->indexSliceEpoch) { - sliceStillValid = true; - // Same busy-tracking stamp the skipped acquire would have made, - // then re-arm the one-compare path for the rest of the frame. - resource->lastUseSerial = frameSerial; - indexMemo->indexFrameSerial = frameSerial; - indexMemo->indexSliceEpochCounter = m_bufferManager.GetSliceEpochCounter(); - } } + // NO cross-frame trust for the EBO either (same corruption class as the + // vertex half, see TryBindResolvedVertexBindings): a memo from an earlier + // frame declines and the draw re-runs the acquire, which is the sync point. if (sliceStillValid) { const VkDeviceSize memoBindOffset = indexMemo->indexSliceOffset + static_cast(pIndexBufferView->indexByteOffset); @@ -5113,13 +5074,21 @@ void main() { } Uint32 VulkanRenderer::GetBaseTransformFlagsRaw() { - // GetShaderTransformFlags is a pure function of the pre-transform, which only - // changes on surface rotation - memoised so the per-draw path pays one field - // compare instead of the call + switch. + // GetShaderTransformFlags is a function of the pre-transform AND of whether + // the bound draw framebuffer is the default one (the Y-flip/rotation bits + // apply only when presenting). Memo keyed on both; keying on the + // pre-transform alone served an FBO pass's unflipped flags to the following + // default-framebuffer pass and flipped the whole frame. const VkSurfaceTransformFlagBitsKHR preTransform = m_swapchainObject.GetPreTransform(); - if (preTransform != m_baseTransformFlagsPreTransform) { + const auto& currentDrawFBO = + MG_State::pGLContext->GetFramebufferBindingSlot(FramebufferTarget::Draw).GetBoundObject(); + const Bool isDefaultFbo = currentDrawFBO != nullptr && currentDrawFBO->IsDefaultFramebuffer(); + if (!m_baseTransformFlagsKeyValid || preTransform != m_baseTransformFlagsPreTransform || + isDefaultFbo != m_baseTransformFlagsIsDefaultFbo) { m_baseTransformFlagsCache = GetShaderTransformFlags(preTransform).GetRaw(); m_baseTransformFlagsPreTransform = preTransform; + m_baseTransformFlagsIsDefaultFbo = isDefaultFbo; + m_baseTransformFlagsKeyValid = true; } return m_baseTransformFlagsCache; } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index 6182d91e..f3592af0 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -648,11 +648,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint32 m_pipelineStateHashColorCount = 0; Uint64 m_pipelineStateHash = 0; Bool m_pipelineStateHashValid = false; - // GetShaderTransformFlags(preTransform) memo: a pure function of the swapchain - // pre-transform, re-evaluated only when that value changes (surface rotation). - // No other invalidation input exists. + // GetShaderTransformFlags memo. NOT pure in the pre-transform alone: the + // function also reads whether the bound DRAW framebuffer is the default one + // (only the default framebuffer gets the Y-flip and rotation bits - an FBO + // pass renders unflipped). Keyed on BOTH inputs; missing the FBO bit shipped + // an upside-down default-framebuffer pass after any render-to-texture + // (minecraft-1.17-main-menu retrace, whole frame flipped). VkSurfaceTransformFlagBitsKHR m_baseTransformFlagsPreTransform = VK_SURFACE_TRANSFORM_FLAG_BITS_MAX_ENUM_KHR; + Bool m_baseTransformFlagsIsDefaultFbo = false; + Bool m_baseTransformFlagsKeyValid = false; Uint32 m_baseTransformFlagsCache = 0; Uint32 GetBaseTransformFlagsRaw(); // Drops every memoized pipeline handle. Required at command-buffer