From 18c17ae5ca47b86018f5529acc7d9bc03ea25ead Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 10:06:41 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectVulkan): a blit into the default framebuffer must execute the clear parked before it, not leave it for the readback --- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 27 +++++- .../Scenarios/ClearThenReadPixelsScenario.cpp | 83 +++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 24119c9f..a1bc1d6a 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -7570,12 +7570,21 @@ void main() { } } - if (!drawIsDefaultFbo) { + const auto destAttachmentType = + ResolveFramebufferCopyAttachmentType(*drawFbo, false, dstBinding.aspectMask); + if (drawIsDefaultFbo) { + // Same ordering rule for the default framebuffer's depth/stencil - see the + // colour twin below. + const Bool dstClearReady = MaterializePendingClearForDefaultFramebuffer( + frame.commandBuffer, *drawFbo, destAttachmentType); + MOBILEGL_ASSERT(dstClearReady, + "BlitFramebuffer: failed to materialize the default framebuffer's pending " + "depth/stencil clear"); + } else { // A clear queued for the destination predates this blit in API order; // execute it now, or its deferred materialization would later stomp the // copied contents (MC 26.3 OIT clears cloud_depth, then blits the main // depth into it - the stale loadOp=CLEAR erased the copy). - const auto destAttachmentType = ResolveFramebufferCopyAttachmentType(*drawFbo, false, dstBinding.aspectMask); const auto& destAttachment = drawFbo->GetAttachment(destAttachmentType); if (auto destTexture = destAttachment.GetTexture(); destTexture != nullptr) { const Bool dstClearReady = MaterializePendingClearForTexture(frame.commandBuffer, *destTexture); @@ -7780,7 +7789,19 @@ void main() { } } - if (!drawIsDefaultFbo) { + if (drawIsDefaultFbo) { + // The default framebuffer needs the same ordering, and needed it before anything + // consumed its parked clear: Minecraft clears the default framebuffer, renders the + // world into its own framebuffer and BLITS the result out, so nothing between the + // clear and the blit ever opens a render pass on the default framebuffer to fold the + // clear in as a loadOp. The clear therefore stayed pending across the whole frame, + // and the first path that did materialize it - the readback - executed it AFTER the + // blit and handed back a blank frame (every DirectVulkan retrace, ssim 0.000005). + const Bool dstClearReady = MaterializePendingClearForDefaultFramebuffer( + frame.commandBuffer, *drawFbo, drawFbo->GetDrawBuffers()[0]); + MOBILEGL_ASSERT(dstClearReady, + "BlitFramebuffer: failed to materialize the default framebuffer's pending clear"); + } else { // A clear queued for the destination predates this blit in API order; execute // it now, or its deferred materialization would later stomp the blitted color. const auto& destAttachment = drawFbo->GetAttachment(drawFbo->GetDrawBuffers()[0]); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp index 230fa553..e39102a3 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp @@ -180,4 +180,87 @@ void main() { o_color = vec4(0.1, 0.2, 0.3, 1.0); } gl.EndFrame(); glDeleteProgram(program); } + + // The other half of the same rule, and the one the first version of this fix got wrong: a + // parked clear must be executed BEFORE whatever writes the framebuffer next, not whenever the + // readback happens to notice it. Minecraft clears the default framebuffer, renders the world + // into its own framebuffer and blits the result out; nothing in between opens a render pass on + // the default framebuffer, so the clear stays parked across the whole frame. Materializing it + // at readback time therefore ran it AFTER the blit and returned a blank frame - which is what + // took every DirectVulkan retrace to ssim 0.000005. + TEST_F(ClearThenReadPixelsScenario, ABlitIntoTheDefaultFramebufferSurvivesAnEarlierClear) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + // Paint a source framebuffer, exactly as a game renders its world off-screen. + ColorFbo source = MakeColorFbo(width, height); + ASSERT_NE(source.fbo, 0u); + BindFbo(source); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + DrawFullViewportQuad(program); + + // Clear the DEFAULT framebuffer, then blit the source over it. The clear is white so a + // frame that lost the blit is unmistakable, and the blit's colour is fshSimple's. + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + ClearTo(1.0f, 1.0f, 1.0f, 1.0f); + glBindFramebuffer(GL_READ_FRAMEBUFFER, source.fbo); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + glBlitFramebuffer(0, 0, width, height, 0, 0, width, height, GL_COLOR_BUFFER_BIT, GL_NEAREST); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + EXPECT_EQ(FirstGLError(), 0u); + + const Image blitted = ReadPixels(width, height); + EXPECT_EQ(FirstGLError(), 0u); + const Rgba8 centre = blitted.At(width / 2, height / 2); + EXPECT_NEAR(centre.r, 26, 2) << "the blit into the default framebuffer did not survive the clear that " + "preceded it; read back rgba(" << static_cast(centre.r) << ", " + << static_cast(centre.g) << ", " << static_cast(centre.b) << ", " + << static_cast(centre.a) << ")"; + EXPECT_NEAR(centre.g, 51, 2); + EXPECT_NEAR(centre.b, 77, 2); + + DestroyColorFbo(source); + gl.EndFrame(); + glDeleteProgram(program); + } + + // The same ordering claim for the path that DOES open a render pass. It passes today (the + // render pass folds the clear into its loadOp and pops it), and it is here so a future change + // to the pending-clear lifecycle cannot quietly reverse clear and draw. + TEST_F(ClearThenReadPixelsScenario, ADrawIntoTheDefaultFramebufferSurvivesAnEarlierClear) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(1.0f, 1.0f, 1.0f, 1.0f); + DrawFullViewportQuad(program); + EXPECT_EQ(FirstGLError(), 0u); + + const Image painted = ReadPixels(width, height); + const Rgba8 centre = painted.At(width / 2, height / 2); + EXPECT_NEAR(centre.r, 26, 2) << "the draw did not survive the clear that preceded it"; + EXPECT_NEAR(centre.g, 51, 2); + EXPECT_NEAR(centre.b, 77, 2); + + gl.EndFrame(); + glDeleteProgram(program); + } } // namespace MGITest