[Fix, Test] (MG_Backend/DirectVulkan): a blit into the default framebuffer must execute the clear parked before it, not leave it for the readback

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]);

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<int>(centre.r) << ", "
+                                     << static_cast<int>(centre.g) << ", " << static_cast<int>(centre.b) << ", "
+                                     << static_cast<int>(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