From b6d63163331c88becfadf8023d87dd77a12ae241 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 10:51:59 -0400 Subject: [PATCH] [Fix] (Blend): decline a GL_SRC1_* factor on an incapable GLES driver even when blending is off --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 45 ++++++++---- .../MG_Test/Framebuffer/FramebufferTest.cpp | 71 +++++++++++++++++++ 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 4472120b..34ba243b 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1957,33 +1957,52 @@ namespace MobileGL::MG_Backend::DirectGLES { // against stale state), and throwing, which is what this did until now, takes the // whole process down over one unsupported blend factor. Declining is defined, // survivable and visible in the log. + // + // NOT gated on Enabled, deliberately, and the same way the Vulkan twin is not gated + // on effectiveBlendEnabled: what has to be kept away from the driver is the FACTOR + // ENUM, and the factor push below never consults Enabled - one glBlendFuncSeparate + // serves every draw buffer when they agree, and the per-index arm diffs factors + // alone. So `glDisable(GL_BLEND); glBlendFunc(GL_SRC1_ALPHA, ...)` followed by any + // draw OR clear would otherwise hand a GL_SRC1_ALPHA to a driver that answers + // GL_INVALID_ENUM, leaving a spurious error in ITS queue for the next internal + // no-error probe to read as its own, and leaving this shadow recording factors the + // ES context rejected. Blending being off makes the picture unaffected; it does not + // make the enum acceptable. const auto* effectiveBlendStates = ¶meters.BlendStates; if (!g_GLESCapabilities.SupportsDualSourceBlend) { + Uint32 declinedWithBlendingOnMask = 0; for (Uint i = 0; i < FBO::MAX_DRAW_BUFFERS; ++i) { const auto& s = parameters.BlendStates[i]; - if (s.Enabled && - (IsDualSourceBlendFactor(s.SrcFactorRGB) || IsDualSourceBlendFactor(s.DstFactorRGB) || - IsDualSourceBlendFactor(s.SrcFactorAlpha) || IsDualSourceBlendFactor(s.DstFactorAlpha))) { + if (IsDualSourceBlendFactor(s.SrcFactorRGB) || IsDualSourceBlendFactor(s.DstFactorRGB) || + IsDualSourceBlendFactor(s.SrcFactorAlpha) || IsDualSourceBlendFactor(s.DstFactorAlpha)) { dualSourceDeclinedMask |= 1u << i; + if (s.Enabled) declinedWithBlendingOnMask |= 1u << i; } } if (dualSourceDeclinedMask != 0) { + // Two masks in the message because they mean different things to whoever + // reads the log: the second one is where a PICTURE was lost. A draw buffer + // in the first mask but not the second had blending off anyway, so nothing + // was blended and nothing was dropped - only the unusable enum was kept out + // of the driver. MGLOG_E_ONCE( - "SyncRenderState: dual-source blending (GL_SRC1_* blend factor) was requested on " - "draw buffer mask 0x%x, but the GLES driver does not expose " - "GL_EXT_blend_func_extended (see the dual-source blend row in the driver POST). " - "Blending is DECLINED on those draw buffers - the fragment's first output is " - "written unblended and the second source is dropped.", - dualSourceDeclinedMask); + "SyncRenderState: a GL_SRC1_* (dual-source) blend factor was set on draw buffer " + "mask 0x%x, but the GLES driver does not expose GL_EXT_blend_func_extended (see " + "the dual-source blend row in the driver POST). Those draw buffers are pushed " + "with neutral One/Zero factors instead. Blending was actually ENABLED on mask " + "0x%x, and only there is anything lost: the fragment's first output is written " + "unblended and the second source is dropped.", + dualSourceDeclinedMask, declinedWithBlendingOnMask); g_dualSourceDeclinedBlendStates = parameters.BlendStates; for (Uint i = 0; i < FBO::MAX_DRAW_BUFFERS; ++i) { if ((dualSourceDeclinedMask & (1u << i)) == 0) continue; auto& s = g_dualSourceDeclinedBlendStates[i]; + // Both halves, for the same reason the Vulkan arm neutralises both: the + // enable so nothing blends against a source the driver cannot produce, + // the factors so no GL_SRC1_* enum is ever handed over. Clearing Enabled + // on a buffer that was already off is a no-op, which is what makes one + // ungated rule serve both cases. s.Enabled = false; - // Neutral factors as well as the disable: the factor push below is not - // gated on Enabled (one glBlendFuncSeparate serves every draw buffer when - // they agree), so leaving Src1Color here would still hand the driver a - // GL_SRC1_* enum it cannot parse. s.SrcFactorRGB = BlendFactor::One; s.DstFactorRGB = BlendFactor::Zero; s.SrcFactorAlpha = BlendFactor::One; diff --git a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp index 10d90a9f..96a072a3 100644 --- a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp +++ b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp @@ -1382,6 +1382,77 @@ TEST_F(FramebufferTest, DualSourceBlendIsDeclinedRatherThanThrownWhenTheExtensio EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); } +// The half the first version of the decline missed: the FACTOR push is not gated on Enabled, so +// GL_BLEND being OFF does not keep a GL_SRC1_* enum away from a driver that cannot parse it. This +// is the sequence - `glDisable(GL_BLEND); glBlendFunc(GL_SRC1_ALPHA, ...)` then any draw or clear - +// and it needs no dual-source shader at all, which is why it survived both the enabled-path unit +// case above and the integration scenario (that one skips on exactly the extension-less lanes this +// concerns, because its probe needs a dual-source program to render). +// +// What a leaked enum costs: the driver answers GL_INVALID_ENUM and keeps its previous factors, so +// the error sits in the ES context's own queue for the next internal `glGetError() == GL_NO_ERROR` +// probe to read as its own failure, and this backend's shadow records factors the context rejected. +TEST_F(FramebufferTest, DualSourceFactorsAreDeclinedEvenWithBlendingDisabled) { + ScopedRenderStateDriverStubs driver(/*dualSourceBlendSupported=*/false); + + MG_Impl::GLImpl::Disable(GL_BLEND); + MG_Impl::GLImpl::BlendFunc(GL_SRC1_ALPHA, GL_ONE_MINUS_SRC1_ALPHA); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + ResetRecordedBlend(); + + ASSERT_NO_THROW(MG_Backend::DirectGLES::RenderStateImpl::SyncRenderState(/*forColorClear=*/false)); + + for (Uint i = 0; i < kRecordedDrawBuffers; ++i) { + EXPECT_FALSE(g_driverBlend[i].enabled) << "draw buffer " << i << ": blending was never enabled"; + EXPECT_NE(g_driverBlend[i].srcRGB, static_cast(GL_SRC1_ALPHA)) + << "draw buffer " << i + << ": a GL_SRC1_* enum must not reach a driver without the extension even with GL_BLEND off"; + EXPECT_NE(g_driverBlend[i].dstRGB, static_cast(GL_ONE_MINUS_SRC1_ALPHA)) << "draw buffer " << i; + EXPECT_NE(g_driverBlend[i].srcAlpha, static_cast(GL_SRC1_ALPHA)) << "draw buffer " << i; + EXPECT_NE(g_driverBlend[i].dstAlpha, static_cast(GL_ONE_MINUS_SRC1_ALPHA)) << "draw buffer " << i; + } + + // A clear reaches the same block by the same route (SyncRenderState(forColorClear=true)), and + // the flag only steers the alpha-widen colour mask, so it must not reopen this either. + MG_Impl::GLImpl::BlendFunc(GL_SRC1_COLOR, GL_ONE_MINUS_SRC1_COLOR); + ResetRecordedBlend(); + ASSERT_NO_THROW(MG_Backend::DirectGLES::RenderStateImpl::SyncRenderState(/*forColorClear=*/true)); + for (Uint i = 0; i < kRecordedDrawBuffers; ++i) { + EXPECT_NE(g_driverBlend[i].srcRGB, static_cast(GL_SRC1_COLOR)) << "draw buffer " << i; + EXPECT_NE(g_driverBlend[i].dstRGB, static_cast(GL_ONE_MINUS_SRC1_COLOR)) << "draw buffer " << i; + } + + // And the shadow records what was PUSHED, not what the frontend holds - otherwise the next + // switch to an ordinary factor diffs against state the ES context never received. + MG_Impl::GLImpl::Enable(GL_BLEND); + MG_Impl::GLImpl::BlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA); + ResetRecordedBlend(); + MG_Backend::DirectGLES::RenderStateImpl::SyncRenderState(/*forColorClear=*/false); + ASSERT_TRUE(g_driverBlend[0].factorsSeen); + EXPECT_TRUE(g_driverBlend[0].enabled) << "the enable has to be pushed - the shadow said 'off' because it was"; + EXPECT_EQ(g_driverBlend[0].srcRGB, static_cast(GL_SRC_ALPHA)); + EXPECT_EQ(g_driverBlend[0].dstRGB, static_cast(GL_ONE_MINUS_SRC_ALPHA)); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + +// The capable driver is unaffected by the ungating: GL_BLEND off with SRC1 factors set is a state +// an application may legitimately hold, and the factors still have to reach a driver that parses +// them - otherwise the next glEnable(GL_BLEND) would blend against neutralised state. +TEST_F(FramebufferTest, DualSourceFactorsWithBlendingDisabledStillReachACapableDriver) { + ScopedRenderStateDriverStubs driver(/*dualSourceBlendSupported=*/true); + + MG_Impl::GLImpl::Disable(GL_BLEND); + MG_Impl::GLImpl::BlendFunc(GL_SRC1_ALPHA, GL_ONE_MINUS_SRC1_ALPHA); + ResetRecordedBlend(); + MG_Backend::DirectGLES::RenderStateImpl::SyncRenderState(/*forColorClear=*/false); + + ASSERT_TRUE(g_driverBlend[0].factorsSeen); + EXPECT_FALSE(g_driverBlend[0].enabled); + EXPECT_EQ(g_driverBlend[0].srcRGB, static_cast(GL_SRC1_ALPHA)); + EXPECT_EQ(g_driverBlend[0].dstRGB, static_cast(GL_ONE_MINUS_SRC1_ALPHA)); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + // --- glFramebufferTexture error conditions (GL 4.6 core 9.2.8) --------------------------------- // // Four of them were missing from the bound-target path while its DSA sibling