diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index f12f8ba0..e3bee89b 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -32,10 +32,22 @@ namespace MobileGL::MG_Impl::GLImpl { static bool CheckShaderNameValidity(Uint shader) { if (shader == 0 || !MG_State::pGLContext->ValidateShaderName(shader)) { + // The mirror of CheckProgramNameValidity below, and for the same reason: programs and + // shaders are drawn from ONE name space (ProgramState hands both out of a single + // generator), so a name that exists but belongs to a PROGRAM is the wrong kind of + // object - GL 3.3 core 2.11.x makes that INVALID_OPERATION - while a name GL never + // handed out is INVALID_VALUE. This half of the split was missing, so every shader + // entry point handed a program name reported INVALID_VALUE; the conformance suite + // reads exactly that code back from glSpecializeShader. + const ErrorCode error = (shader != 0 && MG_State::pGLContext->ValidateProgramName(shader)) + ? ErrorCode::InvalidOperation + : ErrorCode::InvalidValue; MG_State::pGLContext->RecordError( - ErrorCode::InvalidValue, + error, MakeUnique("MG_Impl/GLImpl", __func__, - std::to_string(shader) + " is not a valid name.")); + std::to_string(shader) + + (error == ErrorCode::InvalidOperation ? " is not a shader object." + : " is not a valid name."))); return false; } return true; @@ -451,6 +463,25 @@ namespace MobileGL::MG_Impl::GLImpl { " has no SPIR-V binary; call glShaderBinary first.")); return; } + // ARB_gl_spirv: a shader that has already been specialized may not be specialized again + // until glShaderBinary re-associates a module with it. + if (shaderObject->HasBeenSpecialized()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", __func__, + "shader " + std::to_string(shader) + + " has already been specialized; re-associate its module with " + "glShaderBinary before specializing it again.")); + return; + } + // pEntryPoint names the entry point to specialize; there is no default. A null pointer + // cannot name one, and neither can the empty string. + if (pEntryPoint == nullptr || *pEntryPoint == '\0') { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", __func__, "pEntryPoint must name an entry point.")); + return; + } if (numSpecializationConstants > 0 && (pConstantIndex == nullptr || pConstantValue == nullptr)) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, @@ -475,20 +506,32 @@ namespace MobileGL::MG_Impl::GLImpl { } } - const String entryPoint = pEntryPoint ? String(pEntryPoint) : String{}; + const String entryPoint(pEntryPoint); const GLenum shaderType = MG_Util::ConvertShaderStageToGLEnum(shaderObject->GetShaderStage()); + using SpecializationFailure = MG_Util::ShaderTranspiler::ShaderCompiler::SpecializationFailure; + SpecializationFailure failure = SpecializationFailure::None; auto specialized = MG_Util::ShaderTranspiler::ShaderCompiler::SpecializeAndDecompileSpirvModule( - shaderObject->GetSpirvBinary(), shaderType, entryPoint, constantIds, constantValues); + shaderObject->GetSpirvBinary(), shaderType, entryPoint, constantIds, constantValues, failure); if (!specialized) { - // Specialization failure is a COMPILE failure, not a GL error: ARB_gl_spirv routes it - // through COMPILE_STATUS and the info log exactly as glCompileShader does, so an - // application that checks the status the usual way sees it. MGLOG_D("%s: specialization failed for shader %u: %s", __func__, shader, specialized.error().log.c_str()); + // The two conditions ARB_gl_spirv ENUMERATES are GL errors, and an erroring GL command + // must have no other effect - so the shader object is left exactly as it was rather + // than being pushed into a failed-compile state. Anything else is a genuine compile + // failure of a well-formed request, which the extension routes through COMPILE_STATUS + // and the info log exactly as glCompileShader does. + if (failure == SpecializationFailure::UnknownEntryPoint || + failure == SpecializationFailure::UnknownConstantId) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", __func__, specialized.error().log)); + return; + } shaderObject->RecordSpecializationFailure(String(specialized.error().log)); return; } - shaderObject->SpecializeFromSpirv(Move(specialized.value())); + shaderObject->SpecializeFromSpirv(Move(specialized.value().glsl), Move(specialized.value().xfbVaryings), + specialized.value().xfbBufferMode); } // glMaxShaderCompilerThreadsKHR / glMaxShaderCompilerThreadsARB - one implementation, @@ -1057,9 +1100,13 @@ namespace MobileGL::MG_Impl::GLImpl { } *params = shaderObject->GetInfoLog().empty() ? 0 : (GLint)shaderObject->GetInfoLog().length() + 1; break; - case GL_SHADER_SOURCE_LENGTH: - *params = shaderObject->GetShaderSource().empty() ? 0 : (GLint)shaderObject->GetShaderSource().length() + 1; + case GL_SHADER_SOURCE_LENGTH: { + // The APPLICATION's source, which is empty for a shader that came from glShaderBinary - + // see ShaderObject::GetApplicationShaderSource. + const auto& source = shaderObject->GetApplicationShaderSource(); + *params = source.empty() ? 0 : (GLint)source.length() + 1; break; + } // GL_ARB_gl_spirv. GL_SPIR_V_BINARY and GL_SPIR_V_BINARY_ARB are the same token: TRUE // while the object stands for an application-supplied module. It is the FIRST thing the // conformance suite asks after glShaderBinary, and it used to fall into the terminal @@ -1111,7 +1158,7 @@ namespace MobileGL::MG_Impl::GLImpl { auto& shaderObject = TryToGetShaderObject(shader); if (!shaderObject) return; - auto& src = shaderObject->GetShaderSource(); + auto& src = shaderObject->GetApplicationShaderSource(); CopyStr(bufSize, length, source, src.c_str(), (GLsizei)src.length()); } @@ -1968,8 +2015,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix2fv_State(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLfloat* value) { - if (location == -1) return; - auto& programObject = TryToGetProgramObject(program); if (!programObject) return; @@ -1981,14 +2026,14 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + if (location == -1) return; + UniformMatrixfv_Object(*programObject, __func__, location, count, transpose, value, 2, 2, "program " + std::to_string(program)); } void ProgramUniformMatrix3fv_State(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLfloat* value) { - if (location == -1) return; - auto& programObject = TryToGetProgramObject(program); if (!programObject) return; @@ -2000,6 +2045,8 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + if (location == -1) return; + for (GLint i = 0; i < count; i++) { if (i > 0 && !programObject->UniformLocationsAliasSameUniform(location, location + i)) { // Values for elements beyond the end of the uniform array are ignored. @@ -2025,8 +2072,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix4fv_State(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLfloat* value) { - if (location == -1) return; - auto& programObject = TryToGetProgramObject(program); if (!programObject) return; @@ -2038,6 +2083,8 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + if (location == -1) return; + for (GLint i = 0; i < count; i++) { if (i > 0 && !programObject->UniformLocationsAliasSameUniform(location, location + i)) { // Values for elements beyond the end of the uniform array are ignored. @@ -2059,8 +2106,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrixNonSquarefv_State(const char* caller, GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLfloat* value, Int columns, Int rows) { - if (location == -1) return; - auto& programObject = TryToGetProgramObject(program); if (!programObject) return; @@ -2072,6 +2117,8 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + if (location == -1) return; + UniformMatrixfv_Object(*programObject, caller, location, count, transpose, value, columns, rows, "program " + std::to_string(program)); } @@ -2621,7 +2668,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix2dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2631,6 +2677,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 2, 2); } void UniformMatrix3dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2647,7 +2694,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix3dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2657,6 +2703,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 3, 3); } void UniformMatrix4dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2673,7 +2720,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix4dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2683,6 +2729,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 4, 4); } void UniformMatrix2x3dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2699,7 +2746,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix2x3dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2709,6 +2755,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 2, 3); } void UniformMatrix2x4dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2725,7 +2772,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix2x4dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2735,6 +2781,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 2, 4); } void UniformMatrix3x2dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2751,7 +2798,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix3x2dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2761,6 +2807,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 3, 2); } void UniformMatrix3x4dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2777,7 +2824,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix3x4dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2787,6 +2833,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 3, 4); } void UniformMatrix4x2dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2803,7 +2850,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix4x2dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2813,6 +2859,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 4, 2); } void UniformMatrix4x3dv(GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { @@ -2829,7 +2876,6 @@ namespace MobileGL::MG_Impl::GLImpl { void ProgramUniformMatrix4x3dv(GLuint program, GLint location, GLsizei count, GLboolean transpose, const GLdouble* value) { - if (location == -1) return; auto& programObject = TryToGetProgramObject(program); if (!programObject) return; if (!programObject->GetLinkStatus()) { @@ -2839,6 +2885,7 @@ namespace MobileGL::MG_Impl::GLImpl { "program " + std::to_string(program) + " is not linked.")); return; } + if (location == -1) return; UniformMatrixdv_Object(*programObject, location, count, transpose, value, 4, 3); } void GetUniformdv(GLuint program, GLint location, GLdouble* params) { diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 9cb021e5..fad4b111 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -403,10 +403,46 @@ namespace MobileGL::MG_Impl::GLImpl { MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS)); } - // Array targets store their layer count in z; layers never participate in mip - // reduction (GL 3.3 ยง3.8.14), only true 3D textures halve their depth per level. + // How many components of a GL-space texel size actually halve down the mip chain. + // + // An array texture's LAYER COUNT is not a dimension of the image (GL 4.6 core 8.14.3): it + // stays put all the way down, and it is stored in whichever component sits after the + // image's own dimensions - z for a 2D array or a cube array, and HEIGHT for a 1D array, + // whose level is recorded as {width, layers, 1}. + // + // THE one statement of that rule on the frontend side, because three readers have to agree + // on it or a chain is allocated under one and judged under another: this allocator, + // ComputeMipmapCompleteForFilter (MG_State/GLState/TextureState/TextureObject.cpp, which + // uses the identical 1/2/3 split) and DirectVulkan's MipShrinkingComponentCount. It used to + // be a two-way `depthMips` flag, which had no way to say "height is not a dimension" - so + // glGenerateMipmap on a GL_TEXTURE_1D_ARRAY allocated a chain whose LAYER COUNT halved, + // and the completeness rule then rejected the texture the generate was supposed to make + // complete. The backend allocator could not repair it either: it only ever GROWS a chain, + // and the frontend's (wrong) count is always the longer of the two. + Int MipShrinkingAxisCount(TextureTarget target) { + switch (target) { + case TextureTarget::Texture1D: + // {width, 1, 1} - the other two are already 1, but say so rather than rely on it. + return 1; + case TextureTarget::Texture1DArray: + // {width, layers, 1}: height IS the layer count. + return 1; + case TextureTarget::Texture2DArray: + case TextureTarget::TextureCubeMapArray: + // {width, height, layers}: depth IS the layer count. + return 2; + case TextureTarget::Texture3D: + return 3; + default: + // 2D, cube faces, rectangle, multisample: a plain two-dimensional image. + return 2; + } + } + + // Only true 3D textures halve their depth per level; every array target keeps its layer + // count. Expressed through the rule above so the two cannot drift. Bool DepthParticipatesInMipmapping(TextureTarget target) { - return target == TextureTarget::Texture3D; + return MipShrinkingAxisCount(target) == 3; } // Which targets each glTextureStorage*D accepts (GL 4.6 core 8.19). A texture whose target @@ -427,20 +463,20 @@ namespace MobileGL::MG_Impl::GLImpl { } } - // The longest mip chain the level-0 size admits. A 1D array keeps its layer count in - // height, so unlike a 2D texture its height takes no part in the reduction. - Uint ComputeFullMipmapLevelCount(const IntVec3& baseTexelSize, Bool depthMips); + // The longest mip chain the level-0 size admits, over the axes that actually reduce. + Uint ComputeFullMipmapLevelCount(const IntVec3& baseTexelSize, Int shrinkingAxes); Uint MaxTextureStorageLevels(TextureTarget target, GLsizei width, GLsizei height, GLsizei depth) { - const Int mipHeight = (target == TextureTarget::Texture1DArray) ? 1 : std::max(height, 1); - return ComputeFullMipmapLevelCount({std::max(width, 1), mipHeight, std::max(depth, 1)}, - DepthParticipatesInMipmapping(target)); + return ComputeFullMipmapLevelCount( + {std::max(width, 1), std::max(height, 1), std::max(depth, 1)}, + MipShrinkingAxisCount(target)); } - Uint ComputeFullMipmapLevelCount(const IntVec3& baseTexelSize, Bool depthMips) { - Int maxDimension = std::max( - baseTexelSize.x(), - std::max(baseTexelSize.y(), depthMips ? std::max(baseTexelSize.z(), 1) : 1)); + Uint ComputeFullMipmapLevelCount(const IntVec3& baseTexelSize, Int shrinkingAxes) { + Int maxDimension = 1; + for (Int axis = 0; axis < shrinkingAxes && axis < 3; ++axis) { + maxDimension = std::max(maxDimension, baseTexelSize[axis]); + } Uint mipLevelCount = 1; while (maxDimension > 1) { maxDimension = std::max(maxDimension / 2, 1); @@ -449,13 +485,13 @@ namespace MobileGL::MG_Impl::GLImpl { return mipLevelCount; } - IntVec3 ComputeMipmapTexelSize(const IntVec3& baseTexelSize, Uint relativeLevel, Bool depthMips) { - return { - std::max(baseTexelSize.x() >> static_cast(relativeLevel), 1), - std::max(baseTexelSize.y() >> static_cast(relativeLevel), 1), - depthMips ? std::max(baseTexelSize.z() >> static_cast(relativeLevel), 1) - : std::max(baseTexelSize.z(), 1), - }; + IntVec3 ComputeMipmapTexelSize(const IntVec3& baseTexelSize, Uint relativeLevel, Int shrinkingAxes) { + IntVec3 size = {std::max(baseTexelSize.x(), 1), std::max(baseTexelSize.y(), 1), + std::max(baseTexelSize.z(), 1)}; + for (Int axis = 0; axis < shrinkingAxes && axis < 3; ++axis) { + size[axis] = std::max(size[axis] >> static_cast(relativeLevel), 1); + } + return size; } Bool EnsureGeneratedMipmapStorageAllocated( @@ -477,10 +513,10 @@ namespace MobileGL::MG_Impl::GLImpl { } const SizeT bytesPerTexel = baseByteSize / baseTexelCount; - const Bool depthMips = DepthParticipatesInMipmapping(texture.GetTarget()); - const Uint requiredLevelCount = ComputeFullMipmapLevelCount(baseTexelSize, depthMips); + const Int shrinkingAxes = MipShrinkingAxisCount(texture.GetTarget()); + const Uint requiredLevelCount = ComputeFullMipmapLevelCount(baseTexelSize, shrinkingAxes); for (Uint level = 1; level < requiredLevelCount; ++level) { - const IntVec3 levelTexelSize = ComputeMipmapTexelSize(baseTexelSize, level, depthMips); + const IntVec3 levelTexelSize = ComputeMipmapTexelSize(baseTexelSize, level, shrinkingAxes); const SizeT levelByteSize = bytesPerTexel * static_cast(levelTexelSize.x()) * static_cast(levelTexelSize.y()) * static_cast(levelTexelSize.z()); @@ -3686,6 +3722,59 @@ namespace MobileGL::MG_Impl::GLImpl { GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, const char* caller); + // The destination box of a copy has to lie inside the storage the copy actually WRITES, which + // is the requested (uploadTarget, level) pair's - not level 0's. + // + // This exists because the general-purpose ValidateTextureSubImageOffsets bounds everything by + // ITextureObject::GetBaseSize(), which is hardcoded to level 0 (TextureObject::GetBaseSize -> + // GetTexelSize(0, 0)). CopyReadFramebufferIntoMipmapRegion, meanwhile, sizes its rows and + // slices from GetMipmapTexelSize(uploadTarget, level) and memcpys into the exact-sized + // std::vector MipmapStorage allocated for that level, with no clamp of its own. A box that is + // legal at level 0 and out of range at level N therefore passed validation and wrote past the + // end of the heap allocation - e.g. a 4x4 copy at offset (4,4) into level 2 of an 8x8x4 + // GL_RGBA8 array texture ran 24 bytes past a 64-byte buffer. Every level > 0 of every + // mipmapped texture was reachable that way, and both entry points had been no-ops before, so + // the whole exposure arrived with their implementation. + static Bool ValidateCopySubImageRegionAtLevel(const SharedPtr& textureObject, + TextureUploadTarget uploadTarget, GLint level, GLint xoffset, + GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, + GLsizei depth, const char* caller) { + const auto* mipmapTexture = MG_State::GLState::AsMipmapTexture(textureObject.get()); + if (mipmapTexture == nullptr) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, "The destination texture has no mipmap storage.")); + return false; + } + const IntVec3 levelSize = mipmapTexture->GetMipmapTexelSize(uploadTarget, static_cast(level)); + // A level that was never defined reports a degenerate extent. GL 4.6 core 8.6 makes + // copying into an undefined texture image INVALID_OPERATION, and it is also what keeps the + // writer below from indexing an empty allocation. + if (levelSize.x() <= 0 || levelSize.y() <= 0 || levelSize.z() <= 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, + "The requested texture level has no storage.")); + return false; + } + // Signed 64-bit sums: xoffset and width are both GLint and an application may pass values + // whose sum overflows a GLint, which would otherwise compare as negative and pass. + const Int64 lastX = static_cast(xoffset) + static_cast(width); + const Int64 lastY = static_cast(yoffset) + static_cast(height); + const Int64 lastZ = static_cast(zoffset) + static_cast(depth); + if (xoffset < 0 || yoffset < 0 || zoffset < 0 || lastX > levelSize.x() || lastY > levelSize.y() || + lastZ > levelSize.z()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique( + "MG_Impl/GLImpl", caller, + std::format("The destination region does not lie inside level {} ({}x{}x{}).", level, + levelSize.x(), levelSize.y(), levelSize.z()))); + return false; + } + return true; + } + // The shared body of glCopyTexSubImage3D and glCopyTextureSubImage3D once the caller has // resolved the destination texture. `allowCubeFaceFromZOffset` is the ONE difference between // the two forms: the DSA form takes a cube map and selects the face with zoffset (GL 4.6 core @@ -3696,20 +3785,72 @@ namespace MobileGL::MG_Impl::GLImpl { GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, GLint y, GLsizei width, GLsizei height, Bool allowCubeFaceFromZOffset, const char* caller) { - if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, yoffset, zoffset, width, height, 1, caller)) { + if (!TextureImpl::ValidateTextureLevelNumber(level)) return; + if (width < 0 || height < 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", caller, "Copy dimensions must be non-negative.")); return; } + + // THE FACE MAPPING HAS TO HAPPEN BEFORE THE BOUNDS CHECK, not after it. A cube map stores + // its six faces as six upload targets of ONE z-slice each, so its GetBaseSize().z() is 1 - + // and the generic offset validator, whose z bound always comes from that, rejected every + // zoffset in 1..5 with GL_INVALID_VALUE before the mapping below could run. Five of six + // faces were unreachable through glCopyTextureSubImage3D even though the entry point + // documents zoffset as the face selector (GL 4.6 core 8.6). The cube bound is the FACE + // COUNT, which the generic validator has no way to express because its `depth` parameter + // is the copy extent; glClearTexSubImage already special-cases the same shape. TextureUploadTarget uploadTarget = GetPrimaryUploadTarget(textureObject); GLint sliceOffset = zoffset; if (allowCubeFaceFromZOffset && textureObject->GetTarget() == TextureTarget::TextureCubeMap) { + const SizeT faceCount = textureObject->GetUploadTargets().size(); + if (zoffset < 0 || static_cast(zoffset) >= faceCount) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique( + "MG_Impl/GLImpl", caller, + "zoffset selects the cube map face and must be in [0, " + std::to_string(faceCount) + ").")); + return; + } uploadTarget = static_cast( static_cast(TextureUploadTarget::CubeMapPositiveX) + static_cast(zoffset)); sliceOffset = 0; } + + if (!ValidateCopySubImageRegionAtLevel(textureObject, uploadTarget, level, xoffset, yoffset, sliceOffset, + width, height, /*depth=*/1, caller)) { + return; + } + if (!FramebufferImpl::ValidateReadFramebufferForCopy(caller)) return; CopyReadFramebufferIntoMipmapRegion(textureObject, uploadTarget, level, xoffset, yoffset, sliceOffset, x, y, width, height, caller); } + // The same for the one-dimensional pair. A 1D level is {width, 1, 1}, so the y and z arms of + // the check above are trivially satisfied and the x arm is the whole rule - which is exactly + // the one that overflowed: level 2 of an 8-texel GL_RGBA8 1D texture is 8 bytes, and a 4-texel + // copy at xoffset 4 wrote 16 bytes starting 16 bytes in, entirely outside the allocation. + static void CopyTextureSubImage1DResolved(const SharedPtr& textureObject, + GLint level, GLint xoffset, GLint x, GLint y, GLsizei width, + const char* caller) { + if (!TextureImpl::ValidateTextureLevelNumber(level)) return; + if (width < 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", caller, "Copy dimensions must be non-negative.")); + return; + } + const TextureUploadTarget uploadTarget = GetPrimaryUploadTarget(textureObject); + if (!ValidateCopySubImageRegionAtLevel(textureObject, uploadTarget, level, xoffset, /*yoffset=*/0, + /*zoffset=*/0, width, /*height=*/1, /*depth=*/1, caller)) { + return; + } + if (!FramebufferImpl::ValidateReadFramebufferForCopy(caller)) return; + CopyReadFramebufferIntoMipmapRegion(textureObject, uploadTarget, level, xoffset, /*yoffset=*/0, + /*zoffset=*/0, x, y, width, /*height=*/1, caller); + } + void CopyTexSubImage3D_State(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, GLint y, GLsizei width, GLsizei height) { // GL 4.6 core 8.6 table: the three-dimensional form of the bound-texture copy accepts @@ -4108,9 +4249,7 @@ namespace MobileGL::MG_Impl::GLImpl { const auto textureUploadTarget = MG_Util::ConvertGLEnumToTextureUploadTarget(target); auto& textureObject = GetTextureObjectByTarget(textureUploadTarget, textureTarget); if (!textureObject) return; - if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, 0, 0, width, 1, 1, __func__)) return; - CopyReadFramebufferIntoMipmapRegion(textureObject, GetPrimaryUploadTarget(textureObject), level, xoffset, - /*yoffset=*/0, /*zoffset=*/0, x, y, width, /*height=*/1, __func__); + CopyTextureSubImage1DResolved(textureObject, level, xoffset, x, y, width, __func__); } Bool CopyTexImage2D_State(GLenum target, GLint level, GLenum internalformat, GLint x, GLint y, GLsizei width, @@ -6725,9 +6864,7 @@ namespace MobileGL::MG_Impl::GLImpl { "CopyTextureSubImage1D requires a 1D texture.")); return; } - if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, 0, 0, width, 1, 1, __func__)) return; - CopyReadFramebufferIntoMipmapRegion(textureObject, GetPrimaryUploadTarget(textureObject), level, xoffset, - /*yoffset=*/0, /*zoffset=*/0, x, y, width, /*height=*/1, __func__); + CopyTextureSubImage1DResolved(textureObject, level, xoffset, x, y, width, __func__); } void CopyTextureSubImage3D(GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index 75076420..85680970 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -650,6 +650,12 @@ namespace MobileGL::MG_State { // a graphics program carrying a compute module, which Adreno 830 does not reject // from vkCreateGraphicsPipelines - it SIGSEGVs inside it. Bool anyStage = false; + // Which stages the composite ACTUALLY got a shader for. Not the same question as + // "which stages have a stage program bound": one program bound with + // GL_ALL_SHADER_BITS occupies every slot while contributing a shader to only the + // stages it was linked with. The transform-feedback capture stage is chosen off this, + // because it has to be the stage that will exist in the composite's own link. + Bool compositeHasStage[ProgramPipelineObject::kGraphicsStageCount] = {}; for (SizeT stage = 0; stage < ProgramPipelineObject::kGraphicsStageCount; ++stage) { const auto& stageProgram = pipeline->GetStageProgram(static_cast(stage)); if (!stageProgram) continue; @@ -665,6 +671,7 @@ namespace MobileGL::MG_State { if (!ref.shader || static_cast(ref.shader->GetShaderStage()) != stage) continue; composite->AttachShaderWithPinnedLinkInput(ref); anyStage = true; + compositeHasStage[stage] = true; } } if (!anyStage) return nullProgram; @@ -672,20 +679,47 @@ namespace MobileGL::MG_State { // (GL 4.6 core 11.1.2.1), and glTransformFeedbackVaryings is per-PROGRAM state that // only the stage program carrying that stage can have been given. The composite is // assembled out of the stage programs' shaders and inherits none of their - // GL-thread-owned request state, so without this the composite links with an empty - // capture list and glBeginTransformFeedback rejects the draw with INVALID_OPERATION - // ("the program has no transform feedback varyings") even though - // glValidateProgramPipeline had just passed. Same resolution order as - // ProgramLinkTask::ResolveTransformFeedbackVaryings: geometry, else tessellation - // evaluation, else vertex. + // GL-thread-owned state, so without this it links with an empty capture list and + // glBeginTransformFeedback rejects the draw with INVALID_OPERATION ("the program has + // no transform feedback varyings") even though glValidateProgramPipeline had passed. + // + // TWO RULES, both easy to get subtly wrong and both load-bearing: + // + // (1) THE LINKED LIST, NOT THE PENDING REQUEST. glTransformFeedbackVaryings does not + // take effect until the program's next link (GL 4.6 core 7.3/11.1.2.1), and it + // deliberately bumps no version - so a request written after the stage program's + // last link is invisible to the composite cache's signature yet would be picked up + // by the next rebuild, making the capture list depend on whether some unrelated + // event happened to invalidate the cache. Worse, a name that is not an output of + // the capture stage fails the composite's OWN link, and a failed composite makes + // every draw through the pipeline report INVALID_OPERATION. Reading the LINKED + // snapshot removes the whole class: linked state only moves at a link, and a link + // is exactly what ComputeDrawProgramSignature's per-stage link version tracks, so + // the existing cache key is sufficient by construction. + // GetTransformFeedbackInterfaceNames() is the right accessor rather than the + // resolved xfbVaryings: it is the request as that link consumed it, pseudo-varyings + // (gl_NextBuffer / gl_SkipComponentsN) included, which is what re-issuing it needs. + // + // (2) THE FIRST STAGE THAT EXISTS, not the first with something to capture. This is + // the rule ProgramLinkTask::ResolveTransformFeedbackVaryings applies (it breaks on + // getIntermediate(stage) != nullptr), and the two MUST agree: this loop picks + // WHOSE list, the link task picks WHICH stage's outputs the names resolve against. + // Skipping a geometry stage that has no capture list and installing the vertex + // stage's instead made them disagree, and the composite then resolved a vertex + // program's names against the geometry intermediate - capturing where GL says it + // must not, or failing the link and killing every draw. A capture stage with an + // empty list is not a reason to look further down: it is the answer, and + // glBeginTransformFeedback's INVALID_OPERATION is the correct consequence. for (const ShaderStage captureStage: {ShaderStage::Geometry, ShaderStage::TessEval, ShaderStage::Vertex}) { + if (!compositeHasStage[static_cast(captureStage)]) continue; const auto& captureProgram = pipeline->GetStageProgram(captureStage); if (!captureProgram) continue; - const auto& requested = captureProgram->GetRequestedTransformFeedbackVaryings(); - if (requested.empty()) continue; - composite->SetTransformFeedbackVaryings(Vector(requested), - captureProgram->GetRequestedTransformFeedbackBufferMode()); + const auto& linkedNames = captureProgram->GetTransformFeedbackInterfaceNames(); + if (!linkedNames.empty()) { + composite->SetTransformFeedbackVaryings(Vector(linkedNames), + captureProgram->GetTransformFeedbackBufferMode()); + } break; } // A pipeline with no fragment stage still rasterises, so the default fragment diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp index 90c0a9b3..2c163fd8 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp @@ -540,6 +540,33 @@ namespace MobileGL::MG_State::GLState { task->in.explicitFragDataIndex = m_explicitFragDataIndex; task->in.requestedXfbVaryings = m_requestedXfbVaryings; task->in.requestedXfbBufferMode = m_requestedXfbBufferMode; + // ARB_gl_spirv: a program built from SPIR-V declares its transform feedback through + // XfbBuffer/XfbStride/Offset DECORATIONS, and glTransformFeedbackVaryings has no effect on + // it at all. glSpecializeShader translated those decorations into the equivalent name + // request (ShaderCompiler::SpecializeAndDecompileSpirvModule), and this is where it enters + // the link - so everything downstream, the frontend packer and both backends, sees one + // declaration form instead of two. + // + // The capture stage is the LAST vertex-processing stage the program has, which is the same + // rule ProgramLinkTask::ResolveTransformFeedbackVaryings resolves the names against. The + // application's own request wins if it made one: that can only happen on a mixed program, + // which is not a shape ARB_gl_spirv defines, and honouring what the application explicitly + // asked for is the safer of the two readings. + if (task->in.requestedXfbVaryings.empty()) { + for (const ShaderStage captureStage: + {ShaderStage::Geometry, ShaderStage::TessEval, ShaderStage::Vertex}) { + Bool stagePresent = false; + for (const auto& shader : m_shaders) { + if (!shader || shader->GetShaderStage() != captureStage) continue; + stagePresent = true; + if (shader->GetSpirvXfbVaryings().empty()) continue; + task->in.requestedXfbVaryings = shader->GetSpirvXfbVaryings(); + task->in.requestedXfbBufferMode = shader->GetSpirvXfbBufferMode(); + break; + } + if (stagePresent) break; + } + } task->in.maxFragmentOutputColorNumber = m_maxFragmentOutputColorNumber; Vector> deps; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index bad574f1..5424d6d6 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -1536,14 +1536,14 @@ namespace MobileGL::MG_State::GLState { m_requestedXfbVaryings = Move(names); m_requestedXfbBufferMode = bufferMode; } - // The REQUEST, not the linked result: what glTransformFeedbackVaryings last recorded, - // which the next link will try to resolve. A program pipeline's draw composite reads it - // off the capturing stage program and re-issues it on itself, because the composite is - // built from the stage programs' SHADERS and would otherwise inherit no capture list at - // all - which made glBeginTransformFeedback reject every separable-program capture - // (glcSeparableProgramsTransformFeedbackTests). - const Vector& GetRequestedTransformFeedbackVaryings() const { return m_requestedXfbVaryings; } - GLenum GetRequestedTransformFeedbackBufferMode() const { return m_requestedXfbBufferMode; } + // NO ACCESSOR FOR THE PENDING REQUEST, deliberately. A program pipeline's draw composite + // needs the capture list of the stage program it flattens, and the obvious source - what + // glTransformFeedbackVaryings last recorded - is the wrong one: that request does not take + // effect until the stage program's next link, and it bumps no version, so reading it makes + // the composite's capture list depend on when the composite cache happened to be + // invalidated. GetTransformFeedbackInterfaceNames() below is the source that is correct + // AND cache-safe, because linked state only moves at a link and the composite signature + // already keys on the link version. See GLContext::GetProgramForDraw. GLenum GetTransformFeedbackBufferMode() const { return Artifacts().xfbBufferMode; } SizeT GetTransformFeedbackVaryingCount() const { return Artifacts().xfbVaryings.size(); } const XfbVarying* GetTransformFeedbackVarying(SizeT index) const { diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp index bcf65d2e..346444ba 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp @@ -21,16 +21,31 @@ namespace MobileGL::MG_State::GLState { ReleaseCompileNode(); m_spirvBinary = Move(binary); m_hasSpirvBinary = true; + m_specialized = false; m_specializationFailed = false; m_specializationInfoLog.clear(); + m_spirvXfbVaryings.clear(); + m_spirvXfbBufferMode = GL_INTERLEAVED_ATTRIBS; m_source = MakeShared(String{}); InvalidateCompiledState(); } - void ShaderObject::SpecializeFromSpirv(String&& glsl) { + const String& ShaderObject::GetApplicationShaderSource() const { + static const String kNoSource; + // Both the unspecialized and the specialized windows answer empty: in the first m_source + // already is empty, in the second it holds generated GLSL that the application never wrote. + return m_hasSpirvBinary ? kNoSource : *m_source; + } + + void ShaderObject::SpecializeFromSpirv(String&& glsl, Vector&& xfbVaryings, GLenum xfbBufferMode) { ReleaseCompileNode(); + // The latch goes up HERE and nowhere else - this is the one path that actually specialized + // the shader. + m_specialized = true; m_specializationFailed = false; m_specializationInfoLog.clear(); + m_spirvXfbVaryings = Move(xfbVaryings); + m_spirvXfbBufferMode = xfbBufferMode; // The GLSL the module specializes to enters the ORDINARY pipeline from here: preprocess, // glslang parse, reflection, transpile, both backends. Nothing downstream needs to know // the source was not written by the application - which is the whole reason this hop @@ -61,8 +76,11 @@ namespace MobileGL::MG_State::GLState { m_hasSpirvBinary = false; m_spirvBinary.clear(); m_spirvBinary.shrink_to_fit(); + m_specialized = false; m_specializationFailed = false; m_specializationInfoLog.clear(); + m_spirvXfbVaryings.clear(); + m_spirvXfbBufferMode = GL_INTERLEAVED_ATTRIBS; ReleaseCompileNode(); m_source = MakeShared(source); InvalidateCompiledState(); diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.h b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.h index 9dcf7023..5c463b1b 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.h @@ -78,10 +78,33 @@ namespace MobileGL { // checks explicitly. void SetSpirvBinary(Vector&& binary); Bool HasSpirvBinary() const { return m_hasSpirvBinary; } + // ARB_gl_spirv: "Once specialized, a shader may not be re-specialized without first + // re-associating the original SPIR-V module with it, through ShaderBinary." A second + // glSpecializeShader is GL_INVALID_OPERATION, and this latch is what answers that. + // + // Set ONLY on the success path. A specialization that FAILED did not specialize the + // shader, and the conformance suite relies on that distinction: it deliberately fails + // specialization (a bad entry point, then an unknown constant id) on one shader object + // and then requires the next, well-formed call on that same object to be accepted. + Bool HasBeenSpecialized() const { return m_specialized; } const Vector& GetSpirvBinary() const { return m_spirvBinary; } // glSpecializeShader's half: hand the object the GLSL its module specializes to and // let the ordinary pipeline compile it. - void SpecializeFromSpirv(String&& glsl); + void SpecializeFromSpirv(String&& glsl, Vector&& xfbVaryings, GLenum xfbBufferMode); + // The capture the object's SPIR-V module DECLARED, as the equivalent + // glTransformFeedbackVaryings request. Empty for a GLSL shader and for a SPIR-V module + // that declares no transform feedback. ProgramObject::Link picks this up from the + // program's last vertex-processing stage, because ARB_gl_spirv makes decorations the + // only declaration form for a SPIR-V program and glTransformFeedbackVaryings has no + // effect on one. + const Vector& GetSpirvXfbVaryings() const { return m_spirvXfbVaryings; } + GLenum GetSpirvXfbBufferMode() const { return m_spirvXfbBufferMode; } + // What glGetShaderSource / GL_SHADER_SOURCE_LENGTH must answer. A shader created from + // glShaderBinary never had glShaderSource called on it, so GL 4.6 core 7.1 makes its + // source the empty string - even after glSpecializeShader, when m_source holds the + // SPIRV-Cross GLSL the module was translated into. That text is MobileGL's, not the + // application's, and handing it back invites an application to cache and re-submit it. + const String& GetApplicationShaderSource() const; // The other half: specialization itself failed (a bad entry point, a constant id the // module does not declare, a module spirv-val rejects). There is nothing to compile, // so the verdict is recorded directly - COMPILE_STATUS false with this log - and both @@ -278,6 +301,13 @@ namespace MobileGL { // the ORIGINAL words rather than the ones the first call folded. Vector m_spirvBinary; Bool m_hasSpirvBinary = false; + // "This shader has been specialized"; see HasBeenSpecialized. Cleared by anything that + // re-associates a module (SetSpirvBinary) or turns the object back into a GLSL shader + // (either SetShaderSource overload) - which is exactly the re-association ARB_gl_spirv + // names as the way to make a second specialization legal again. + Bool m_specialized = false; + Vector m_spirvXfbVaryings; + GLenum m_spirvXfbBufferMode = GL_INTERLEAVED_ATTRIBS; // A specialization that failed before any compile could start. Kept beside the // compile artifacts rather than inside them because there is no compile job to hang // it on - see RecordSpecializationFailure. Cleared by anything that gives the object diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp index fe75f231..8945b5b6 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp @@ -248,11 +248,12 @@ namespace MobileGL { } void RenderState::SetPolygonOffset(Float factor, Float units) { - if (m_parameters.PolygonOffsetFactor == factor && m_parameters.PolygonOffsetUnits == units) return; - - m_parameters.PolygonOffsetFactor = factor; - m_parameters.PolygonOffsetUnits = units; - ++m_version; + // GL 4.6 core 14.6.5 defines PolygonOffset(factor, units) as EQUIVALENT to + // PolygonOffsetClamp(factor, units, 0) - the equivalence is total, so the clamp is + // written too, not merely left alone. Leaving it meant a glPolygonOffsetClamp(1, 1, + // 0.5) followed by a plain glPolygonOffset(3, 4) still reported a clamp of 0.5, and + // the early-out below could even skip the version bump while doing it. + SetPolygonOffsetClamped(factor, units, 0.0f); } Float RenderState::GetPolygonOffsetFactor() const { diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index ba00e611..7af54ec0 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -3072,14 +3072,19 @@ TEST(GetterSanity, CombinedUniformComponentsSaturateInsteadOfOverflowing) { MG_State::pGLContext = MakeUnique(); + // GL_MAX_COMBINED_COMPUTE_UNIFORM_COMPONENTS (0x8266), NOT the per-stage + // GL_MAX_COMPUTE_UNIFORM_COMPONENTS (0x8263) this list used to name. The per-stage token is + // answered by a frontend constant and never reaches GetMaxCombinedUniformComponents at all, so + // both assertions on it were vacuous - and it displaced the ONE reader whose block count comes + // from the backend (ClampUniformBlockCount(dynamicParameters.MaxComputeUniformBlocks)) rather + // than from a frontend constant, i.e. the only call site where the saturation actually depends + // on data a driver supplies. static constexpr GLenum kCombinedPnames[] = { GL_MAX_COMBINED_VERTEX_UNIFORM_COMPONENTS, GL_MAX_COMBINED_FRAGMENT_UNIFORM_COMPONENTS, GL_MAX_COMBINED_GEOMETRY_UNIFORM_COMPONENTS, GL_MAX_COMBINED_TESS_CONTROL_UNIFORM_COMPONENTS, - GL_MAX_COMBINED_TESS_EVALUATION_UNIFORM_COMPONENTS, GL_MAX_COMPUTE_UNIFORM_COMPONENTS, + GL_MAX_COMBINED_TESS_EVALUATION_UNIFORM_COMPONENTS, GL_MAX_COMBINED_COMPUTE_UNIFORM_COMPONENTS, }; - // The GL 4.6 core table 23.64 floors for the five combined pnames above; compute's per-stage - // GL_MAX_COMPUTE_UNIFORM_COMPONENTS is not a combined limit and carries its own, much smaller - // floor, so only the sign of its answer is asserted. + // The GL 4.6 core table 23.64 floor, which all six combined pnames carry. static constexpr GLint kCombinedFloor = 58368; { @@ -3091,9 +3096,7 @@ TEST(GetterSanity, CombinedUniformComponentsSaturateInsteadOfOverflowing) { GLint reported = 0; MG_Impl::GLImpl::GetIntegerv(pname, &reported); EXPECT_GT(reported, 0) << "pname 0x" << pname << " wrapped to a negative combined component count"; - if (pname != GL_MAX_COMPUTE_UNIFORM_COMPONENTS) { - EXPECT_GE(reported, kCombinedFloor) << "pname 0x" << pname << " fell under the GL 4.6 floor"; - } + EXPECT_GE(reported, kCombinedFloor) << "pname 0x" << pname << " fell under the GL 4.6 floor"; } MG_Backend::pActiveBackendObject.reset(); } diff --git a/MobileGL/MG_Test/State/RenderStateTest.cpp b/MobileGL/MG_Test/State/RenderStateTest.cpp index c89cd46e..986f056b 100644 --- a/MobileGL/MG_Test/State/RenderStateTest.cpp +++ b/MobileGL/MG_Test/State/RenderStateTest.cpp @@ -980,15 +980,25 @@ TEST_F(RenderStateTest, PolygonOffsetClampStoresTheClampAndTheFactorUnitsPair) { EXPECT_NEAR(asDouble, 0.5, 1e-6); EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); - // glPolygonOffset is the clamp = 0 case of the same state, but it must not DISTURB the clamp - // it does not take - GL 4.6 core 14.6.5 defines it as PolygonOffsetClamp(factor, units, 0) - // only in the sense that the clamp it leaves is whatever glPolygonOffset itself sets, which - // for MobileGL is "unchanged". Assert the factor/units half instead, which is unambiguous. + // GL 4.6 core 14.6.5 defines glPolygonOffset(factor, units) as EQUIVALENT to + // glPolygonOffsetClamp(factor, units, 0) - totally, not "except for the clamp". So it writes + // all three, and a clamp left over from an earlier glPolygonOffsetClamp must be gone. MG_Impl::GLImpl::PolygonOffset(3.0f, 4.0f); MG_Impl::GLImpl::GetFloatv(GL_POLYGON_OFFSET_FACTOR, &factor); MG_Impl::GLImpl::GetFloatv(GL_POLYGON_OFFSET_UNITS, &units); + MG_Impl::GLImpl::GetFloatv(GL_POLYGON_OFFSET_CLAMP, &clamp); EXPECT_FLOAT_EQ(factor, 3.0f); EXPECT_FLOAT_EQ(units, 4.0f); + EXPECT_FLOAT_EQ(clamp, 0.0f) << "glPolygonOffset IS PolygonOffsetClamp(factor, units, 0)"; + + // The same rule when factor and units do NOT change: the clamp still has to be cleared, which + // an early-out keyed on the factor/units pair alone would skip. + MG_Impl::GLImpl::PolygonOffsetClamp(3.0f, 4.0f, 0.75f); + MG_Impl::GLImpl::GetFloatv(GL_POLYGON_OFFSET_CLAMP, &clamp); + ASSERT_FLOAT_EQ(clamp, 0.75f); + MG_Impl::GLImpl::PolygonOffset(3.0f, 4.0f); + MG_Impl::GLImpl::GetFloatv(GL_POLYGON_OFFSET_CLAMP, &clamp); + EXPECT_FLOAT_EQ(clamp, 0.0f) << "a no-op factor/units write must still clear the clamp"; MG_Impl::GLImpl::PolygonOffsetClamp(0.0f, 0.0f, 0.0f); EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 7f65a0f3..00bcb0be 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -5775,3 +5775,232 @@ TEST_F(TextureTest, TextureBufferKeepsInvalidOperationForANonBufferTexture) { MG_Impl::GLImpl::DeleteBuffers(1, &buffer); DrainPendingGlErrors(); } + +// --------------------------------------------------------------------------------------------- +// The destination box of a copy is bounded by the LEVEL it writes, not by level 0. +// +// glCopyTexSubImage3D/1D validated through ValidateTextureSubImageOffsets, whose bound is +// ITextureObject::GetBaseSize() - hardcoded to level 0 - while CopyReadFramebufferIntoMipmapRegion +// indexes GetMipmapTexelSize(uploadTarget, level) and memcpys into the exact-sized allocation +// MipmapStorage made for that level. A box legal at level 0 and out of range at level N wrote past +// the end of the heap buffer. Both entry points were `// TODO: implement` no-ops before this +// branch, so implementing them is what opened the path. +// +// The region check runs BEFORE the read-framebuffer check on purpose, which is what lets this +// GPU-free binary assert it: no complete read FBO is needed to prove the box was rejected. +// --------------------------------------------------------------------------------------------- + +TEST_F(TextureTest, CopyTexSubImage3DBoundsTheDestinationByTheRequestedLevelNotLevelZero) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D_ARRAY, 1, &texture); + // 4 levels of an 8x8x4 array: level 0 is 8x8, level 1 4x4, level 2 2x2; the layer count stays + // 4 at every level. + MG_Impl::GLImpl::TextureStorage3D(texture, 4, GL_RGBA8, 8, 8, 4); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_ARRAY, texture); + DrainPendingGlErrors(); + + // THE OVERFLOW: 4+4 <= 8 and 4+4 <= 8 against level 0, but level 2 is only 2x2. This used to + // pass validation and write 24 bytes past a 64-byte allocation. + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 2, 4, 4, 0, 0, 0, 4, 4); + ExpectSingleGlError(GL_INVALID_VALUE); + + // The same box one axis at a time, so a check that only looked at x or only at y cannot pass. + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 1, 3, 0, 0, 0, 0, 2, 2); + ExpectSingleGlError(GL_INVALID_VALUE); + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 1, 0, 3, 0, 0, 0, 2, 2); + ExpectSingleGlError(GL_INVALID_VALUE); + + // A box that IS inside level 1 (4x4) must get past the region check. It cannot complete here - + // this binary has no complete read framebuffer - but it must not be the box that is refused. + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 1, 2, 2, 3, 0, 0, 2, 2); + EXPECT_NE(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE) + << "an in-range level-1 box must reach the framebuffer check, not be rejected as out of range"; + DrainPendingGlErrors(); + + // The layer axis is bounded by the level's layer count, which does NOT shrink down the chain. + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 1, 0, 0, 4, 0, 0, 2, 2); + ExpectSingleGlError(GL_INVALID_VALUE); + + // A level the texture never had is INVALID_OPERATION, not a write into an empty allocation. + MG_Impl::GLImpl::CopyTexSubImage3D(GL_TEXTURE_2D_ARRAY, 5, 0, 0, 0, 0, 0, 1, 1); + ExpectSingleGlError(GL_INVALID_OPERATION); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_ARRAY, 0); + DrainPendingGlErrors(); +} + +TEST_F(TextureTest, CopyTexSubImage1DBoundsTheDestinationByTheRequestedLevel) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_1D, 1, &texture); + MG_Impl::GLImpl::TextureStorage1D(texture, 4, GL_RGBA8, 8); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D, texture); + DrainPendingGlErrors(); + + // Level 2 is two texels, i.e. eight bytes; this used to write sixteen bytes starting sixteen + // bytes in - entirely outside the allocation. + MG_Impl::GLImpl::CopyTexSubImage1D(GL_TEXTURE_1D, 2, 4, 0, 0, 4); + ExpectSingleGlError(GL_INVALID_VALUE); + + // In range at level 1 (four texels). + MG_Impl::GLImpl::CopyTexSubImage1D(GL_TEXTURE_1D, 1, 2, 0, 0, 2); + EXPECT_NE(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D, 0); + DrainPendingGlErrors(); +} + +// glCopyTextureSubImage3D documents zoffset as the cube-map FACE selector, but the face mapping +// ran AFTER a z-bounds check taken from GetBaseSize().z(), which for a cube map is one face's +// depth - i.e. 1. Every zoffset in 1..5 was rejected with GL_INVALID_VALUE, so five of the six +// faces were unreachable. The mapping now runs first and is bounded by the face count. +TEST_F(TextureTest, CopyTextureSubImage3DCanAddressEveryCubeMapFace) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_CUBE_MAP, 1, &texture); + MG_Impl::GLImpl::TextureStorage2D(texture, 2, GL_RGBA8, 4, 4); + DrainPendingGlErrors(); + + for (GLint face = 0; face < 6; ++face) { + MG_Impl::GLImpl::CopyTextureSubImage3D(texture, 0, 0, 0, face, 0, 0, 4, 4); + EXPECT_NE(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE) + << "face " << face << " must be reachable; the z bound is the face count, not a face's depth"; + DrainPendingGlErrors(); + } + + // Past the last face is still GL_INVALID_VALUE. + MG_Impl::GLImpl::CopyTextureSubImage3D(texture, 0, 0, 0, 6, 0, 0, 4, 4); + ExpectSingleGlError(GL_INVALID_VALUE); + MG_Impl::GLImpl::CopyTextureSubImage3D(texture, 0, 0, 0, -1, 0, 0, 4, 4); + ExpectSingleGlError(GL_INVALID_VALUE); + + // And the per-FACE extent still bounds x/y at the requested level: level 1 of a 4x4 cube is + // 2x2, so a 4x4 box into face 3 is out of range even though it fits level 0. + MG_Impl::GLImpl::CopyTextureSubImage3D(texture, 1, 0, 0, 3, 0, 0, 4, 4); + ExpectSingleGlError(GL_INVALID_VALUE); + + DrainPendingGlErrors(); +} + +// --------------------------------------------------------------------------------------------- +// glGenerateMipmap allocates the chain in the FRONTEND before it dispatches to the backend, and +// that allocator used a two-way "does depth mip?" flag which had no way to say that a 1D array's +// HEIGHT is its layer count. It therefore both counted the layer axis into the chain length and +// halved it per level. The backend allocator cannot repair that - it only ever GROWS a chain, and +// the frontend's (wrong) count is always the longer one - so the layer-shrinking chain survived on +// both backends, and ComputeMipmapCompleteForFilter (which knows height is not a dimension for +// this target) then judged the texture mipmap-INCOMPLETE, i.e. sampling returns (0,0,0,1). +// --------------------------------------------------------------------------------------------- + +namespace { + // glGenerateMipmap dispatches to the backend after the frontend allocation; this binary has no + // GL context, so the hook is stubbed for the duration of the case. What is under test is the + // frontend allocation the stub cannot influence. + struct ScopedNoOpGenerateMipmap { + ScopedNoOpGenerateMipmap(): m_snapshot(MobileGL::MG_Backend::gBackendFunctionsTable) { + MobileGL::MG_Backend::gBackendFunctionsTable.GL.GenerateMipmap = [](GLenum) {}; + } + ~ScopedNoOpGenerateMipmap() { MobileGL::MG_Backend::gBackendFunctionsTable = m_snapshot; } + ScopedNoOpGenerateMipmap(const ScopedNoOpGenerateMipmap&) = delete; + ScopedNoOpGenerateMipmap& operator=(const ScopedNoOpGenerateMipmap&) = delete; + + private: + MobileGL::MG_Backend::GlobalBackendFunctionsTable m_snapshot; + }; + + GLint LevelParam(GLenum target, GLint level, GLenum pname) { + GLint value = -1; + MG_Impl::GLImpl::GetTexLevelParameteriv(target, level, pname, &value); + return value; + } +} // namespace + +TEST_F(TextureTest, GenerateMipmapKeepsA1DArrayLayerCountAtEveryLevel) { + ScopedNoOpGenerateMipmap noOpBackend; + + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D_ARRAY, texture); + // Width 8, FOUR layers. The layer count is carried in `height` for this target. + MG_Impl::GLImpl::TexImage2D(GL_TEXTURE_1D_ARRAY, 0, GL_RGBA8, 8, 4, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::GenerateMipmap(GL_TEXTURE_1D_ARRAY); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // The chain length comes from the WIDTH alone: 8 -> 4 -> 2 -> 1 is four levels. Counting the + // layer axis too would give the same four here, so the width is chosen larger than the layer + // count on purpose and the layer assertions below are what actually discriminate. + for (GLint level = 0; level < 4; ++level) { + EXPECT_EQ(LevelParam(GL_TEXTURE_1D_ARRAY, level, GL_TEXTURE_WIDTH), std::max(8 >> level, 1)) + << "level " << level << " width"; + EXPECT_EQ(LevelParam(GL_TEXTURE_1D_ARRAY, level, GL_TEXTURE_HEIGHT), 4) + << "level " << level << " must keep all four layers; height is the layer count for a 1D array"; + } + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D_ARRAY, 0); + DrainPendingGlErrors(); +} + +// The mirror case: more layers than texels. The chain must be as long as the WIDTH admits, not as +// long as the layer count admits - a chain sized off the layers would allocate levels whose width +// has already bottomed out at 1 while the layer count kept halving. +TEST_F(TextureTest, GenerateMipmapSizesA1DArrayChainFromWidthAloneEvenWithMoreLayersThanTexels) { + ScopedNoOpGenerateMipmap noOpBackend; + + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D_ARRAY, texture); + // Width 2, sixteen layers: counting the layer axis would ask for five levels, the width for two. + MG_Impl::GLImpl::TexImage2D(GL_TEXTURE_1D_ARRAY, 0, GL_RGBA8, 2, 16, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::GenerateMipmap(GL_TEXTURE_1D_ARRAY); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + EXPECT_EQ(LevelParam(GL_TEXTURE_1D_ARRAY, 1, GL_TEXTURE_WIDTH), 1); + EXPECT_EQ(LevelParam(GL_TEXTURE_1D_ARRAY, 1, GL_TEXTURE_HEIGHT), 16); + // Level 2 must not exist: the chain ends where the width does. + EXPECT_EQ(LevelParam(GL_TEXTURE_1D_ARRAY, 2, GL_TEXTURE_WIDTH), 0); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_1D_ARRAY, 0); + DrainPendingGlErrors(); +} + +// The 2D-array/cube-array side of the same rule, so a fix that swung the other way (making depth +// mip-able again) cannot pass. Depth is the layer count for these; only width and height reduce. +TEST_F(TextureTest, GenerateMipmapKeepsA2DArrayLayerCountAtEveryLevel) { + ScopedNoOpGenerateMipmap noOpBackend; + + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_ARRAY, texture); + MG_Impl::GLImpl::TexImage3D(GL_TEXTURE_2D_ARRAY, 0, GL_RGBA8, 8, 8, 3, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::GenerateMipmap(GL_TEXTURE_2D_ARRAY); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + for (GLint level = 0; level < 4; ++level) { + EXPECT_EQ(LevelParam(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_WIDTH), std::max(8 >> level, 1)); + EXPECT_EQ(LevelParam(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_HEIGHT), std::max(8 >> level, 1)); + EXPECT_EQ(LevelParam(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_DEPTH), 3) + << "level " << level << " must keep all three layers"; + } + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // And a true 3D texture still halves all three, which is the case the layer rule must not eat. + GLuint volume = 0; + MG_Impl::GLImpl::GenTextures(1, &volume); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_3D, volume); + MG_Impl::GLImpl::TexImage3D(GL_TEXTURE_3D, 0, GL_RGBA8, 8, 8, 8, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + DrainPendingGlErrors(); + MG_Impl::GLImpl::GenerateMipmap(GL_TEXTURE_3D); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + EXPECT_EQ(LevelParam(GL_TEXTURE_3D, 1, GL_TEXTURE_DEPTH), 4); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_3D, 0); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_ARRAY, 0); + DrainPendingGlErrors(); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index ac5aa4a1..fc03b021 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -1702,6 +1702,54 @@ namespace MobileGL { return SpvExecutionModelFragment; } } + // The decorated capture layout, as the equivalent glTransformFeedbackVaryings + // request. GL 4.6 core 11.1.2.1 / ARB_transform_feedback3 give the name list two + // pseudo-varyings that are exactly what a decoration layout needs: gl_NextBuffer + // moves to the next capture buffer, and gl_SkipComponentsN (N in 1..4) advances the + // cursor without capturing. Together they can express any offset/stride layout + // whose offsets are component-aligned, which SPIR-V's are (Offset is in bytes and + // xfb offsets are four-byte aligned by rule). + Vector BuildXfbVaryingRequest(const Vector& captures) { + Vector names; + if (captures.empty()) return names; + + auto emitSkip = [&names](Uint32 components) { + while (components > 0) { + const Uint32 step = std::min(components, 4); + names.push_back("gl_SkipComponents" + std::to_string(step)); + components -= step; + } + }; + + Uint32 currentBuffer = captures.front().buffer; + Uint32 cursorComponents = 0; + Uint32 currentStride = 0; + // Buffers below the first captured one still have to be stepped over, so the + // Nth gl_NextBuffer really does land on buffer N. + for (Uint32 buffer = 0; buffer < currentBuffer; ++buffer) { + names.push_back("gl_NextBuffer"); + } + for (const SpirvXfbCapture& capture : captures) { + if (capture.buffer != currentBuffer) { + // Pad the buffer being left out to its declared stride, so the record + // size the module asked for survives. + if (currentStride / 4 > cursorComponents) emitSkip(currentStride / 4 - cursorComponents); + for (Uint32 buffer = currentBuffer; buffer < capture.buffer; ++buffer) { + names.push_back("gl_NextBuffer"); + } + currentBuffer = capture.buffer; + cursorComponents = 0; + currentStride = 0; + } + const Uint32 offsetComponents = capture.offset / 4; + if (offsetComponents > cursorComponents) emitSkip(offsetComponents - cursorComponents); + names.push_back(capture.name); + cursorComponents = offsetComponents + capture.componentCount; + currentStride = std::max(currentStride, capture.stride); + } + if (currentStride / 4 > cursorComponents) emitSkip(currentStride / 4 - cursorComponents); + return names; + } } // namespace Result ShaderCompiler::ValidateSpirvModule(const Vector& spirv) { @@ -1734,15 +1782,29 @@ namespace MobileGL { return {}; } - Result ShaderCompiler::SpecializeAndDecompileSpirvModule(const Vector& spirv, - GLenum shaderType, - const String& entryPoint, - const Vector& constantIds, - const Vector& constantValues) { + Result ShaderCompiler::SpecializeAndDecompileSpirvModule( + const Vector& spirv, GLenum shaderType, const String& entryPoint, + const Vector& constantIds, const Vector& constantValues, + SpecializationFailure& outFailure) { + outFailure = SpecializationFailure::None; + SpvcSession session(spirv, SessionUsageBit::Transpile); + if (!session.IsTranspileReady()) { + // SPIRV-Cross could not parse the module. glShaderBinary's spirv-val pass is a + // validity check, not a parseability one, so this is reachable with a module + // that validates - hence a diagnosis rather than the null dereference the + // unchecked constructor used to walk into. + outFailure = SpecializationFailure::ModuleRejected; + ResultInfo r; + r.errc = -11; + r.log = "Error: [ARB_gl_spirv] the module could not be parsed:\n" + + String(session.GetLastErrorString()); + return std::unexpected(r); + } Uint32 unknownConstantId = 0; if (!session.SetSpecializationConstants(constantIds, constantValues, unknownConstantId)) { + outFailure = SpecializationFailure::UnknownConstantId; ResultInfo r; r.errc = -7; r.log = "Error: [ARB_gl_spirv] constant index " + std::to_string(unknownConstantId) + @@ -1750,19 +1812,32 @@ namespace MobileGL { return std::unexpected(r); } - if (!entryPoint.empty()) { - if (session.SetEntryPoint(entryPoint.c_str(), ExecutionModelForShaderType(shaderType)) != - SPVC_SUCCESS) { - ResultInfo r; - r.errc = -8; - r.log = "Error: [ARB_gl_spirv] the module has no entry point named '" + entryPoint + - "' for this shader stage:\n" + String(session.GetLastErrorString()); - return std::unexpected(r); - } + // No `if (!entryPoint.empty())` guard any more. ARB_gl_spirv makes pEntryPoint the + // name of the entry point to specialize, and no module carries one named ""; the + // guard turned an empty name into "whichever entry point happens to be default", + // which is neither what the application asked for nor an error it was told about. + if (session.SetEntryPoint(entryPoint.c_str(), ExecutionModelForShaderType(shaderType)) != + SPVC_SUCCESS) { + outFailure = SpecializationFailure::UnknownEntryPoint; + ResultInfo r; + r.errc = -8; + r.log = "Error: [ARB_gl_spirv] the module has no entry point named '" + entryPoint + + "' for this shader stage:\n" + String(session.GetLastErrorString()); + return std::unexpected(r); } + // Read the declared capture layout, then REMOVE the decorations that describe it. + // Both halves matter: without the read a SPIR-V program captures nothing, and + // without the strip the decorations round-trip through the emitted GLSL back into + // the regenerated SPIR-V, where DirectGLES's ESSL hop refuses them outright and + // loses the stage. See SpvcSession::StripTransformFeedbackDecorations. + SpecializedModule specialized; + specialized.xfbVaryings = BuildXfbVaryingRequest(session.ReflectTransformFeedbackCaptures()); + session.StripTransformFeedbackDecorations(); + spvc_compiler_options options; if (session.CreateOptions(&options) != SPVC_SUCCESS) { + outFailure = SpecializationFailure::ModuleRejected; ResultInfo r; r.errc = -9; r.log = "Error: [ARB_gl_spirv] could not create SPIRV-Cross options for the module."; @@ -1786,13 +1861,15 @@ namespace MobileGL { const char* emitted = nullptr; session.Compile(&emitted); if (!emitted) { + outFailure = SpecializationFailure::ModuleRejected; ResultInfo r; r.errc = -10; r.log = "Error: [ARB_gl_spirv] could not translate the module to GLSL:\n" + String(session.GetLastErrorString()); return std::unexpected(r); } - return String(emitted); + specialized.glsl = String(emitted); + return specialized; } Result ShaderCompiler::DecompileShader(SpvcSession& session) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 3264c56f..07cdbe7a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -434,10 +434,43 @@ namespace MobileGL { // `constantIds` and `constantValues` are the parallel arrays the entry point // takes. A constant id the module does not declare is GL_INVALID_VALUE per the // extension; it is reported through the error log rather than silently ignored. - static Result SpecializeAndDecompileSpirvModule(const Vector& spirv, - GLenum shaderType, const String& entryPoint, - const Vector& constantIds, - const Vector& constantValues); + // Why the caller needs a REASON and not just a failure: ARB_gl_spirv splits the + // ways specialization can fail into two groups with different GL surfaces. A bad + // entry-point name and a constant id the module does not declare are enumerated + // errors - GL_INVALID_VALUE, and, being errors, they must leave the shader object + // exactly as it was. Everything else (a module SPIRV-Cross cannot translate) is a + // COMPILE failure, reported through COMPILE_STATUS and the info log like any other + // glCompileShader outcome. Returning one undifferentiated error is what made both + // groups look like the second. + enum class SpecializationFailure { + None, + UnknownConstantId, // GL_INVALID_VALUE + UnknownEntryPoint, // GL_INVALID_VALUE + ModuleRejected, // COMPILE_STATUS false + info log + }; + + // What a specialized module turns into: the GLSL the ordinary pipeline compiles, + // plus the transform-feedback capture the module DECLARED, re-expressed as the + // glTransformFeedbackVaryings request that produces the same layout. + // + // The re-expression is the whole design. ARB_gl_spirv makes XfbBuffer/XfbStride/ + // Offset decorations the only way a SPIR-V program declares capture, and MobileGL's + // capture machinery - the frontend packer, DirectGLES's forwarding to the ES + // driver, DirectVulkan's XfbCaptureDecoratePass - is driven entirely by a name + // list. Translating the decorations into the equivalent name list (with + // ARB_transform_feedback3's gl_NextBuffer / gl_SkipComponentsN spelling carrying + // the buffer breaks and the gaps) hands a SPIR-V program to the machinery that + // already exists, instead of teaching every consumer a second declaration form. + struct SpecializedModule { + String glsl; + Vector xfbVaryings; + GLenum xfbBufferMode = GL_INTERLEAVED_ATTRIBS; + }; + + static Result SpecializeAndDecompileSpirvModule( + const Vector& spirv, GLenum shaderType, const String& entryPoint, + const Vector& constantIds, const Vector& constantValues, + SpecializationFailure& outFailure); // spirv-val over an application-supplied module, against the environment MobileGL // parses and emits under. glShaderBinary is where a malformed module has to be diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp index 622f92d7..3faa175b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp @@ -8,6 +8,8 @@ #include "SpvcSession.h" +#include + namespace MobileGL { namespace MG_Util { namespace ShaderTranspiler { @@ -184,11 +186,29 @@ namespace MobileGL { const SpvId* p_spirv = spirv.data(); size_t word_count = spirv.size(); - spvc_context_create(&context); - spvc_context_parse_spirv(context, p_spirv, word_count, &ir); - spvc_context_create_compiler(context, SPVC_BACKEND_GLSL, ir, SPVC_CAPTURE_MODE_TAKE_OWNERSHIP, - &compiler); - spvc_compiler_create_shader_resources(compiler, &resources); + // Every step is checked, and each guards the next: the C API writes its + // out-param only on success, so passing a failed step's null handle to the + // step after it is a raw dereference (spvc_context_create_compiler does + // `parsed_ir->parsed`, spvc_compiler_create_shader_resources does + // `compiler->context`). IsTranspileReady() is how a caller asks whether this + // sequence got all the way through. + if (spvc_context_create(&context) != SPVC_SUCCESS) { + context = nullptr; + return; + } + if (spvc_context_parse_spirv(context, p_spirv, word_count, &ir) != SPVC_SUCCESS) { + ir = nullptr; + return; + } + if (spvc_context_create_compiler(context, SPVC_BACKEND_GLSL, ir, + SPVC_CAPTURE_MODE_TAKE_OWNERSHIP, &compiler) != SPVC_SUCCESS) { + compiler = nullptr; + return; + } + if (spvc_compiler_create_shader_resources(compiler, &resources) != SPVC_SUCCESS) { + resources = nullptr; + return; + } } else if (usage & SessionUsageBit::Reflection) { SpvReflectResult result = spvReflectCreateShaderModule( spirv.size() * sizeof(uint32_t), spirv.data(), &reflectModule); @@ -496,8 +516,144 @@ namespace MobileGL { SPVC_CHK_RETURN } + namespace { + // How many 32-bit components a captured variable occupies, which is what the + // gl_SkipComponentsN padding below is counted in. Matrices and arrays multiply. + Uint32 XfbComponentCount(spvc_compiler compiler, spvc_type_id typeId) { + const spvc_type type = spvc_compiler_get_type_handle(compiler, typeId); + if (type == nullptr) return 0; + Uint32 components = spvc_type_get_vector_size(type) * spvc_type_get_columns(type); + const unsigned dimensions = spvc_type_get_num_array_dimensions(type); + for (unsigned d = 0; d < dimensions; ++d) { + const unsigned length = spvc_type_get_array_dimension(type, d); + if (length != 0) components *= length; + } + // A double occupies two component slots per scalar (GL 4.6 core 11.1.2.1). + const spvc_basetype base = spvc_type_get_basetype(type); + if (base == SPVC_BASETYPE_FP64 || base == SPVC_BASETYPE_INT64 || + base == SPVC_BASETYPE_UINT64) { + components *= 2; + } + return components; + } + } // namespace + + Vector SpvcSession::ReflectTransformFeedbackCaptures() const { + Vector captures; + if (compiler == nullptr || resources == nullptr) return captures; + + const spvc_reflected_resource* outputs = nullptr; + SizeT outputCount = 0; + if (spvc_resources_get_resource_list_for_type(resources, SPVC_RESOURCE_TYPE_STAGE_OUTPUT, &outputs, + &outputCount) != SPVC_SUCCESS) { + return captures; + } + + for (SizeT i = 0; i < outputCount; ++i) { + const spvc_reflected_resource& output = outputs[i]; + // XfbBuffer/XfbStride sit on the VARIABLE; Offset sits on the variable for a + // plain output and on each MEMBER for a block (which is how a redeclared + // gl_PerVertex carries it). + const Bool hasBuffer = + spvc_compiler_has_decoration(compiler, output.id, SpvDecorationXfbBuffer) == SPVC_TRUE; + const Uint32 buffer = + hasBuffer ? spvc_compiler_get_decoration(compiler, output.id, SpvDecorationXfbBuffer) : 0u; + const Uint32 stride = + spvc_compiler_has_decoration(compiler, output.id, SpvDecorationXfbStride) == SPVC_TRUE + ? spvc_compiler_get_decoration(compiler, output.id, SpvDecorationXfbStride) + : 0u; + + const spvc_type type = spvc_compiler_get_type_handle(compiler, output.base_type_id); + const unsigned memberCount = + type != nullptr && spvc_type_get_basetype(type) == SPVC_BASETYPE_STRUCT + ? spvc_type_get_num_member_types(type) + : 0u; + + if (memberCount == 0) { + if (spvc_compiler_has_decoration(compiler, output.id, SpvDecorationOffset) != SPVC_TRUE) { + continue; + } + SpirvXfbCapture capture; + capture.name = output.name ? output.name : ""; + capture.buffer = buffer; + capture.stride = stride; + capture.offset = spvc_compiler_get_decoration(compiler, output.id, SpvDecorationOffset); + capture.componentCount = XfbComponentCount(compiler, output.type_id); + if (!capture.name.empty()) captures.push_back(Move(capture)); + continue; + } + + for (unsigned member = 0; member < memberCount; ++member) { + if (spvc_compiler_has_member_decoration(compiler, output.base_type_id, member, + SpvDecorationOffset) != SPVC_TRUE) { + continue; + } + const char* memberName = + spvc_compiler_get_member_name(compiler, output.base_type_id, member); + if (memberName == nullptr || *memberName == '\0') continue; + SpirvXfbCapture capture; + // A redeclared built-in block contributes its members by their own names + // ("gl_Position"), which is how GL's capture interface spells them; an + // application block spells them "Block.member". + const String blockName = output.name ? String(output.name) : String{}; + const Bool isBuiltInBlock = blockName.compare(0, 3, "gl_") == 0; + capture.name = isBuiltInBlock || blockName.empty() + ? String(memberName) + : blockName + "." + String(memberName); + capture.buffer = buffer; + capture.stride = stride; + capture.offset = spvc_compiler_get_member_decoration(compiler, output.base_type_id, member, + SpvDecorationOffset); + capture.componentCount = XfbComponentCount( + compiler, spvc_type_get_member_type(type, member)); + captures.push_back(Move(capture)); + } + } + + // Capture order IS buffer-then-offset order: that is the order the equivalent + // glTransformFeedbackVaryings request has to name them in for the frontend's + // packer to reproduce the declared layout. + std::stable_sort(captures.begin(), captures.end(), + [](const SpirvXfbCapture& a, const SpirvXfbCapture& b) { + if (a.buffer != b.buffer) return a.buffer < b.buffer; + return a.offset < b.offset; + }); + return captures; + } + + void SpvcSession::StripTransformFeedbackDecorations() { + if (compiler == nullptr || resources == nullptr) return; + + const spvc_reflected_resource* outputs = nullptr; + SizeT outputCount = 0; + if (spvc_resources_get_resource_list_for_type(resources, SPVC_RESOURCE_TYPE_STAGE_OUTPUT, &outputs, + &outputCount) != SPVC_SUCCESS) { + return; + } + for (SizeT i = 0; i < outputCount; ++i) { + const spvc_reflected_resource& output = outputs[i]; + spvc_compiler_unset_decoration(compiler, output.id, SpvDecorationXfbBuffer); + spvc_compiler_unset_decoration(compiler, output.id, SpvDecorationXfbStride); + spvc_compiler_unset_decoration(compiler, output.id, SpvDecorationOffset); + + const spvc_type type = spvc_compiler_get_type_handle(compiler, output.base_type_id); + if (type == nullptr || spvc_type_get_basetype(type) != SPVC_BASETYPE_STRUCT) continue; + const unsigned memberCount = spvc_type_get_num_member_types(type); + for (unsigned member = 0; member < memberCount; ++member) { + spvc_compiler_unset_member_decoration(compiler, output.base_type_id, member, + SpvDecorationOffset); + spvc_compiler_unset_member_decoration(compiler, output.base_type_id, member, + SpvDecorationXfbBuffer); + spvc_compiler_unset_member_decoration(compiler, output.base_type_id, member, + SpvDecorationXfbStride); + } + } + } + spvc_result SpvcSession::SetEntryPoint(const char* name, SpvExecutionModel model) { - if (compiler == nullptr || name == nullptr || *name == '\0') return SPVC_SUCCESS; + // A null compiler or a null/empty name is a FAILURE, not a silent success: the + // caller is asking for a specific entry point and there is none to give it. + if (compiler == nullptr || name == nullptr || *name == '\0') return SPVC_ERROR_INVALID_ARGUMENT; return spvc_compiler_set_entry_point(compiler, name, model); } diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h index 63516db0..f899e5bb 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h @@ -56,6 +56,19 @@ namespace MobileGL { } }; + // One output a SPIR-V module asked to have captured, as its Xfb decorations describe + // it. ARB_gl_spirv makes these decorations the ONLY way a SPIR-V program declares + // transform feedback - glTransformFeedbackVaryings has no effect on such a program - + // so a module that carries them and an implementation that ignores them capture + // nothing at all. + struct SpirvXfbCapture { + String name; // the GL interface name: "gl_Position", or "Block.member" + Uint32 buffer = 0; // XfbBuffer on the declaring variable + Uint32 offset = 0; // Offset on the variable or on the member + Uint32 stride = 0; // XfbStride on the declaring variable + Uint32 componentCount = 0; // how many 32-bit components the capture occupies + }; + enum class SessionUsageBit { Reflection = 1 << 0, Transpile = 1 << 1, @@ -164,6 +177,28 @@ namespace MobileGL { // Select which OpEntryPoint of `model` this session compiles. A module may carry // several of the same execution model, and glSpecializeShader names the one the // shader object stands for. + // Whether the transpile constructor actually built a compiler. Every SPIRV-Cross + // handle below is default-null and the C API leaves its out-params untouched on + // failure, so a module SPIRV-Cross cannot parse used to leave `ir` null and then + // have spvc_context_create_compiler dereference it - a raw null read that + // SPVC_BEGIN_SAFE_SCOPE cannot catch. Only glShaderBinary feeds this class bytes + // MobileGL did not generate itself, which is why the check earns its keep now. + Bool IsTranspileReady() const { return compiler != nullptr && resources != nullptr; } + // Read the module's transform-feedback layout out of its Xfb decorations, in + // (buffer, offset) order. Empty when the module declares no capture. + Vector ReflectTransformFeedbackCaptures() const; + // Remove every Xfb decoration the reflection above just read. + // + // This is not tidying: the decorations must not survive into the GLSL this session + // emits. SPIRV-Cross re-emits them as `layout(xfb_buffer = N, xfb_stride = M) out + // gl_PerVertex { layout(xfb_offset = K) ... }`, glslang re-encodes that into the + // regenerated SPIR-V, and the DirectGLES leg then transpiles THAT to ESSL - where + // the same SPIRV-Cross throws "Need GL_ARB_enhanced_layouts for xfb_stride or + // xfb_buffer" and the stage silently fails to build, leaving a program that links + // clean and draws nothing. Stripping them and re-declaring the capture through + // MobileGL's ordinary capture machinery (which both backends already implement) + // routes a SPIR-V program down exactly the path a GLSL program takes. + void StripTransformFeedbackDecorations(); spvc_result SetEntryPoint(const char* name, SpvExecutionModel model); // Bake glSpecializeShader's values into the module's specialization constants. // Every value is a GLuint on the GL side and is reinterpreted according to the diff --git a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp index 70596c5f..686bd80b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp @@ -176,11 +176,17 @@ namespace MobileGL { // memo keys. The conformance suite accepts a link-time rejection: its predicate is // compiledAndLinked(), which is the AND of the two. // - // ONE enforcement point for all five kinds, on purpose. Before this, exactly one kind - - // shader-storage blocks - was checked, by a bespoke lexical scan of the shader source, which - // is why the storage sub-family was the one that passed while sampler, image, uniform-block - // and atomic-counter bindings sailed past every ceiling. That scanner is retired; a second - // enforcement point is a second thing to drift. + // FIVE KINDS HERE, AND ONE OF THEM IS ALSO CHECKED EARLIER. Before this, exactly one kind - + // shader-storage blocks - was checked at all, by a bespoke lexical scan of the shader source, + // which is why the storage sub-family was the one that passed while sampler, image, + // uniform-block and atomic-counter bindings sailed past every ceiling. + // + // That scan is deliberately KEPT (ShaderCompileTask.cpp's MaxShaderStorageBufferBindings + // explains why: GLSL makes an over-range binding a COMPILE-time error, and the relaxed Vulkan + // parse leaves the scan as the only place MobileGL can raise one). So the storage arm has two + // enforcement points and the other four have this one. What keeps them from drifting is not + // that there is only one site but that both read the SAME numbers - ResolveResourceBindingLimits + // is the single derivation, and neither site computes a ceiling of its own. void TMglGlslIoResolver::CheckDeclaredBindingRange(const glslang::TType& type, const glslang::TString& name) { if (m_bindingLimits == nullptr || m_bindingViolation == nullptr) return; if (!m_bindingViolation->empty()) return; // first violation wins; the link is already lost @@ -229,10 +235,17 @@ namespace MobileGL { // The ARRAYED-INSTANCE rule: an array of N takes bindings base .. base + N - 1, and every // one of them has to fit. getCumulativeArraySize() folds a multi-dimensional array into // the count of leaf elements, which is exactly how many consecutive bindings GL hands out. - // An unsized or implicitly-sized array reports 0; treat it as one binding rather than - // guess, since it cannot be the shape the rule is about. + // + // isSizedArray() is MANDATORY, not defensive. glslang's TArraySizes::getCumulativeSize() + // asserts `sizes.getDimSize(d) != UnsizedArraySize` ("this only makes sense in paths that + // have a known array size"), so calling it on a run-time-sized array - the ordinary shape + // of a storage block's trailing member, and legal on the block instance itself - aborts + // the process inside mapIO's collect callback in any build with assertions live. The + // repo defines no NDEBUG of its own, so a CMake Debug build is exactly such a build; the + // "reports 0" behaviour the previous comment relied on is only what NDEBUG happens to do. + // An unsized array occupies one binding here, which is also what GL means by it. long long elementCount = 1; - if (type.isArray()) { + if (type.isArray() && type.isSizedArray()) { const int cumulative = static_cast(type.getCumulativeArraySize()); if (cumulative > 1) elementCount = cumulative; }