diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 79e0f4f9..f351cb37 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -1718,14 +1718,16 @@ namespace MobileGL::MG_Backend::DirectGLES { m_syncedAttributeVersions[attribIndex].BufferVersion; if (!needsSyncFormat && !needsSyncBuffer && !needsSyncBaseInstance) continue; - // Defence in depth. The frontend already declines glVertexAttribLFormat on this - // backend (SupportsFloat64VertexAttributes is false - ES has no GL_DOUBLE vertex - // format and ESSL has no fp64 type), so IsLong should never arrive here; if it ever - // did, passing GL_DOUBLE to glVertexAttribPointer would only raise GL_INVALID_ENUM on - // the real driver. Disabling rather than merely skipping matters: becoming long bumps - // FormatVersion, not SwitchVersion, so the enable/disable block above will not run - // again and an already-enabled array would stay enabled with no pointer and no - // ARRAY_BUFFER binding - which ES 3.1+ makes an INVALID_OPERATION at draw. + // This is where a 64-bit array actually stops. glVertexAttribLFormat is a legal call + // in a GL 4.3 context and the frontend RECORDS its format (the state queries have to + // answer), so IsLong does arrive here - what this backend cannot do is FEED it: + // SupportsFloat64VertexAttributes is false because ES has no GL_DOUBLE vertex format + // and ESSL has no fp64 type, and passing GL_DOUBLE to glVertexAttribPointer would + // only raise GL_INVALID_ENUM on the real driver. Disabling rather than merely + // skipping matters: becoming long bumps FormatVersion, not SwitchVersion, so the + // enable/disable block above will not run again and an already-enabled array would + // stay enabled with no pointer and no ARRAY_BUFFER binding - which ES 3.1+ makes an + // INVALID_OPERATION at draw. // // IsLong is not the only way a 64-bit array gets here: glVertexAttribFormat // with GL_DOUBLE asks for doubles in memory CONVERTED to float, so it is not diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index caf17a7a..c69a994a 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -8,6 +8,7 @@ #include "VertexInputStateFactory.h" #include "MG_Util/Converters/MGToStr/DataTypeConverter.h" +#include #include namespace MobileGL::MG_Backend::DirectVulkan { @@ -330,6 +331,20 @@ namespace MobileGL::MG_Backend::DirectVulkan { // for every R64 float format, so a native 64-bit vertex fetch is simply unavailable there // while shaderFloat64 is not. Both halves key off nothing but the attribute being long, // so they always agree without extra plumbing. + // + // ... as long as the shader half still runs. It does not when the backend has declared + // no 64-bit vertex attribute support: DemoteFloat64Pass has already narrowed every + // `dvec` input to a `vec` by then, so PackDoubleVertexInputsPass finds nothing to pack + // and a UINT-formatted attribute would be fed to a float input - garbage with no + // diagnostic anywhere. Declining here drops the array instead (the caller skips + // UNDEFINED attributes and reports them through unsupportedAttribMask), which is what + // DirectGLES does for the same state. The frontend RECORDS the format either way, so + // this gate is the only thing standing between a legal glVertexAttribLFormat and a + // mismatched pipeline. + if (MG_Backend::pActiveBackendObject == nullptr || + !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) { + return VK_FORMAT_UNDEFINED; + } if (!isLong || isInteger || normalized) return VK_FORMAT_UNDEFINED; switch (size) { case 1: return VK_FORMAT_R32G32_UINT; diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp index 6fd7ad63..cfffb533 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp @@ -514,10 +514,15 @@ namespace MobileGL::MG_Impl::GLImpl { // recorded DataType is always Float64 - what IsLong adds is that this is the *unconverted* form, // as opposed to VertexAttribFormat(GL_DOUBLE), which asks for a float conversion. // - // Whether the backend can feed it is detected, not assumed: DirectVulkan needs shaderFloat64, - // and DirectGLES can never have it at all. A backend without it declines here, loudly - GL error - // plus a log line naming the reason - rather than accepting state no draw could honour and - // rendering garbage. The matching startup POST row is in MG_Util/SelfTest/DriverPost.cpp. + // Whether the backend can FEED it is detected, not assumed: DirectVulkan needs shaderFloat64, + // and DirectGLES can never have it at all. What that costs is the ARRAY, not the call: GL 4.6 + // core 10.3.2 defines no error for a well-formed glVertexAttribLFormat, and a GL 4.3 context + // has 64-bit attributes in core, so declining the call would be non-conformant and would make + // the four pure state queries (VERTEX_ATTRIB_ARRAY_SIZE / _TYPE / _LONG / _RELATIVE_OFFSET) + // unanswerable (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore + // RECORDED here and the enabled array is dropped at draw instead - loudly, once, naming the + // reason. The matching startup POST row is in MG_Util/SelfTest/DriverPost.cpp; the draw-side + // drop is DirectGLES/Managers.cpp and, on DirectVulkan, VertexInputStateFactory's Float64 case. static void VertexAttribLFormatSeparate_State(const SharedPtr& vao, GLuint attribindex, GLint size, GLenum type, GLuint relativeoffset) { @@ -528,14 +533,11 @@ namespace MobileGL::MG_Impl::GLImpl { if (!MG_Backend::pActiveBackendObject || !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) { MGLOG_W_ONCE("VertexAttribLFormat: attribute %u asked for a 64-bit (GL_DOUBLE) format, but this " - "backend has no double-precision vertex attribute support - see the " - "\"64-bit vertex attributes\" / \"shaderFloat64\" POST row for what that costs", + "backend has no double-precision vertex attribute support - the format is recorded " + "and queryable, but the array will be DROPPED at draw and the attribute will read " + "its generic current value; see the \"64-bit vertex attributes\" / \"shaderFloat64\" " + "POST row for what that costs", attribindex); - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", "VertexAttribLFormat", - "64-bit vertex attributes are not supported by this backend.")); - return; } vao->SetAttributeFormatSeparate(attribindex, size, MG_Util::ConvertGLEnumToDataType(type), diff --git a/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp index a59e3d69..682719e2 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp @@ -697,24 +697,127 @@ void main() { EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); } - TEST_F(DoublePrecisionScenario, A64BitVertexFormatIsDeclinedOnEveryBackend) { + TEST_F(DoublePrecisionScenario, A64BitVertexFormatIsRecordedAndItsArrayIsDroppedAtDraw) { if (!Ready()) return; // The demotion leaves no 64-bit shader input to feed, so there is nothing a 64-bit // vertex FETCH could be fetched into - on either backend, and no longer only on the - // ones whose device lacks shaderFloat64. Declined loudly rather than accepted and - // drawn as garbage; the matching POST row says the same thing at startup. + // ones whose device lacks shaderFloat64. + // + // What that costs is the ARRAY, not the CALL. GL 4.6 core 10.3.2 defines no error for + // a well-formed glVertexAttribLFormat and 64-bit attributes are core in the GL 4.3 + // context MobileGL advertises, so refusing the call would be non-conformant and would + // leave four pure state queries unanswerable + // (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore recorded and + // queryable; the enabled array is what gets dropped, and the attribute then reads its + // generic current value. The matching POST row says exactly that at startup. GLuint vao = 0; glGenVertexArrays(1, &vao); glBindVertexArray(vao); while (glGetError() != GL_NO_ERROR) {} - glVertexAttribLFormat(0, 3, GL_DOUBLE, 0); - EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)); + glVertexAttribLFormat(1, 3, GL_DOUBLE, 8); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "glVertexAttribLFormat is a legal call in a GL 4.3 context"; + + GLint attribSize = 0; + GLint attribType = 0; + GLint attribIsLong = 0; + GLint attribRelativeOffset = 0; + glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_SIZE, &attribSize); + glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_TYPE, &attribType); + glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_LONG, &attribIsLong); + glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_RELATIVE_OFFSET, &attribRelativeOffset); + EXPECT_EQ(attribSize, 3); + EXPECT_EQ(attribType, static_cast(GL_DOUBLE)); + EXPECT_EQ(attribIsLong, GL_TRUE) << "GL_VERTEX_ATTRIB_ARRAY_LONG is what makes this the " + "unconverted form; without it the state is a lie"; + EXPECT_EQ(attribRelativeOffset, 8); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); glBindVertexArray(0); glDeleteVertexArrays(1, &vao); while (glGetError() != GL_NO_ERROR) {} } + // The consequence of recording the state rather than refusing the call: a 64-bit array can + // now be ENABLED in a VAO that a draw uses, which it never could before. That must not + // take the draw down. Leaving such an array enabled with no pointer behind it is exactly + // the documented Adreno null-deref (SIGSEGV inside the next glDraw*), so DirectGLES + // disables it before glVertexAttribPointer can ever see GL_DOUBLE, and DirectVulkan maps + // the format to VK_FORMAT_UNDEFINED so it never enters the pipeline's vertex input state. + // + // The shader deliberately does NOT read location 1: that keeps the two backends on the + // same path (DirectVulkan declines a draw whose SHADER reads an unsupported enabled array, + // by design and loudly, which is a different assertion from this one) and it is the shape + // the crash needed - an enabled array nothing set a pointer for. + TEST_F(DoublePrecisionScenario, AnEnabledLongArrayDoesNotBreakADrawThatIgnoresIt) { + if (!Ready()) return; + + constexpr const char* kVs = R"(#version 430 core +layout(location = 0) in vec2 aPos; +void main() { gl_Position = vec4(aPos, 0.0, 1.0); } +)"; + constexpr const char* kFs = R"(#version 430 core +out vec4 o_color; +void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } +)"; + std::string error; + const unsigned int program = CompileProgram(kVs, kFs, &error); + ASSERT_NE(program, 0u) << error; + + ColorFbo target = MakeColorFbo(32, 32); + ASSERT_NE(target.fbo, 0u) << "could not create the render target"; + BindFbo(target); + + const float positions[8] = {-1.0f, -1.0f, 1.0f, -1.0f, -1.0f, 1.0f, 1.0f, 1.0f}; + const double doubles[4] = {1.0, 2.0, 3.0, 4.0}; + + GLuint vao = 0; + GLuint positionBuffer = 0; + GLuint doubleBuffer = 0; + glGenVertexArrays(1, &vao); + glBindVertexArray(vao); + glGenBuffers(1, &positionBuffer); + glBindBuffer(GL_ARRAY_BUFFER, positionBuffer); + glBufferData(GL_ARRAY_BUFFER, sizeof(positions), positions, GL_STATIC_DRAW); + glGenBuffers(1, &doubleBuffer); + glBindBuffer(GL_ARRAY_BUFFER, doubleBuffer); + glBufferData(GL_ARRAY_BUFFER, sizeof(doubles), doubles, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glVertexAttribFormat(0, 2, GL_FLOAT, GL_FALSE, 0); + glVertexAttribBinding(0, 0); + glBindVertexBuffer(0, positionBuffer, 0, static_cast(2 * sizeof(float))); + glEnableVertexAttribArray(0); + + glVertexAttribLFormat(1, 1, GL_DOUBLE, 0); + glVertexAttribBinding(1, 1); + glBindVertexBuffer(1, doubleBuffer, 0, static_cast(sizeof(double))); + glEnableVertexAttribArray(1); + EXPECT_EQ(FirstGLError(), 0u) << "setting up the 64-bit array was refused"; + + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glUseProgram(program); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + EXPECT_EQ(FirstGLError(), 0u) << "a draw with an enabled 64-bit array must not raise an error"; + + const Image image = ReadPixels(target.width, target.height); + ASSERT_FALSE(image.Empty()); + EXPECT_GT(image.At(target.width / 2, target.height / 2).g, 200) + << "the draw did not happen; the enabled 64-bit array must be dropped, not fatal"; + + glDisableVertexAttribArray(0); + glDisableVertexAttribArray(1); + glBindVertexArray(0); + glDeleteVertexArrays(1, &vao); + glDeleteBuffers(1, &positionBuffer); + glDeleteBuffers(1, &doubleBuffer); + BindDefaultFramebuffer(); + DestroyColorFbo(target); + glUseProgram(0); + glDeleteProgram(program); + EXPECT_EQ(FirstGLError(), 0u); + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_Util/SelfTest/DriverPost.cpp b/MobileGL/MG_Util/SelfTest/DriverPost.cpp index 4cd7c41d..460060e1 100644 --- a/MobileGL/MG_Util/SelfTest/DriverPost.cpp +++ b/MobileGL/MG_Util/SelfTest/DriverPost.cpp @@ -521,9 +521,11 @@ namespace MobileGL::MG_Util::SelfTest { builder.Warn("64-bit vertex attributes", "not supported (ES has no GL_DOUBLE vertex format, and after the fp64 demotion " "above there is no 64-bit shader input left to feed either); " - "glVertexAttribLFormat / glVertexArrayAttribLFormat report " - "GL_INVALID_OPERATION - feed the attribute with glVertexAttribPointer(GL_FLOAT), " - "which a demoted dvec input reads correctly"); + "glVertexAttribLFormat / glVertexArrayAttribLFormat succeed and their state is " + "queryable, but an ENABLED 64-bit array is DROPPED at draw and the attribute " + "reads its generic current value - feed the attribute with " + "glVertexAttribPointer(GL_FLOAT) instead, which a demoted dvec input reads " + "correctly"); if (glesFuncs.glPatchParameteri != nullptr) { builder.Pass("Tessellation patch parameters", "glPatchParameteri present (GL_PATCH_VERTICES reaches the driver)"); @@ -2336,8 +2338,10 @@ namespace MobileGL::MG_Util::SelfTest { builder.Warn("64-bit vertex attributes", "not supported; there is no 64-bit shader input left to feed after the fp64 demotion " "above, and no VK_FORMAT_R64*_SFLOAT vertex fetch to feed it with on most devices " - "anyway. glVertexAttribLFormat reports GL_INVALID_OPERATION - feed the attribute with " - "glVertexAttribPointer(GL_FLOAT), which a demoted dvec input reads correctly"); + "anyway. glVertexAttribLFormat succeeds and its state is queryable, but an ENABLED " + "64-bit array is DROPPED at pipeline build and the attribute reads its generic " + "current value - feed the attribute with glVertexAttribPointer(GL_FLOAT) instead, " + "which a demoted dvec input reads correctly"); Bool shaderDrawParameters = false; if (vkGetPhysicalDeviceFeatures2Fn != nullptr && properties.apiVersion >= VK_API_VERSION_1_1) {