From e310e3e9ff78bc0393188e3ed8db0ed82cc7ca26 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 01:08:11 -0400 Subject: [PATCH] [Fix, Test] (MG_Util, MG_Backend/DirectGLES, MG_Test): resolve buffer textures through the entry point the tier ships, not the ES 3.2 core name; bound the OES retarget to an exact extension name --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 61 +++++- MobileGL/MG_Backend/DirectGLES/DirectGLES.h | 8 + MobileGL/MG_Backend/DirectGLES/Managers.cpp | 28 +-- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 26 ++- .../Scenarios/BufferTextureScenario.cpp | 4 + .../Backend/DirectGLES/EsslShaderPassTest.cpp | 27 +++ .../BackendLoader/BackendLoaderTest.cpp | 180 ++++++++++++++++++ .../MG_Util/BackendLoaders/OpenGL/Loader.cpp | 37 +++- .../MG_Util/BackendLoaders/OpenGL/Loader.h | 14 ++ MobileGL/MG_Util/SelfTest/DriverPost.cpp | 55 +++--- .../ShaderTranspiler/ShaderCompiler.cpp | 18 +- .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 7 +- 12 files changed, 409 insertions(+), 56 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 0a6b2b95..c07a7e2a 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -7233,13 +7233,66 @@ namespace MobileGL::MG_Backend::DirectGLES { g_GLESFuncs.glGetQueryObjectui64vEXT; } + namespace { + // The entry point the resolved tier's support ships, or null when there is none. + MG_External::GLES::glTexBuffer_PTR ResolveTexBufferEntryPoint() { + using Tier = MG_External::GLESCapabilities::TextureBufferTier; + switch (g_GLESCapabilities.TextureBufferSupport) { + case Tier::ExtensionEXT: + return g_GLESFuncs.glTexBufferEXT ? g_GLESFuncs.glTexBufferEXT : g_GLESFuncs.glTexBuffer; + case Tier::ExtensionOES: + return g_GLESFuncs.glTexBufferOES ? g_GLESFuncs.glTexBufferOES : g_GLESFuncs.glTexBuffer; + case Tier::CoreEs32: + return g_GLESFuncs.glTexBuffer; + case Tier::None: + default: + return nullptr; + } + } + + MG_External::GLES::glTexBufferRange_PTR ResolveTexBufferRangeEntryPoint() { + using Tier = MG_External::GLESCapabilities::TextureBufferTier; + switch (g_GLESCapabilities.TextureBufferSupport) { + case Tier::ExtensionEXT: + return g_GLESFuncs.glTexBufferRangeEXT ? g_GLESFuncs.glTexBufferRangeEXT + : g_GLESFuncs.glTexBufferRange; + case Tier::ExtensionOES: + return g_GLESFuncs.glTexBufferRangeOES ? g_GLESFuncs.glTexBufferRangeOES + : g_GLESFuncs.glTexBufferRange; + case Tier::CoreEs32: + return g_GLESFuncs.glTexBufferRange; + case Tier::None: + default: + return nullptr; + } + } + } // namespace + Bool AreBufferTexturesSupported() { - // The tier already folds in the resolved-pointer requirement (see FillInGLESCapabilities), - // but the pointer is re-checked here because the tier is only meaningful once the - // capabilities have been filled in, and callers may run before that. + // Both halves matter. The tier is what the driver ADVERTISES, and it is only meaningful + // once the capabilities have been filled in; the resolved pointer is what MobileGL can + // actually call, through the spelling that tier's support ships. Gating on the + // unsuffixed name alone would call an entry point an EXT/OES driver never exported. return g_GLESCapabilities.TextureBufferSupport != MG_External::GLESCapabilities::TextureBufferTier::None && - g_GLESFuncs.glTexBuffer != nullptr; + ResolveTexBufferEntryPoint() != nullptr; + } + + void CallTexBuffer(GLenum target, GLenum internalFormat, GLuint buffer) { + MG_External::GLES::glTexBuffer_PTR entryPoint = ResolveTexBufferEntryPoint(); + if (entryPoint == nullptr) { + return; + } + entryPoint(target, internalFormat, buffer); + } + + Bool CallTexBufferRange(GLenum target, GLenum internalFormat, GLuint buffer, GLintptr offset, GLsizeiptr size) { + MG_External::GLES::glTexBufferRange_PTR entryPoint = ResolveTexBufferRangeEntryPoint(); + if (entryPoint == nullptr) { + return false; + } + entryPoint(target, internalFormat, buffer, offset, size); + return true; } const char* GetBufferTextureTierName() { diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.h b/MobileGL/MG_Backend/DirectGLES/DirectGLES.h index 7b315002..bff29e7b 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.h +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.h @@ -127,6 +127,14 @@ namespace MobileGL::MG_Backend::DirectGLES { // Human-readable name of the buffer-texture tier for diagnostics and the driver POST: // "core (ES 3.2)", "GL_EXT_texture_buffer", "GL_OES_texture_buffer" or "unsupported". const char* GetBufferTextureTierName(); + // glTexBuffer / glTexBufferRange through whichever spelling this driver's buffer-texture + // support actually ships: the unsuffixed names are ES 3.2 core, while an EXT/OES driver + // exports glTexBuffer{,Range}EXT / OES. Callers must have checked + // AreBufferTexturesSupported() first. CallTexBufferRange reports whether it could honour + // the range - no tier is required to expose the range form, and the whole-buffer form is + // the documented fallback. + void CallTexBuffer(GLenum target, GLenum internalFormat, GLuint buffer); + Bool CallTexBufferRange(GLenum target, GLenum internalFormat, GLuint buffer, GLintptr offset, GLsizeiptr size); // GL timer-query objects, backed by GL_EXT_disjoint_timer_query. The // creators return null (the frontend then falls back to an immediately // available zero result) when the calling thread does not own the ES diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index bba3d1ff..bb868dc6 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -2812,17 +2812,19 @@ namespace MobileGL::MG_Backend::DirectGLES { // is absent). const SizeT rangeOffset = textureBufferObject->GetBufferRangeOffset(); const SizeT rangeSize = textureBufferObject->GetBufferRangeSizeInBytes(); + // Through CallTexBuffer/CallTexBufferRange rather than g_GLESFuncs directly: + // the unsuffixed entry points are the ES 3.2 core spelling, and a driver + // whose buffer textures come from EXT/OES_texture_buffer exports the + // suffixed ones instead. The dispatchers pick whichever this tier ships. if (rangeOffset == 0 && rangeSize == buffer->GetSize()) { - g_GLESFuncs.glTexBuffer(GL_TEXTURE_BUFFER, glInternalFormat, backendId); - } else if (g_GLESFuncs.glTexBufferRange != nullptr) { - g_GLESFuncs.glTexBufferRange(GL_TEXTURE_BUFFER, glInternalFormat, backendId, - static_cast(rangeOffset), - static_cast(rangeSize)); - } else { - MGLOG_E("Texture buffer %u names a sub-range but the driver has no " + CallTexBuffer(GL_TEXTURE_BUFFER, glInternalFormat, backendId); + } else if (!CallTexBufferRange(GL_TEXTURE_BUFFER, glInternalFormat, backendId, + static_cast(rangeOffset), + static_cast(rangeSize))) { + MGLOG_I("Texture buffer %u names a sub-range but the driver has no " "glTexBufferRange; binding the whole buffer instead", stateTextureObject->GetExternalIndex()); - g_GLESFuncs.glTexBuffer(GL_TEXTURE_BUFFER, glInternalFormat, backendId); + CallTexBuffer(GL_TEXTURE_BUFFER, glInternalFormat, backendId); } DebugImpl::ErrorLopper::Loop( [file = __FILE__, line = __LINE__, func = __func__, glInternalFormat, backendId](GLenum err) { @@ -4442,11 +4444,11 @@ namespace MobileGL::MG_Backend::DirectGLES { source = result; - // First in the chain because it is the only header-level rewrite: it edits - // #extension directives and never the body, so it is independent of every pass - // below and running it early keeps the directive block correct for - // ForceSupporterOutput, which scans for the last #extension line to decide where - // its precision statements go. + // Position in the chain is arbitrary: this is the only header-level rewrite, it + // edits #extension directives and never the body, and the replacement is the + // same length and stays an #extension line - so it commutes with every pass + // below, including ForceSupporterOutput's scan for the last directive. First, + // because a header concern reads better before the body ones. source = RetargetTextureBufferExtension(std::move(source), g_GLESCapabilities.TextureBufferSupport); diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index ed534f71..0456e53a 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -448,6 +448,10 @@ namespace MobileGL::MG_Backend::DirectGLES { // advertise is a hard compile error - so on an OES-only driver the emitted shader // fails to compile for the sake of one token. // + // Line comments are excluded by the directive check below; a `#extension` line inside + // a /* */ block is not, and would be rewritten. That is harmless (it stays a comment) + // and is not worth a preprocessor-aware scan here. + // // Deliberately a directive rewrite and nothing more. The alternative - teaching the // SPIR-V to stop asking for the extension - is not available: the requirement is // synthesized by SPIRV-Cross from the image type itself, not carried in the module, @@ -464,9 +468,17 @@ namespace MobileGL::MG_Backend::DirectGLES { static_assert(sizeof("GL_OES_texture_buffer") - 1 == kExtNameLength, "the two spellings must be the same length for the in-place replace"); - // Only rewrite the name where it is the subject of an #extension directive. The same - // token can legitimately appear in a comment SPIRV-Cross carried through, and a - // shader that merely mentions the string must not be edited. + // Only rewrite the name where it is the whole subject of an #extension directive. + // Two separate guards, both load-bearing: + // * the directive check, so a line-comment mentioning the name is left alone; + // * the identifier-boundary check, because GL_EXT_texture_buffer is a PREFIX of + // GL_EXT_texture_buffer_object - a different, real extension that SPIRV-Cross + // emits from the same `case DimBuffer:` on its legacy-desktop branch. Without + // the boundary this pass would silently rewrite a request for that extension + // into a request for a GL_OES_texture_buffer_object that does not exist. + const auto isIdentifierChar = [](char c) { + return std::isalnum(static_cast(c)) != 0 || c == '_'; + }; SizeT searchFrom = 0; while (true) { const SizeT hit = glslCode.find(kExtName, searchFrom); @@ -475,6 +487,14 @@ namespace MobileGL::MG_Backend::DirectGLES { } searchFrom = hit + kExtNameLength; + // Identifier boundary on both sides, so the name is not a fragment of a longer one. + if (hit > 0 && isIdentifierChar(glslCode[hit - 1])) { + continue; + } + if (hit + kExtNameLength < glslCode.size() && isIdentifierChar(glslCode[hit + kExtNameLength])) { + continue; + } + // Walk back to the start of the line and require that it is an #extension // directive, allowing whitespace between '#' and the keyword. SizeT lineStart = glslCode.rfind('\n', hit); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp index 47d3d3cf..9079a0c8 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp @@ -120,6 +120,10 @@ void main() { o_color = vec4(float(vFace) / 255.0, 0.0, 0.0, 1.0); } std::vector texels(64, 0); texels[0] = kInitial; + // The harness shares one context across every scenario in the process, so an error left + // by an earlier one would surface below as "glTexBuffer was refused". + FirstGLError(); + GLuint buffer = 0; glGenBuffers(1, &buffer); glBindBuffer(GL_TEXTURE_BUFFER, buffer); diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index a8246cb5..b0e3a487 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -325,6 +325,33 @@ void main() EXPECT_EQ(RetargetTextureBufferExtension(source, Tier::ExtensionOES), source); } +// The dangerous collision, and the one the directive check alone does NOT catch: +// GL_EXT_texture_buffer is a strict prefix of GL_EXT_texture_buffer_object, a different and +// real extension that SPIRV-Cross emits from the same Dim=Buffer branch on its legacy-desktop +// path. Rewriting it would turn a valid request into one for a GL_OES_texture_buffer_object +// that does not exist. Only an identifier-boundary check saves this, so it gets its own test +// with the lookalike on a genuine #extension line. +TEST(RetargetTextureBufferExtensionTest, ALongerExtensionSharingThePrefixIsNotRewritten) { + const String source = R"(#version 310 es +#extension GL_EXT_texture_buffer_object : require +precision highp float; +void main() {} +)"; + EXPECT_EQ(RetargetTextureBufferExtension(source, Tier::ExtensionOES), source); + + // And when both appear, exactly the exact-match one moves. + const String mixed = R"(#version 310 es +#extension GL_EXT_texture_buffer_object : require +#extension GL_EXT_texture_buffer : require +precision highp float; +void main() {} +)"; + const String out = RetargetTextureBufferExtension(mixed, Tier::ExtensionOES); + EXPECT_TRUE(Contains(out, "#extension GL_EXT_texture_buffer_object : require")) << out; + EXPECT_TRUE(Contains(out, "#extension GL_OES_texture_buffer : require")) << out; + EXPECT_EQ(CountOf(out, "GL_OES_texture_buffer_object"), 0u) << out; +} + // Whitespace between '#' and the keyword is legal in GLSL, and a shader carrying several // extension directives must have exactly the one retargeted. TEST(RetargetTextureBufferExtensionTest, SpacedDirectiveIsRewrittenAndNeighboursAreLeftAlone) { diff --git a/MobileGL/MG_Test/BackendLoader/BackendLoaderTest.cpp b/MobileGL/MG_Test/BackendLoader/BackendLoaderTest.cpp index ae1b6e59..8ddcc142 100644 --- a/MobileGL/MG_Test/BackendLoader/BackendLoaderTest.cpp +++ b/MobileGL/MG_Test/BackendLoader/BackendLoaderTest.cpp @@ -52,6 +52,19 @@ namespace { GLfloat maxTextureMaxAnisotropy = 16.0f; bool maxTextureMaxAnisotropyQueried = false; + // Buffer textures. GL_MAX_TEXTURE_BUFFER_SIZE is only a legal pname once they exist, so + // asking on a driver without them raises GL_INVALID_ENUM - the same shape as the + // anisotropy probe above. The three entry-point knobs are separate because the + // unsuffixed name is the ES 3.2 CORE spelling while an EXT/OES driver exports the + // suffixed one: a resolver that only looks for the core name declares every extension + // driver unsupported, which is exactly the bug these knobs exist to pin. + GLint maxTextureBufferSize = 131072; + bool maxTextureBufferSizeQueried = false; + bool textureBufferSizeQueryRaisesError = false; + bool hasCoreTexBufferEntryPoint = true; + bool hasExtTexBufferEntryPoint = false; + bool hasOesTexBufferEntryPoint = false; + GLuint nextBufferId = 1; GLuint nextShaderId = 1; GLuint nextProgramId = 1; @@ -121,6 +134,14 @@ namespace { *data = g_fake.fragmentInterpolationOffsetBits; } break; + case GL_MAX_TEXTURE_BUFFER_SIZE: + g_fake.maxTextureBufferSizeQueried = true; + if (g_fake.textureBufferSizeQueryRaisesError) { + g_fake.pendingError = GL_INVALID_ENUM; + } else { + *data = g_fake.maxTextureBufferSize; + } + break; // FillInGLESCapabilities reads the context version before running the // baseInstance probe, which requires ES >= 3.1. case GL_MAJOR_VERSION: @@ -332,6 +353,21 @@ namespace { funcs.glDisable = [](GLenum) {}; funcs.glMemoryBarrier = [](GLbitfield) {}; + // Buffer-texture entry points, each present only when its knob says so. A real loader + // resolves the suffixed names only on a driver whose support is that extension. + funcs.glTexBuffer = g_fake.hasCoreTexBufferEntryPoint + ? static_cast( + [](GLenum, GLenum, GLuint) {}) + : nullptr; + funcs.glTexBufferEXT = g_fake.hasExtTexBufferEntryPoint + ? static_cast( + [](GLenum, GLenum, GLuint) {}) + : nullptr; + funcs.glTexBufferOES = g_fake.hasOesTexBufferEntryPoint + ? static_cast( + [](GLenum, GLenum, GLuint) {}) + : nullptr; + // The probe's vertex shader writes the gl_InstanceID it observed into the // result SSBO at binding 0. A conforming driver observes 0; a leaking one // observes the indirect command's baseInstance word (byte offset 12). @@ -520,6 +556,150 @@ TEST(FragmentInterpolationCapabilities, QueriesOnlyWhenSupportedAndPreservesDriv EXPECT_EQ(funcs.glGetError(), GL_NO_ERROR); } +// Buffer textures are core in the OpenGL 3.1+ context MobileGL advertises but need ES 3.2 or +// EXT/OES_texture_buffer on the host. The tier decides three things at once: whether glTexBuffer +// may be called at all, which #extension directive the emitted ESSL must carry, and whether +// GL_MAX_TEXTURE_BUFFER_SIZE is a driver answer or MobileGL's own floor. +using TextureBufferTier = MobileGL::MG_External::GLESCapabilities::TextureBufferTier; + +TEST(BufferTextureCapabilities, Es32ResolvesToCoreAndTakesTheDriverLimit) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.glesMinorVersion = 2; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::CoreEs32); + EXPECT_TRUE(caps.MaxTextureBufferSizeIsDriverReported); + EXPECT_EQ(caps.MaxTextureBufferSize, g_fake.maxTextureBufferSize); + EXPECT_TRUE(g_fake.maxTextureBufferSizeQueried); +} + +// The regression this pins: an ES 3.1 driver whose support is GL_EXT_texture_buffer exports +// glTexBufferEXT and NOT the unsuffixed core name. A resolver that requires the core pointer +// declares this driver unsupported and then refuses to compile shaders it could have run. +TEST(BufferTextureCapabilities, Es31WithExtResolvesThroughTheSuffixedEntryPoint) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.extensions.emplace_back("GL_EXT_texture_buffer"); + g_fake.hasCoreTexBufferEntryPoint = false; + g_fake.hasExtTexBufferEntryPoint = true; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::ExtensionEXT); + EXPECT_TRUE(caps.MaxTextureBufferSizeIsDriverReported); + EXPECT_EQ(caps.MaxTextureBufferSize, g_fake.maxTextureBufferSize); +} + +TEST(BufferTextureCapabilities, Es31WithOesResolvesThroughTheSuffixedEntryPoint) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.extensions.emplace_back("GL_OES_texture_buffer"); + g_fake.hasCoreTexBufferEntryPoint = false; + g_fake.hasOesTexBufferEntryPoint = true; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + // The tier, not just a boolean: it is what selects the OES spelling of the #extension + // directive SPIRV-Cross hardcodes as EXT. + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::ExtensionOES); + EXPECT_TRUE(caps.MaxTextureBufferSizeIsDriverReported); +} + +// EXT wins over OES on a driver advertising both, because SPIRV-Cross emits the EXT spelling +// natively and that tier needs no directive rewriting at all. +TEST(BufferTextureCapabilities, ExtIsPreferredWhenBothExtensionsArePresent) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.extensions.emplace_back("GL_OES_texture_buffer"); + g_fake.extensions.emplace_back("GL_EXT_texture_buffer"); + g_fake.hasExtTexBufferEntryPoint = true; + g_fake.hasOesTexBufferEntryPoint = true; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::ExtensionEXT); +} + +// The motivating driver (the emulator SDK's ANGLE: ES 3.1, neither extension). The pname is +// never asked - it would raise GL_INVALID_ENUM - and the floor MobileGL keeps advertising is +// flagged as not being a driver answer, because an OpenGL 4.x context may not report 0. +TEST(BufferTextureCapabilities, Es31WithNeitherExtensionIsUnsupportedAndNeverQueriesTheLimit) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::None); + EXPECT_FALSE(caps.MaxTextureBufferSizeIsDriverReported); + EXPECT_FALSE(g_fake.maxTextureBufferSizeQueried); + EXPECT_EQ(caps.MaxTextureBufferSize, 65536) << "the OpenGL 3.1 spec floor, not the fake's limit"; +} + +// An extension string with no entry point behind it is not support. This is the ES analogue of +// the multi-draw stub hazard: eglGetProcAddress may hand back live-looking pointers, so the +// two signals are required together. +TEST(BufferTextureCapabilities, AnExtensionStringWithoutAnEntryPointIsNotSupport) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.extensions.emplace_back("GL_EXT_texture_buffer"); + g_fake.hasCoreTexBufferEntryPoint = false; + g_fake.hasExtTexBufferEntryPoint = false; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::None); + EXPECT_FALSE(caps.MaxTextureBufferSizeIsDriverReported); +} + +// A driver that claims buffer textures and then refuses the query is a driver bug. The floor +// stands in, and the flag says the number was not the driver's - the POST row and the +// capability log both branch on exactly that. +TEST(BufferTextureCapabilities, ARejectedLimitQueryIsDrainedAndMarkedAsNotDriverReported) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.glesMinorVersion = 2; + g_fake.textureBufferSizeQueryRaisesError = true; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_EQ(caps.TextureBufferSupport, TextureBufferTier::CoreEs32); + EXPECT_TRUE(g_fake.maxTextureBufferSizeQueried); + EXPECT_FALSE(caps.MaxTextureBufferSizeIsDriverReported); + EXPECT_EQ(caps.MaxTextureBufferSize, 65536); + EXPECT_EQ(funcs.glGetError(), GL_NO_ERROR) << "the failed query must not leave an error behind"; +} + +// A stale error from an earlier probe must not be mistaken for this query failing. +TEST(BufferTextureCapabilities, AStaleErrorDoesNotDiscardTheDriverLimit) { + ResetFakeDriver(); + g_fake.maxVertexSsboBlocks = 0; + g_fake.glesMinorVersion = 2; + g_fake.pendingError = GL_INVALID_OPERATION; + const auto funcs = MakeFakeGLESFunctions(); + + MobileGL::MG_External::GLESCapabilities caps; + ASSERT_TRUE(MobileGL::MG_Util::BackendLoader::FillInGLESCapabilities(caps, funcs)); + + EXPECT_TRUE(caps.MaxTextureBufferSizeIsDriverReported); + EXPECT_EQ(caps.MaxTextureBufferSize, g_fake.maxTextureBufferSize); +} + TEST(FragmentInterpolationCapabilities, QueryErrorIsDrainedAndFallsBackToCoreMinimums) { ResetFakeDriver(); g_fake.maxVertexSsboBlocks = 0; diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp index 30528254..5bca6847 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp @@ -511,6 +511,13 @@ namespace MobileGL::MG_Util::BackendLoader { INIT_GLES_FUNC(glGetSamplerParameterIuiv) INIT_GLES_FUNC(glTexBuffer) INIT_GLES_FUNC(glTexBufferRange) + // Optional: absent on an ES 3.2 core driver, and absent on ES 3.1 without the + // matching extension. The tier resolution below picks whichever spelling the + // driver's own support actually comes from. + INIT_GLES_FUNC_OPTIONAL(glTexBufferEXT) + INIT_GLES_FUNC_OPTIONAL(glTexBufferOES) + INIT_GLES_FUNC_OPTIONAL(glTexBufferRangeEXT) + INIT_GLES_FUNC_OPTIONAL(glTexBufferRangeOES) INIT_GLES_FUNC(glTexStorage3DMultisample) INIT_GLES_FUNC(glMapBufferRange) INIT_GLES_FUNC(glBufferStorageEXT) @@ -1120,21 +1127,27 @@ namespace MobileGL::MG_Util::BackendLoader { // with both needs no directive retargeting. The entry point has to have resolved either // way - the extension string alone is not support (see the multi-draw note above). { - const Bool textureBufferIsCore = - caps.GLESVersion.Major > 3 || (caps.GLESVersion.Major == 3 && caps.GLESVersion.Minor >= 2); using Tier = MG_External::GLESCapabilities::TextureBufferTier; - if (glesFuncs.glTexBuffer == nullptr) { - caps.TextureBufferSupport = Tier::None; - } else if (textureBufferIsCore) { + // Each tier needs the entry point that tier's support actually ships. Gating all + // three on the unsuffixed name - the ES 3.2 CORE spelling - would make every + // EXT/OES driver look unsupported on a strict loader, and would make MobileGL call + // a core entry point the driver never exported on a permissive one. The suffixed + // name is preferred where the support is an extension, with the core name accepted + // as a fallback because drivers that expose both alias them. + const Bool hasCoreEntryPoint = glesFuncs.glTexBuffer != nullptr; + if (esAtLeast32 && hasCoreEntryPoint) { caps.TextureBufferSupport = Tier::CoreEs32; - } else if (hasExtTextureBuffer) { + } else if (hasExtTextureBuffer && (glesFuncs.glTexBufferEXT != nullptr || hasCoreEntryPoint)) { caps.TextureBufferSupport = Tier::ExtensionEXT; - } else if (hasOesTextureBuffer) { + } else if (hasOesTextureBuffer && (glesFuncs.glTexBufferOES != nullptr || hasCoreEntryPoint)) { caps.TextureBufferSupport = Tier::ExtensionOES; } else { caps.TextureBufferSupport = Tier::None; } + // Assigned unconditionally, like every other capability in this function, so a + // second fill on a reused struct cannot keep a stale true. + caps.MaxTextureBufferSizeIsDriverReported = false; if (caps.TextureBufferSupport != Tier::None) { // Drain first: an error left by any earlier probe would otherwise read as this // query having failed, and the value would be discarded as a non-answer. @@ -1285,8 +1298,16 @@ namespace MobileGL::MG_Util::BackendLoader { MGLOG_I(" GL_MAX_COMPUTE_UNIFORM_BLOCKS: %d", caps.MaxComputeUniformBlocks); MGLOG_I(" GL_MAX_COMPUTE_WORK_GROUP_INVOCATIONS: %d", caps.MaxComputeWorkGroupInvocations); MGLOG_I(" GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS: %d", caps.MaxShaderStorageBufferBindings); + // Three distinct states, and the suffix must not conflate them: a driver answer, a floor + // kept because there are no buffer textures to ask about, and a floor kept because the + // driver claimed buffer textures but then refused the query (which is a driver bug worth + // seeing spelled out rather than hidden behind the same wording as the honest case). MGLOG_I(" GL_MAX_TEXTURE_BUFFER_SIZE: %d%s", caps.MaxTextureBufferSize, - caps.MaxTextureBufferSizeIsDriverReported ? "" : " (MobileGL floor - the driver has no buffer textures to ask)"); + caps.MaxTextureBufferSizeIsDriverReported + ? "" + : (caps.TextureBufferSupport == MG_External::GLESCapabilities::TextureBufferTier::None + ? " (MobileGL floor - the driver has no buffer textures to ask)" + : " (MobileGL floor - the driver claims buffer textures but rejected the query)")); MGLOG_I(" GL_MAX_UNIFORM_BUFFER_BINDINGS: %d", caps.MaxUniformBufferBindings); MGLOG_I(" GL_MAX_UNIFORM_BLOCK_SIZE: %d", caps.MaxUniformBlockSize); MGLOG_I(" GL_MAX_IMAGE_UNITS: %d", caps.MaxImageUnits); diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h index c0647271..e32c5bf5 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h @@ -600,6 +600,16 @@ namespace MobileGL { GL_FUNC_TYPEDEF(void, glSamplerParameterIuiv, GLuint sampler, GLenum pname, const GLuint* param) GL_FUNC_TYPEDEF(void, glGetSamplerParameterIiv, GLuint sampler, GLenum pname, GLint* params) GL_FUNC_TYPEDEF(void, glGetSamplerParameterIuiv, GLuint sampler, GLenum pname, GLuint* params) + // The unsuffixed names are the ES 3.2 CORE entry points. A driver whose buffer-texture + // support comes from GL_EXT_texture_buffer or GL_OES_texture_buffer exports the + // suffixed spellings instead, and a strict eglGetProcAddress returns NULL for the core + // one there - so resolving only the core name makes both extension tiers look absent. + GL_FUNC_TYPEDEF(void, glTexBufferEXT, GLenum target, GLenum internalformat, GLuint buffer) + GL_FUNC_TYPEDEF(void, glTexBufferOES, GLenum target, GLenum internalformat, GLuint buffer) + GL_FUNC_TYPEDEF(void, glTexBufferRangeEXT, GLenum target, GLenum internalformat, GLuint buffer, + GLintptr offset, GLsizeiptr size) + GL_FUNC_TYPEDEF(void, glTexBufferRangeOES, GLenum target, GLenum internalformat, GLuint buffer, + GLintptr offset, GLsizeiptr size) GL_FUNC_TYPEDEF(void, glTexBuffer, GLenum target, GLenum internalformat, GLuint buffer) GL_FUNC_TYPEDEF(void, glTexBufferRange, GLenum target, GLenum internalformat, GLuint buffer, GLintptr offset, GLsizeiptr size) @@ -1002,6 +1012,10 @@ namespace MobileGL { GL_FUNC_DECL(glGetSamplerParameterIuiv) GL_FUNC_DECL(glTexBuffer) GL_FUNC_DECL(glTexBufferRange) + GL_FUNC_DECL(glTexBufferEXT) + GL_FUNC_DECL(glTexBufferOES) + GL_FUNC_DECL(glTexBufferRangeEXT) + GL_FUNC_DECL(glTexBufferRangeOES) GL_FUNC_DECL(glTexStorage3DMultisample) GL_FUNC_DECL(glMapBufferRange) GL_FUNC_DECL(glBufferStorageEXT) diff --git a/MobileGL/MG_Util/SelfTest/DriverPost.cpp b/MobileGL/MG_Util/SelfTest/DriverPost.cpp index d8c208b5..3f9ccba6 100644 --- a/MobileGL/MG_Util/SelfTest/DriverPost.cpp +++ b/MobileGL/MG_Util/SelfTest/DriverPost.cpp @@ -422,45 +422,56 @@ namespace MobileGL::MG_Util::SelfTest { "map array texture gets no driver storage at all, so sampling one reads nothing " "and rendering to one does not reach the screen"); } - // FAIL, not WARN: buffer textures are CORE in OpenGL 3.1 and MobileGL advertises a 4.x - // context, so an application may use one without asking - and nothing degrades - // gracefully when they are absent. The texture gets no driver storage (glTexBuffer does - // not exist), and, worse, every shader declaring a samplerBuffer fails to compile - // outright, because SPIRV-Cross emits `#extension GL_EXT_texture_buffer : require` for - // it below ESSL 320. The program then never links and every draw using it silently - // draws nothing - which is how Minecraft 26.3, whose cloud layer is built entirely from - // gl_VertexID plus texelFetch on a GL_R8I buffer texture, loses its clouds. + // WARN, not FAIL, and the choice is deliberate. The consequence is severe - buffer + // textures are CORE in OpenGL 3.1 and MobileGL advertises a 4.x context, so an + // application may use one without asking, and nothing degrades gracefully: the + // texture gets no driver storage, and every shader declaring a samplerBuffer fails + // to compile outright, because SPIRV-Cross emits `#extension GL_EXT_texture_buffer : + // require` for it below ESSL 320, so the program never links and every draw using it + // silently draws nothing. That is how Minecraft 26.3, whose cloud layer is built + // entirely from gl_VertexID plus texelFetch on a GL_R8I buffer texture, loses its + // clouds. But FAIL means "this backend cannot run on this driver", and that is not + // true: such a device runs everything that does not touch a buffer texture. It is + // also exactly the shape of the "Texture cube map array" row above, which loses its + // shaders to the same SPIRV-Cross `: require` mechanism and is a WARN - two adjacent + // rows with one consequence must not carry two severities. // The limit is stated on every tier because it is the one number an application can // read, and on the None tier it is knowingly a fiction (see below). { using Tier = MG_External::GLESCapabilities::TextureBufferTier; const Int advertisedLimit = caps.MaxTextureBufferSize; + // A supported tier that then refused GL_MAX_TEXTURE_BUFFER_SIZE is a driver bug; + // the row must not call MobileGL's floor "the driver's own answer" there. + const char* limitProvenance = + caps.MaxTextureBufferSizeIsDriverReported + ? "the driver's own answer" + : "MobileGL's floor - this driver claims buffer textures but rejected the query"; switch (caps.TextureBufferSupport) { case Tier::CoreEs32: builder.Pass("Buffer textures", - format("core in ES 3.2; GL_MAX_TEXTURE_BUFFER_SIZE = {} is the " - "driver's own answer, and ESSL 320 needs no #extension " - "directive to declare a samplerBuffer", - advertisedLimit)); + format("core in ES 3.2; GL_MAX_TEXTURE_BUFFER_SIZE = {} is {}, and " + "ESSL 320 needs no #extension directive to declare a " + "samplerBuffer", + advertisedLimit, limitProvenance)); break; case Tier::ExtensionEXT: builder.Pass("Buffer textures", - format("GL_EXT_texture_buffer; GL_MAX_TEXTURE_BUFFER_SIZE = {} is " - "the driver's own answer, and the directive SPIRV-Cross " - "emits (GL_EXT_texture_buffer) is the one this driver wants", - advertisedLimit)); + format("GL_EXT_texture_buffer; GL_MAX_TEXTURE_BUFFER_SIZE = {} is {}, " + "and the directive SPIRV-Cross emits " + "(GL_EXT_texture_buffer) is the one this driver wants", + advertisedLimit, limitProvenance)); break; case Tier::ExtensionOES: builder.Pass("Buffer textures", - format("GL_OES_texture_buffer; GL_MAX_TEXTURE_BUFFER_SIZE = {} is " - "the driver's own answer. SPIRV-Cross hardcodes the EXT " - "spelling, so MobileGL retargets the emitted #extension " - "directive to the OES one this driver advertises", - advertisedLimit)); + format("GL_OES_texture_buffer; GL_MAX_TEXTURE_BUFFER_SIZE = {} is {}. " + "SPIRV-Cross hardcodes the EXT spelling, so MobileGL " + "retargets the emitted #extension directive to the OES one " + "this driver advertises", + advertisedLimit, limitProvenance)); break; case Tier::None: default: - builder.Fail("Buffer textures", + builder.Warn("Buffer textures", format("not supported (pre-ES 3.2 without GL_EXT/OES_texture_buffer); " "glTexBuffer does not exist, so a buffer texture gets no storage, " "and any shader declaring a samplerBuffer fails to compile and " diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 91f4b440..b4fbe707 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -541,6 +541,12 @@ namespace MobileGL { } Bool ShaderCompiler::ModuleDeclaresBufferTextureSampler(const Vector& spirv) { + if (spirv.empty()) { + // Early out rather than letting BuildModule reject it: an empty module is a + // stage that produced no SPIR-V, which is not a capability verdict, and the + // parse would push a spurious diagnostic through the message consumer first. + return false; + } // Callers gate this on the driver LACKING buffer textures, so the module build // here only ever happens on a degraded driver that is about to fail the compile // anyway - it is not on the healthy path. @@ -556,10 +562,14 @@ namespace MobileGL { if (type.opcode() != spv::Op::OpTypeImage) { continue; } - // OpTypeImage operands: Sampled Type, Dim, Depth, Arrayed, MS, Sampled, Format. - // Dim is operand 1; SpvDimBuffer is what samplerBuffer/isamplerBuffer/ - // usamplerBuffer all lower to, whatever their sampled type. - if (static_cast(type.GetSingleWordInOperand(1)) == spv::Dim::Buffer) { + // OpTypeImage in-operands: Sampled Type, Dim, Depth, Arrayed, MS, Sampled, + // Format. Dim is operand 1; Dim::Buffer is what samplerBuffer/isamplerBuffer/ + // usamplerBuffer all lower to, whatever their sampled type - and equally what + // the imageBuffer family lowers to, which is correct here because SPIRV-Cross + // requires the same extension for those. The operand-count guard mirrors + // NormalizeRectCoordinatesPass, which reads the same operand. + if (type.NumInOperands() >= 2 && + static_cast(type.GetSingleWordInOperand(1)) == spv::Dim::Buffer) { return true; } } diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 6d7f6c37..787fc543 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -134,8 +134,11 @@ namespace MobileGL { static Uint64 SpirvValidationFailureCount(); static Uint64 NoteSpirvValidationFailure(); - // True when the module declares any buffer-texture sampler - a samplerBuffer, - // isamplerBuffer or usamplerBuffer, i.e. an OpTypeImage with Dim = Buffer. + // True when the module declares any buffer-backed image type - an OpTypeImage with + // Dim = Buffer. That is the samplerBuffer / isamplerBuffer / usamplerBuffer + // family and equally the imageBuffer / iimageBuffer / uimageBuffer one: SPIRV-Cross + // requires GL_EXT_texture_buffer for both, from the same branch, so both are + // uncompilable on a driver without buffer textures and both belong here. // DirectGLES asks before handing the transpiled ESSL to the driver: buffer // textures are core in the OpenGL 3.1+ context MobileGL advertises but need // ES 3.2 or EXT/OES_texture_buffer on the host, and on a driver without them