From a6e029734bc1d95567ee2e7d2848583ac0f82226 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 13:57:32 -0400 Subject: [PATCH] [Test, Doc] (MG_IntegrationTest, MG_Backend/DirectGLES): review wave - stop the skip paths leaking their GL objects, normalise the clip enables a neighbouring scenario leaves behind, and state what the software lanes cannot falsify --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 18 +++++++ .../Scenarios/ClipDistanceScenario.cpp | 54 ++++++++++++++----- ...StencilReadbackAttachmentShapeScenario.cpp | 8 +++ 3 files changed, 68 insertions(+), 12 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 418ae89c..78215b41 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -6168,6 +6168,14 @@ namespace MobileGL::MG_Backend::DirectGLES { // stay in the list behind it, so a wrong first answer costs one rejected blit // instead of the whole readback. The probe's own refusal must not be left on // the error queue for the caller's next glGetError to pick up as its own. + // + // The cost of keeping the fallbacks: a source whose OWN format cannot back a + // staging texture (GL_STENCIL_INDEX8 without EXT/OES_texture_stencil8, say) no + // longer fails cleanly - it retries with a packed format, and a driver lax + // enough to accept the resulting mismatched depth/stencil blit would hand back + // data indistinguishable from a correct read. ES conformance forbids that blit, + // so this trades a spec-guaranteed rejection for a driver-bug-only wrong answer; + // the shapes it rescues (array and cube attachments) are otherwise unreadable. const GLenum exact = ReplicateBlitImpl::QueryAttachmentSizedFormat(point); ClearGLErrors(); if (exact != 0) { @@ -7696,6 +7704,16 @@ namespace MobileGL::MG_Backend::DirectGLES { } if (depthBits <= 0 && stencilBits <= 0) { + // Nothing usable came back from either witness. Returning here leaves whatever a + // PREVIOUS surface published in place, which would be stale - this function runs + // once per surface activation, not once per process. It is written this way anyway + // because the branch is unreachable for a surface MobileGL chose itself: + // InitDisplayAndContext asks eglChooseConfig for EGL_DEPTH_SIZE 24 and + // EGL_STENCIL_SIZE 8, and so does its alpha-free retry, so g_Config always has both + // and the EGL fallback above always answers. A caller that supplies its own + // depth-less config would keep the previous surface's description; publishing a + // guess instead would be a different lie, and the placeholder attachment model has + // no way to say "this buffer does not exist" short of detaching it. MGLOG_D("DirectGLES: default framebuffer reports no depth or stencil; leaving the " "placeholder attachment formats untouched"); return; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ClipDistanceScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ClipDistanceScenario.cpp index 790d399e..15ccbfc1 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ClipDistanceScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ClipDistanceScenario.cpp @@ -25,6 +25,22 @@ // actual claim. The disabled case is the negative control - the identical shader with the // identical distances and the enable turned off must leave both sides painted, which is what // says the pixels below are being removed by clipping and not by something else. +// +// HONEST LIMIT OF THIS FILE IN CI. Of the four cases, only EnableIsObservableThroughIsEnabled is +// falsifiable on the software rasterizers every automated lane runs on. llvmpipe and lavapipe +// clip by EVERY declared gl_ClipDistance regardless of the enables, so +// AnEnabledClipDistanceRemovesTheNegativeHalf goes green there against the broken tree as well, +// and the two cases that need real per-distance semantics skip (see +// DriverHonoursPerDistanceEnables). What actually pins the behaviour is Adreno, through +// KHR-GLxx.clip_distance.functional - whose "without dynamic redeclaration" variants declare all +// gl_MaxClipDistances slots and enable only the first N, i.e. exactly the subset semantics these +// skipped cases assert. Read a green CI run here as "the state survives the frontend", not as +// "clipping is correct"; the second claim is a device claim. +// +// Every case disables all eight distances on entry rather than assuming they start off: +// XfbAfterClipDistanceScenario deliberately leaves one enabled for the rest of the process, and +// forwarding the enables is what turned that leftover from inert bookkeeping into live driver +// state. #include #include @@ -131,9 +147,16 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } return px[0] > 192 && px[1] < 64; } - unsigned char PixelAt(int x, int y, unsigned char* out) const { + void PixelAt(int x, int y, unsigned char* out) const { glReadPixels(x, y, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, out); - return out[0]; + } + + // Never assume the eight start disabled - see the header note about + // XfbAfterClipDistanceScenario leaving one on for the rest of the process. + static void DisableEveryClipDistance() { + for (int i = 0; i < 8; ++i) { + glDisable(static_cast(GL_CLIP_DISTANCE0 + i)); + } } // True when the driver under this backend actually implements PER-DISTANCE enable @@ -183,7 +206,9 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } if (!Ready()) return; HeadlessGL& gl = Gl(); - EXPECT_EQ(glIsEnabled(GL_CLIP_DISTANCE0), GL_FALSE) << "GL_CLIP_DISTANCE0 must start disabled"; + DisableEveryClipDistance(); + EXPECT_EQ(glIsEnabled(GL_CLIP_DISTANCE0), GL_FALSE) + << "glDisable(GL_CLIP_DISTANCE0) is not observable through glIsEnabled"; glEnable(GL_CLIP_DISTANCE0); EXPECT_EQ(FirstGLError(), 0u); EXPECT_EQ(glIsEnabled(GL_CLIP_DISTANCE0), GL_TRUE) @@ -222,6 +247,9 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } glDisable(GL_CULL_FACE); glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + // Distance 1 is positive by a single pixel at the sampled row, so a stray enable on it + // would put the "kept" probe right on the clip boundary. + DisableEveryClipDistance(); glEnable(GL_CLIP_DISTANCE0); DrawClippedTriangle(program, vao); EXPECT_EQ(FirstGLError(), 0u); @@ -268,8 +296,7 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } glDisable(GL_SCISSOR_TEST); glDisable(GL_DEPTH_TEST); glDisable(GL_CULL_FACE); - glDisable(GL_CLIP_DISTANCE0); - glDisable(GL_CLIP_DISTANCE1); + DisableEveryClipDistance(); DrawClippedTriangle(program, vao); EXPECT_EQ(FirstGLError(), 0u); @@ -281,18 +308,19 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } EXPECT_EQ(FirstGLError(), 0u); EXPECT_TRUE(IsGreen(right)) << "with every clip distance disabled the whole triangle must survive"; - if (!IsGreen(left)) { - GTEST_SKIP() << "renderer " << gl.RendererString() - << " clips by a DISABLED gl_ClipDistance - it does not implement per-distance enable state " - "(see DriverHonoursPerDistanceEnables). Emulating GL's semantics there needs shader-side " - "masking keyed on the enable mask, which is a separate feature"; - } + const bool driverHonoursEnables = IsGreen(left); glUseProgram(0); glBindVertexArray(0); glDeleteProgram(program); glDeleteVertexArrays(1, &vao); gl.EndFrame(); + if (!driverHonoursEnables) { + GTEST_SKIP() << "renderer " << gl.RendererString() + << " clips by a DISABLED gl_ClipDistance - it does not implement per-distance enable state " + "(see DriverHonoursPerDistanceEnables). Emulating GL's semantics there needs shader-side " + "masking keyed on the enable mask, which is a separate feature"; + } } // The eight enables are independent: enabling only distance 1 must clip by distance 1 and @@ -322,12 +350,14 @@ void main() { fragColor = vec4(0.0, 1.0, 0.0, 1.0); } glBindVertexArray(0); glDeleteProgram(program); glDeleteVertexArrays(1, &vao); + DisableEveryClipDistance(); + gl.EndFrame(); GTEST_SKIP() << "renderer " << gl.RendererString() << " clips by every declared gl_ClipDistance regardless of the enables, so per-distance " "independence is not observable here"; } - glDisable(GL_CLIP_DISTANCE0); + DisableEveryClipDistance(); glEnable(GL_CLIP_DISTANCE1); DrawClippedTriangle(program, vao); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackAttachmentShapeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackAttachmentShapeScenario.cpp index 9d170590..d36be1b1 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackAttachmentShapeScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackAttachmentShapeScenario.cpp @@ -342,6 +342,14 @@ namespace MGITest { // depth/stencil claim below falsifiable instead of drowning it in an unrelated // harness limitation. if (int(color[1]) <= 192) { + // GTEST_SKIP() expands to a return, so the teardown below it would never run and this + // scenario would hand the next one a foreign framebuffer plus three leaked objects - + // and this is the path DirectVulkan takes on every headless run, not a rare one. + BindDefaultFramebuffer(); + glDeleteFramebuffers(1, &fbo); + glDeleteRenderbuffers(1, &colorRbo); + glDeleteRenderbuffers(1, &depthRbo); + gl.EndFrame(); GTEST_SKIP() << "backend " << gl.BackendName() << " on this surface transferred no colour either (green=" << int(color[1]) << "): it cannot blit out of the default framebuffer here, so the depth/stencil half proves "