[MG_Backend, MG_IntegrationTest, scripts] (P7): a declined multisample depth/stencil shape is decided once before any aspect runs, so an erroring blit writes nothing, and the shader resolve arm gets a flipped knob twin

- BlitWireFramebuffers runs colour first. A scaled COLOR|DEPTH blit out of a multisample framebuffer scaled and wrote its colour and then recorded INVALID_OPERATION on its depth, and an X-mirrored one mirrored its colour and left its depth alone - partial effects GL 18.3.1 does not allow. The shape decline (scale, X mirror, cross-format Y mirror) moves into DeclineWireDepthStencilResolveShape, which BlitWireFramebuffers asks for every depth/stencil aspect that would take the multisample resolve arm before any aspect runs; ResolveWireDepthStencil asks it too for any other caller. Default read/draw framebuffers are left to the lambda (a default read is never multisampled; a default draw behind one is the region Fatal, and resolving it early would move its write index). - The X decline and the cross-format Y decline get their own MGLOG_E_ONCE sites; one site used to cover both, so after the first X decline a cross-format Y decline logged nothing. - An empty rectangle on one side only (64x48 onto 0x48) is a size mismatch and now takes the scale decline (INVALID_OPERATION) instead of returning silently; empty on both sides stays a silent no-op. - DirectVulkan.{Split,Spawn}.MsFlip1. forces MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE=1, so the shader arm min-corner rectangle under a flip runs on this host; split+spawn only and named in MAGMA_SERVER_ENV_KNOB_NO_TCP. - spawn_lane_parity.py: every server-env-knob tail must match exactly one split key, so a dead or over-broad tail fails the gate instead of riding silently. - The flipped-resolve case gains a COLOR|DEPTH scale leg (error, colour untouched, depth untouched) and an empty-destination leg (error, depth untouched). - Red-once, each on split + spawn (+ tcp MsFlip.) and restored: pre-pass disabled -> all five MsFlip/MsFlip1 entries fail "the declined COLOR|DEPTH scale must not have written its colour either" (read {255,0,0,255}, want {0,0,255,255}); round-1 silent empty-side return -> all five fail the empty-destination leg (error 0, want 1282); shader arm handed sx0/sy0 instead of the min corner -> MsFlip1 fails on both arms ("768 of 768 depth values differ from 0.75", "928 of 3072 stencil values are wrong; first at (20, 4): got 255, want 99") while MsFlip. stays green; a dead ".MsFlip2." tail -> parity rc 1 "matches 0 split entrie(s)". - The split once-sites have no lane red-once: the cross-format Y decline needs a device whose renderbuffer and texture D24S8 map to different VkFormats, which lavapipe is not.
2026-11-07 18:13:23 +09:00 · 2026-09-22 20:49:45 -04:00
parent 1e46a67091
commit 4db17d58c6
5 changed files with 179 additions and 38 deletions
@@ -513,6 +513,13 @@ namespace MobileGL::MG_Backend::DirectVulkan {
        void ResolveWireDepthStencil(WireImage source, WireImage destination,
                                     GLint sx0, GLint sy0, GLint sx1, GLint sy1,
                                     GLint dx0, GLint dy0, GLint dx1, GLint dy1);
+        // True when the rectangles give a multisample depth/stencil resolve a shape it declines
+        // (a scale, an X mirror, a Y mirror across two formats); the decline has then been
+        // logged and its INVALID_OPERATION recorded. BlitWireFramebuffers asks it BEFORE any
+        // aspect runs, so a declined call leaves every attachment alone.
+        Bool DeclineWireDepthStencilResolveShape(const WireImage& source, const WireImage& destination,
+                                                 GLint sx0, GLint sy0, GLint sx1, GLint sy1,
+                                                 GLint dx0, GLint dy0, GLint dx1, GLint dy1);
        PFN_vkCreateRenderPass2 m_wireCreateRenderPass2 = nullptr;
        VkResolveModeFlags m_wireDepthResolveModes = 0;
        VkResolveModeFlags m_wireStencilResolveModes = 0;
@@ -432,6 +432,68 @@
            MagmaWireFatal("read-conversion");
    }

+    Bool VulkanRenderer::DeclineWireDepthStencilResolveShape(const WireImage& source,
+        const WireImage& destination, GLint sx0, GLint sy0, GLint sx1, GLint sy1,
+        GLint dx0, GLint dy0, GLint dx1, GLint dy1) {
+        const Int64 spanX = static_cast<Int64>(sx1) - sx0;
+        const Int64 spanY = static_cast<Int64>(sy1) - sy0;
+        const Int64 dstSpanX = static_cast<Int64>(dx1) - dx0;
+        const Int64 dstSpanY = static_cast<Int64>(dy1) - dy0;
+        const Int64 width = spanX < 0 ? -spanX : spanX;
+        const Int64 height = spanY < 0 ? -spanY : spanY;
+        const Int64 dstWidth = dstSpanX < 0 ? -dstSpanX : dstSpanX;
+        const Int64 dstHeight = dstSpanY < 0 ? -dstSpanY : dstSpanY;
+        // A SCALE has no arm here and is not supposed to: 18.3.1 makes a multisample blit whose
+        // rectangles differ in size INVALID_OPERATION. Rule I (a), the same three lines the
+        // colour arm's multisample-destination decline uses. A ZERO span on one side only is a
+        // size mismatch too (review round 2): it used to return silently with the other side
+        // non-empty, where GL raises the error. Only when BOTH sides are empty on an axis is
+        // there nothing to do and nothing to say - that is the caller's early return.
+        if (width != dstWidth || height != dstHeight) {
+            MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a %lldx%lld source rectangle "
+                         "cannot reach a %lldx%lld destination rectangle - a multisample resolve "
+                         "carries no scale; declining the blit",
+                         static_cast<long long>(width), static_cast<long long>(height),
+                         static_cast<long long>(dstWidth), static_cast<long long>(dstHeight));
+            RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation,
+                                   "glBlitFramebuffer from a multisampled read framebuffer requires "
+                                   "identical source and destination rectangle dimensions.");
+            return true;
+        }
+        if (!width || !height) return false;
+        // WHICH MIRRORS HAVE AN ARM. The copy-out in ResolveWireDepthStencil moves the resolved
+        // aspect through a buffer, and a buffer copy addresses ROWS: one VkBufferImageCopy per
+        // row, walked backwards, is a vertical mirror for free. A HORIZONTAL one would need a
+        // region per TEXEL, which is not a copy any more - and re-encoding across formats
+        // (BlitDepthAcrossFormats, shared with the monolith arm) has no flip either. Both
+        // decline by name, each under its OWN once-site (review round 2: one site covered both,
+        // so after the first X decline a cross-format Y decline logged nothing); §6 in
+        // notes/p7/magma-b2.md carries them. These are declines of a LEGAL blit - GL would
+        // raise no error here - so the INVALID_OPERATION is rule I (a)'s named refusal, not
+        // GL's answer.
+        const Bool mirrorX = (spanX < 0) != (dstSpanX < 0);
+        const Bool mirrorY = (spanY < 0) != (dstSpanY < 0);
+        if (mirrorX) {
+            MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a multisample resolve mirrored on "
+                         "X (source format %d, destination format %d) has no arm - the copy-out "
+                         "reverses whole rows and nothing smaller; declining the blit",
+                         static_cast<Int>(source.format), static_cast<Int>(destination.format));
+            RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation,
+                                   "This device cannot mirror that multisample depth/stencil blit.");
+            return true;
+        }
+        if (mirrorY && source.format != destination.format) {
+            MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a multisample resolve mirrored on "
+                         "Y across two formats (source format %d, destination format %d) has no "
+                         "arm - the cross-format re-encode has no flip; declining the blit",
+                         static_cast<Int>(source.format), static_cast<Int>(destination.format));
+            RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation,
+                                   "This device cannot mirror that multisample depth/stencil blit.");
+            return true;
+        }
+        return false;
+    }
+
    void VulkanRenderer::ResolveWireDepthStencil(WireImage source, WireImage destination,
        GLint sx0, GLint sy0, GLint sx1, GLint sy1, GLint dx0, GLint dy0, GLint dx1, GLint dy1) {
        // P7 wave 2-B2 (review round): A REVERSED RECTANGLE IS NOT A MALFORMED ONE. This guard
@@ -443,31 +505,23 @@
        // spans - so a flip is a legal blit that has to produce a mirrored picture.
        //
        // Spans are therefore signed and everything downstream works off the min corner.
+        //
+        // THE SHAPE DECLINES ARE DECIDED ONCE PER CALL (review round 2). BlitWireFramebuffers
+        // asks DeclineWireDepthStencilResolveShape before ANY aspect runs, because the colour
+        // lambda runs first: asked only here, a scaled COLOR|DEPTH blit scaled and wrote its
+        // colour and THEN recorded INVALID_OPERATION on its depth - a partial effect GL 18.3.1
+        // forbids (the whole call errors) - and an X-mirrored one mirrored its colour and left
+        // its depth alone. The call below is the same question for a caller that did not ask;
+        // on the BlitWireFramebuffers path it has already answered false.
+        if (DeclineWireDepthStencilResolveShape(source, destination, sx0, sy0, sx1, sy1, dx0, dy0, dx1, dy1))
+            return;
        const Int64 spanX = static_cast<Int64>(sx1) - sx0;
        const Int64 spanY = static_cast<Int64>(sy1) - sy0;
-        const Int64 dstSpanX = static_cast<Int64>(dx1) - dx0;
-        const Int64 dstSpanY = static_cast<Int64>(dy1) - dy0;
-        if (!spanX || !spanY || !dstSpanX || !dstSpanY) return;
+        if (!spanX || !spanY) return; // both sides empty on an axis (the helper errored otherwise)
        const Int64 width = spanX < 0 ? -spanX : spanX;
        const Int64 height = spanY < 0 ? -spanY : spanY;
-        // A SCALE has no arm here and is not supposed to: 18.3.1 makes a multisample blit whose
-        // rectangles differ in size INVALID_OPERATION. Rule I (a), the same three lines the
-        // colour arm's multisample-destination decline uses.
-        if (width != (dstSpanX < 0 ? -dstSpanX : dstSpanX) ||
-            height != (dstSpanY < 0 ? -dstSpanY : dstSpanY)) {
-            MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a %lldx%lld source rectangle "
-                         "cannot reach a %lldx%lld destination rectangle - a multisample resolve "
-                         "carries no scale; declining the blit",
-                         static_cast<long long>(width), static_cast<long long>(height),
-                         static_cast<long long>(dstSpanX < 0 ? -dstSpanX : dstSpanX),
-                         static_cast<long long>(dstSpanY < 0 ? -dstSpanY : dstSpanY));
-            RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation,
-                                   "glBlitFramebuffer from a multisampled read framebuffer requires "
-                                   "identical source and destination rectangle dimensions.");
-            return;
-        }
-        const Bool mirrorX = (spanX < 0) != (dstSpanX < 0);
-        const Bool mirrorY = (spanY < 0) != (dstSpanY < 0);
+        // Past the helper the only mirror left is a same-format vertical one.
+        const Bool mirrorY = (spanY < 0) != (dy1 < dy0);
        const GLint sxMin = std::min(sx0, sx1), syMin = std::min(sy0, sy1);
        const GLint dxMin = std::min(dx0, dx1), dyMin = std::min(dy0, dy1);
        if (source.isDefault || destination.isDefault || source.is3D || destination.is3D ||
@@ -477,22 +531,6 @@
            static_cast<Uint64>(dxMin + width) > destination.extent.width ||
            static_cast<Uint64>(dyMin + height) > destination.extent.height)
            MagmaWireFatal("multisample-depth-resolve-region");
-        // WHICH MIRRORS HAVE AN ARM. The copy-out below moves the resolved aspect through a
-        // buffer, and a buffer copy addresses ROWS: one VkBufferImageCopy per row, walked
-        // backwards, is a vertical mirror for free. A HORIZONTAL one would need a region per
-        // TEXEL, which is not a copy any more - and re-encoding across formats
-        // (BlitDepthAcrossFormats, shared with the monolith arm) has no flip either. Both
-        // decline by name; §12 in notes/p7/magma-b2.md carries them.
-        if (mirrorX || (mirrorY && source.format != destination.format)) {
-            MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a multisample resolve mirrored on "
-                         "%s (source format %d, destination format %d) has no arm - the copy-out "
-                         "reverses whole rows and nothing smaller; declining the blit",
-                         mirrorX ? "X" : "Y across two formats",
-                         static_cast<Int>(source.format), static_cast<Int>(destination.format));
-            RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation,
-                                   "This device cannot mirror that multisample depth/stencil blit.");
-            return;
-        }
        VkImageAspectFlags formatAspects = VK_IMAGE_ASPECT_DEPTH_BIT;
        if (source.format == VK_FORMAT_S8_UINT) formatAspects = VK_IMAGE_ASPECT_STENCIL_BIT;
        else if (source.format == VK_FORMAT_D16_UNORM_S8_UINT || source.format == VK_FORMAT_D24_UNORM_S8_UINT ||
@@ -772,6 +810,33 @@
        const auto* draw = named ? state.FramebufferRecordFor(state.VerbBlitDrawFbo) : state.DrawFramebuffer();
        if (!read || !draw) MagmaWireFatal("blit-framebuffer-record");
        state.VerbBlitNamedConsumed = named;
+        // P7 wave 2-B2 (review round 2): ONE ANSWER FOR THE WHOLE CALL. The aspects below run
+        // colour first, so a multisample depth/stencil shape that declines (a scale, an X
+        // mirror, a Y mirror across two formats) used to be discovered only after the colour
+        // attachment had already been scaled or mirrored and written - the call then recorded
+        // INVALID_OPERATION with half its effect in place, which GL 18.3.1 does not allow (an
+        // erroring blit writes nothing). The shape is a fact about the rectangles and the two
+        // images, so it is asked here, before any aspect runs, for every depth/stencil aspect
+        // the mask names that would take the multisample resolve arm.
+        //
+        // A default READ framebuffer is never multisampled here, and a default DRAW one behind a
+        // multisample read is ResolveWireDepthStencil's region Fatal whatever the shape - and
+        // resolving the default draw image early would move its write index - so both are left
+        // to the lambda.
+        if ((mask & (GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT)) && !read->IsDefault && !draw->IsDefault) {
+            const auto declines = [&](const MG_Pipe::MGPSurface& src, const MG_Pipe::MGPSurface& dst,
+                                      VkImageAspectFlags aspect) -> Bool {
+                const WireImage source = ResolveWireImage(*read, src, aspect, /*isWriteTarget=*/false);
+                if (!source.image || source.samples == VK_SAMPLE_COUNT_1_BIT) return false;
+                const WireImage destination = ResolveWireImage(*draw, dst, aspect, /*isWriteTarget=*/true);
+                if (!destination.image || destination.samples != VK_SAMPLE_COUNT_1_BIT) return false;
+                return DeclineWireDepthStencilResolveShape(source, destination, sx0, sy0, sx1, sy1,
+                                                           dx0, dy0, dx1, dy1);
+            };
+            if (((mask & GL_DEPTH_BUFFER_BIT) && declines(read->Depth, draw->Depth, VK_IMAGE_ASPECT_DEPTH_BIT)) ||
+                ((mask & GL_STENCIL_BUFFER_BIT) && declines(read->Stencil, draw->Stencil, VK_IMAGE_ASPECT_STENCIL_BIT)))
+                return;
+        }
        const auto blit = [&](const MG_Pipe::MGPSurface& src, const MG_Pipe::MGPSurface& dst,
                              VkImageAspectFlags aspect) {
            WireImage source = ResolveWireImage(*read,src,aspect, /*isWriteTarget=*/false);
@@ -3228,6 +3228,20 @@ if (MOBILEGL_BUILD_DISAGGREGATED)
            PROPERTIES LABELS "${mglItestMagmaLabels}" TIMEOUT ${MGL_ITEST_TIMEOUT}
                ${mglItestArmProperties}
                ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-msflip${mglItestArmLogTag}.log")
+        # ITS KNOB TWIN (review round 2). Knob-free, lavapipe answers every resolve through the
+        # VK_KHR_depth_stencil_resolve arm, so the shader arm's min-corner rectangle under a
+        # flip - the device WITHOUT the extension - never ran. MsFlip1 forces it. Split+spawn
+        # only, the server-side-knob reason above, named in spawn_lane_parity.py's
+        # MAGMA_SERVER_ENV_KNOB_NO_TCP; the knob-free MsFlip. keeps all three arms.
+        if(mglItestServerEnvKnobArm)
+            gtest_discover_tests(MobileGLIntegrationTest
+                TEST_PREFIX "DirectVulkan.${mglItestArm}.MsFlip1."
+                TEST_FILTER "DepthStencilReadbackMatrixScenario.AFlippedMultisampleResolveMirrorsTheBandsAndAScaleDeclines"
+                DISCOVERY_TIMEOUT 30
+                PROPERTIES LABELS "${mglItestMagmaLabels}" TIMEOUT ${MGL_ITEST_TIMEOUT}
+                    ${mglItestArmProperties}
+                    ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE=1\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-msflip${mglItestArmLogTag}-shader1.log")
+        endif()
        foreach(inPlaceCase CopyImageInPlaceAcrossLevelsAndLayers
                            CopyImageInPlaceOverlapDeclinesAndLeavesTheLevelAlone)
            gtest_discover_tests(MobileGLIntegrationTest
@@ -566,6 +566,45 @@ namespace MGITest {
                       "the declined scale must have left the destination alone");
        EXPECT_EQ(FirstGLError(), 0u) << "the session survived the decline";

+        // THE SAME SCALE WITH COLOUR IN THE MASK. An erroring blit writes nothing (18.3.1), so
+        // the colour attachment must come through untouched too. The colour arm CAN scale a
+        // multisample resolve on its own, and it runs before the depth arm - so a decline
+        // decided per aspect scaled and wrote the colour and then raised the error on the
+        // depth. The shape is decided once, before any aspect, and this is what pins it.
+        glBindFramebuffer(GL_FRAMEBUFFER, resolved.fbo);
+        ClearDepthStencil(GL_DEPTH24_STENCIL8, 0.5f, kPrimeStencil);
+        glClearColor(0, 0, 1, 1);
+        glClear(GL_COLOR_BUFFER_BIT);
+        ASSERT_EQ(FirstGLError(), 0u);
+        glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled.fbo);
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolved.fbo);
+        glBlitFramebuffer(0, 0, kWidth, kHeight, 0, 0, kWidth / 2, kHeight / 2,
+                          GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT, GL_NEAREST);
+        glFinish();
+        EXPECT_EQ(FirstGLError(), GLenum(GL_INVALID_OPERATION))
+            << "a scaled multisample COLOR|DEPTH blit is INVALID_OPERATION as a whole";
+        glBindFramebuffer(GL_FRAMEBUFFER, resolved.fbo);
+        std::array<GLubyte, 4> untouched{};
+        glReadPixels(1, 1, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, untouched.data());
+        EXPECT_EQ(untouched, (std::array<GLubyte, 4>{0, 0, 255, 255}))
+            << "the declined COLOR|DEPTH scale must not have written its colour either";
+        ExpectAllDepth(ReadDepthFloat(0, 0, kWidth, kHeight), 0.5f,
+                       "the declined COLOR|DEPTH scale must have left the depth alone");
+        EXPECT_EQ(FirstGLError(), 0u) << "the session survived the combined decline";
+
+        // AN EMPTY DESTINATION BEHIND A NON-EMPTY SOURCE is a size mismatch as well, not a
+        // no-op: 64x48 cannot reach 0x48.
+        glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled.fbo);
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolved.fbo);
+        glBlitFramebuffer(0, 0, kWidth, kHeight, 0, 0, 0, kHeight, GL_DEPTH_BUFFER_BIT, GL_NEAREST);
+        glFinish();
+        EXPECT_EQ(FirstGLError(), GLenum(GL_INVALID_OPERATION))
+            << "a multisample resolve onto an empty destination rectangle is a size mismatch";
+        glBindFramebuffer(GL_FRAMEBUFFER, resolved.fbo);
+        ExpectAllDepth(ReadDepthFloat(0, 0, kWidth, kHeight), 0.5f,
+                       "the empty-destination decline must have left the depth alone");
+        EXPECT_EQ(FirstGLError(), 0u) << "the session survived the empty-destination decline";
+
        DestroySource(resolved);
        DestroySource(multisampled);
        glBindFramebuffer(GL_FRAMEBUFFER, 0);
@@ -76,8 +76,10 @@ MAGMA_INPROC_ONLY = ("MagmaRunAheadScenario.", "MagmaWireCacheScenario.")
 # P7 wave 2-B3 (ID-P7-34) adds `.StaleSerial.`: MGITEST_MAGMA_FORCE_STALE_BUFFER_SERIAL is read by
 # the server too (VkBufferManager.cpp), so the streamed subdata-then-draw red-once has the same
 # two arms and the same tcp absence. One list, one mechanism, for every server-side knob.
+# `.MsFlip1.` (B2 review round 2) is the shader-resolve knob twin of the knob-free `.MsFlip.`,
+# which keeps its tcp entry.
 MAGMA_SERVER_ENV_KNOB_NO_TCP = (".ShaderMip1.", ".ShaderMip2.", ".DepthMip.",
-                                ".DefaultBlitShape1.", ".MsResolve1.", ".StaleSerial.")
+                                ".DefaultBlitShape1.", ".MsResolve1.", ".StaleSerial.", ".MsFlip1.")


 def lane_names(build_dir, label):
@@ -130,6 +132,20 @@ def compare_arms(build_dir, tier, labels, inproc_only=(), no_tcp=()):
              f"from the comparison by name ({', '.join(inproc_only)})")
    if no_tcp:
        dropped = {k for k in comparable if any(only in k for only in no_tcp)}
+        # EVERY TAIL MUST NAME SOMETHING (review round 2). A tail that matches no split key is
+        # silently inert - a renamed prefix would leave the exception in the list and the
+        # entry it was about riding under some other name - so an unmatched tail is an error,
+        # and so is a tail that suddenly matches a different number of entries than the one
+        # knob case it was written for.
+        for tail in no_tcp:
+            hits = sorted(k for k in comparable if tail in k)
+            if len(hits) != 1:
+                print(f"::error::{tier}: the server-env-knob tail {tail!r} matches {len(hits)} "
+                      f"split entrie(s) {hits}, not exactly one. Each tail names ONE knob case; a "
+                      f"tail that names none is an exception about nothing, and one that names "
+                      f"more has let a second case ride off the tcp arm unexamined.",
+                      file=sys.stderr)
+                return True
        if dropped:
            print(f"{tier}: {len(dropped)} server-env-knob entrie(s) excluded from the TCP "
                  f"comparison only ({', '.join(no_tcp)}) - the knob is read on the server and "