Support struct varying for transform feedback

include/GLSLANG/ShaderVars.h

@@ -224,7 +224,7 @@ struct Varying : public VariableWithLocation
 {
     Varying();
     ~Varying();
-    Varying(const Varying &otherg);
+    Varying(const Varying &other);
     Varying &operator=(const Varying &other);
     bool operator==(const Varying &other) const;
     bool operator!=(const Varying &other) const

src/common/utilities.cpp

@@ -7,6 +7,7 @@
 // utilities.cpp: Conversion functions and other utility routines.
 
 #include "common/utilities.h"
+#include <GLSLANG/ShaderVars.h>
 #include "common/mathutil.h"
 #include "common/platform.h"
 
@@ -770,6 +771,38 @@ std::string ParseResourceName(const std::string &name, std::vector<unsigned int>
     return name.substr(0, baseNameLength);
 }
 
+const sh::ShaderVariable *FindShaderVarField(const sh::ShaderVariable &var,
+                                             const std::string &fullName)
+{
+    if (var.fields.empty())
+    {
+        return nullptr;
+    }
+    size_t pos = fullName.find_first_of(".");
+    if (pos == std::string::npos)
+    {
+        return nullptr;
+    }
+    std::string topName = fullName.substr(0, pos);
+    if (topName != var.name)
+    {
+        return nullptr;
+    }
+    std::string fieldName = fullName.substr(pos + 1);
+    if (fieldName.empty())
+    {
+        return nullptr;
+    }
+    for (const auto &field : var.fields)
+    {
+        if (field.name == fieldName)
+        {
+            return &field;
+        }
+    }
+    return nullptr;
+}
+
 unsigned int ArraySizeProduct(const std::vector<unsigned int> &arraySizes)
 {
     unsigned int arraySizeProduct = 1u;

src/common/utilities.h

@@ -19,6 +19,11 @@
 
 #include "common/mathutil.h"
 
+namespace sh
+{
+struct ShaderVariable;
+}
+
 namespace gl
 {
 
@@ -57,6 +62,11 @@ GLenum LayerIndexToCubeMapTextureTarget(size_t index);
 // outSubscripts.
 std::string ParseResourceName(const std::string &name, std::vector<unsigned int> *outSubscripts);
 
+// Find the child field which matches 'fullName' == var.name + "." + field.name.
+// Return nullptr if not found.
+const sh::ShaderVariable *FindShaderVarField(const sh::ShaderVariable &var,
+                                             const std::string &fullName);
+
 // Find the range of index values in the provided indices pointer.  Primitive restart indices are
 // only counted in the range if primitive restart is disabled.
 IndexRange ComputeIndexRange(GLenum indexType,

src/libANGLE/Program.cpp

@@ -320,6 +320,28 @@ void InitShaderStorageBlockLinker(const gl::Context *context,
     }
 }
 
+// Find the matching varying or field by name.
+const sh::ShaderVariable *FindVaryingOrField(const ProgramMergedVaryings &varyings,
+                                             const std::string &name)
+{
+    const sh::ShaderVariable *var = nullptr;
+    for (const auto &ref : varyings)
+    {
+        const sh::Varying *varying = ref.second.get();
+        if (varying->name == name)
+        {
+            var = varying;
+            break;
+        }
+        var = FindShaderVarField(*varying, name);
+        if (var != nullptr)
+        {
+            break;
+        }
+    }
+    return var;
+}
+
 }  // anonymous namespace
 
 const char *const g_fakepath = "C:\\fakepath";
@@ -2642,91 +2664,113 @@ bool Program::linkValidateTransformFeedback(const gl::Context *context,
                                             const ProgramMergedVaryings &varyings,
                                             const Caps &caps) const
 {
-    size_t totalComponents = 0;
 
+    // Validate the tf names regardless of the actual program varyings.
     std::set<std::string> uniqueNames;
+    for (const std::string &tfVaryingName : mState.mTransformFeedbackVaryingNames)
+    {
+        if (context->getClientVersion() < Version(3, 1) &&
+            tfVaryingName.find('[') != std::string::npos)
+        {
+            infoLog << "Capture of array elements is undefined and not supported.";
+            return false;
+        }
+        if (context->getClientVersion() >= Version(3, 1))
+        {
+            if (IncludeSameArrayElement(uniqueNames, tfVaryingName))
+            {
+                infoLog << "Two transform feedback varyings include the same array element ("
+                        << tfVaryingName << ").";
+                return false;
+            }
+        }
+        else
+        {
+            if (uniqueNames.count(tfVaryingName) > 0)
+            {
+                infoLog << "Two transform feedback varyings specify the same output variable ("
+                        << tfVaryingName << ").";
+                return false;
+            }
+        }
+        uniqueNames.insert(tfVaryingName);
+    }
 
+    // Validate against program varyings.
+    size_t totalComponents = 0;
     for (const std::string &tfVaryingName : mState.mTransformFeedbackVaryingNames)
     {
-        bool found = false;
         std::vector<unsigned int> subscripts;
         std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
 
-        for (const auto &ref : varyings)
+        const sh::ShaderVariable *var = FindVaryingOrField(varyings, baseName);
+        if (var == nullptr)
         {
-            const sh::Varying *varying = ref.second.get();
+            infoLog << "Transform feedback varying " << tfVaryingName
+                    << " does not exist in the vertex shader.";
+            return false;
+        }
 
-            if (baseName == varying->name)
-            {
-                if (uniqueNames.count(tfVaryingName) > 0)
-                {
-                    infoLog << "Two transform feedback varyings specify the same output variable ("
-                            << tfVaryingName << ").";
-                    return false;
-                }
-                if (context->getClientVersion() >= Version(3, 1))
-                {
-                    if (IncludeSameArrayElement(uniqueNames, tfVaryingName))
-                    {
-                        infoLog
-                            << "Two transform feedback varyings include the same array element ("
-                            << tfVaryingName << ").";
-                        return false;
-                    }
-                }
-                else if (varying->isArray())
-                {
-                    infoLog << "Capture of arrays is undefined and not supported.";
-                    return false;
-                }
+        // Validate the matching variable.
+        if (var->isStruct())
+        {
+            infoLog << "Struct cannot be captured directly (" << baseName << ").";
+            return false;
+        }
 
-                uniqueNames.insert(tfVaryingName);
+        size_t elementCount   = 0;
+        size_t componentCount = 0;
 
-                // TODO(jmadill): Investigate implementation limits on D3D11
+        if (var->isArray())
+        {
+            if (context->getClientVersion() < Version(3, 1))
+            {
+                infoLog << "Capture of arrays is undefined and not supported.";
+                return false;
+            }
 
-                // GLSL ES 3.10 section 4.3.6: A vertex output can't be an array of arrays.
-                ASSERT(!varying->isArrayOfArrays());
-                size_t elementCount =
-                    ((varying->isArray() && subscripts.empty()) ? varying->getOutermostArraySize()
-                                                                : 1);
-                size_t componentCount = VariableComponentCount(varying->type) * elementCount;
-                if (mState.mTransformFeedbackBufferMode == GL_SEPARATE_ATTRIBS &&
-                    componentCount > caps.maxTransformFeedbackSeparateComponents)
-                {
-                    infoLog << "Transform feedback varying's " << varying->name << " components ("
-                            << componentCount << ") exceed the maximum separate components ("
-                            << caps.maxTransformFeedbackSeparateComponents << ").";
-                    return false;
-                }
+            // GLSL ES 3.10 section 4.3.6: A vertex output can't be an array of arrays.
+            ASSERT(!var->isArrayOfArrays());
 
-                totalComponents += componentCount;
-                found = true;
-                break;
+            if (!subscripts.empty() && subscripts[0] >= var->getOutermostArraySize())
+            {
+                infoLog << "Cannot capture outbound array element '" << tfVaryingName << "'.";
+                return false;
             }
+            elementCount = (subscripts.empty() ? var->getOutermostArraySize() : 1);
         }
-        if (context->getClientVersion() < Version(3, 1) &&
-            tfVaryingName.find('[') != std::string::npos)
+        else
         {
-            infoLog << "Capture of array elements is undefined and not supported.";
-            return false;
+            if (!subscripts.empty())
+            {
+                infoLog << "Varying '" << baseName
+                        << "' is not an array to be captured by element.";
+                return false;
+            }
+            elementCount = 1;
         }
-        if (!found)
+
+        // TODO(jmadill): Investigate implementation limits on D3D11
+        componentCount = VariableComponentCount(var->type) * elementCount;
+        if (mState.mTransformFeedbackBufferMode == GL_SEPARATE_ATTRIBS &&
+            componentCount > caps.maxTransformFeedbackSeparateComponents)
         {
-            infoLog << "Transform feedback varying " << tfVaryingName
-                    << " does not exist in the vertex shader.";
+            infoLog << "Transform feedback varying " << tfVaryingName << " components ("
+                    << componentCount << ") exceed the maximum separate components ("
+                    << caps.maxTransformFeedbackSeparateComponents << ").";
             return false;
         }
-    }
 
-    if (mState.mTransformFeedbackBufferMode == GL_INTERLEAVED_ATTRIBS &&
-        totalComponents > caps.maxTransformFeedbackInterleavedComponents)
-    {
-        infoLog << "Transform feedback varying total components (" << totalComponents
-                << ") exceed the maximum interleaved components ("
-                << caps.maxTransformFeedbackInterleavedComponents << ").";
-        return false;
+        totalComponents += componentCount;
+        if (mState.mTransformFeedbackBufferMode == GL_INTERLEAVED_ATTRIBS &&
+            totalComponents > caps.maxTransformFeedbackInterleavedComponents)
+        {
+            infoLog << "Transform feedback varying total components (" << totalComponents
+                    << ") exceed the maximum interleaved components ("
+                    << caps.maxTransformFeedbackInterleavedComponents << ").";
+            return false;
+        }
     }
-
     return true;
 }
 
@@ -2782,6 +2826,15 @@ void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyi
                     *varying, static_cast<GLuint>(subscript));
                 break;
             }
+            else if (varying->isStruct())
+            {
+                const auto *field = FindShaderVarField(*varying, tfVaryingName);
+                if (field != nullptr)
+                {
+                    mState.mLinkedTransformFeedbackVaryings.emplace_back(*field, *varying);
+                    break;
+                }
+            }
         }
     }
 }

src/libANGLE/Program.h

@@ -201,6 +201,17 @@ struct TransformFeedbackVarying : public sh::Varying
     {
         ASSERT(!isArrayOfArrays());
     }
+
+    TransformFeedbackVarying(const sh::ShaderVariable &field, const sh::Varying &parent)
+        : arrayIndex(GL_INVALID_INDEX)
+    {
+        sh::ShaderVariable *thisVar = this;
+        *thisVar                    = field;
+        interpolation               = parent.interpolation;
+        isInvariant                 = parent.isInvariant;
+        name                        = parent.name + "." + name;
+    }
+
     std::string nameWithArrayIndex() const
     {
         std::stringstream fullNameStr;

src/libANGLE/Shader.cpp

@@ -534,6 +534,12 @@ std::string Shader::getTransformFeedbackVaryingMappedName(const std::string &tfV
             {
                 return varying.mappedName;
             }
+            else if (varying.isStruct())
+            {
+                const auto *field = FindShaderVarField(varying, tfVaryingName);
+                ASSERT(field != nullptr && !field->isStruct() && !field->isArray());
+                return varying.mappedName + "." + field->mappedName;
+            }
         }
     }
     UNREACHABLE();

src/libANGLE/VaryingPacking.cpp

@@ -300,13 +300,13 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
                 {
                     ASSERT(!field.isStruct() && !field.isArray());
                     mPackedVaryings.push_back(PackedVarying(field, interpolation, output->name));
-                    uniqueFullNames.insert(mPackedVaryings.back().nameWithArrayIndex());
+                    uniqueFullNames.insert(mPackedVaryings.back().fullName());
                 }
             }
             else
             {
                 mPackedVaryings.push_back(PackedVarying(*output, interpolation));
-                uniqueFullNames.insert(mPackedVaryings.back().nameWithArrayIndex());
+                uniqueFullNames.insert(mPackedVaryings.back().fullName());
             }
             continue;
         }
@@ -331,16 +331,26 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
             {
                 continue;
             }
+            if (input->isStruct())
+            {
+                const sh::ShaderVariable *field = FindShaderVarField(*input, tfVarying);
+                if (field != nullptr)
+                {
+                    ASSERT(!field->isStruct() && !field->isArray());
+                    mPackedVaryings.emplace_back(*field, input->interpolation, input->name);
+                    mPackedVaryings.back().vertexOnly = true;
+                    mPackedVaryings.back().arrayIndex = GL_INVALID_INDEX;
+                    uniqueFullNames.insert(tfVarying);
+                }
+            }
             // Array as a whole and array element conflict has already been checked in
             // linkValidateTransformFeedback.
-            if (baseName == input->name)
+            else if (baseName == input->name)
             {
-                // Transform feedback for varying structs is underspecified.
-                // See Khronos bug 9856.
-                // TODO(jmadill): Figure out how to be spec-compliant here.
-                if (!input->isStruct() && tfVarying.compare(0, 3, "gl_") != 0)
+                // only pack varyings that are not builtins.
+                if (tfVarying.compare(0, 3, "gl_") != 0)
                 {
-                    mPackedVaryings.push_back(PackedVarying(*input, input->interpolation));
+                    mPackedVaryings.emplace_back(*input, input->interpolation);
                     mPackedVaryings.back().vertexOnly = true;
                     mPackedVaryings.back().arrayIndex = static_cast<GLuint>(subscript);
                     uniqueFullNames.insert(tfVarying);
@@ -372,7 +382,7 @@ bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
     {
         if (!packVarying(packedVarying))
         {
-            infoLog << "Could not pack varying " << packedVarying.nameWithArrayIndex();
+            infoLog << "Could not pack varying " << packedVarying.fullName();
             return false;
         }
     }

src/libANGLE/VaryingPacking.h

@@ -47,9 +47,14 @@ struct PackedVarying
 
     bool isArrayElement() const { return arrayIndex != GL_INVALID_INDEX; }
 
-    std::string nameWithArrayIndex() const
+    std::string fullName() const
     {
         std::stringstream fullNameStr;
+        if (isStructField())
+        {
+            fullNameStr << parentStructName << ".";
+        }
+
         fullNameStr << varying->name;
         if (arrayIndex != GL_INVALID_INDEX)
         {
@@ -97,7 +102,17 @@ struct PackedVaryingRegister final
         return registerRow * 4 + registerColumn;
     }
 
-    bool isStructField() const { return !structFieldName.empty(); }
+    std::string tfVaryingName() const
+    {
+        if (packedVarying->isArrayElement() || packedVarying->isStructField())
+        {
+            return packedVarying->fullName();
+        }
+        else
+        {
+            return packedVarying->varying->name;
+        }
+    }
 
     // Index to the array of varyings.
     const PackedVarying *packedVarying;
@@ -116,9 +131,6 @@ struct PackedVaryingRegister final
 
     // Assigned after packing
     unsigned int semanticIndex;
-
-    // Struct member this varying corresponds to.
-    std::string structFieldName;
 };
 
 // Supported packing modes:

src/libANGLE/renderer/d3d/ProgramD3D.cpp

@@ -2678,30 +2678,16 @@ void ProgramD3D::gatherTransformFeedbackVaryings(const gl::VaryingPacking &varyi
         }
         else
         {
-            std::vector<unsigned int> subscripts;
-            std::string baseName = gl::ParseResourceName(tfVaryingName, &subscripts);
-            size_t subscript     = GL_INVALID_INDEX;
-            if (!subscripts.empty())
-            {
-                subscript = subscripts.back();
-            }
             for (const auto &registerInfo : varyingPacking.getRegisterList())
             {
                 const auto &varying   = *registerInfo.packedVarying->varying;
                 GLenum transposedType = gl::TransposeMatrixType(varying.type);
-                int componentCount = gl::VariableColumnCount(transposedType);
-                ASSERT(!varying.isBuiltIn());
-
-                // Transform feedback for varying structs is underspecified.
-                // See Khronos bug 9856.
-                // TODO(jmadill): Figure out how to be spec-compliant here.
-                if (registerInfo.packedVarying->isStructField() || varying.isStruct())
-                    continue;
+                int componentCount    = gl::VariableColumnCount(transposedType);
+                ASSERT(!varying.isBuiltIn() && !varying.isStruct());
 
                 // There can be more than one register assigned to a particular varying, and each
                 // register needs its own stream out entry.
-                if (baseName == registerInfo.packedVarying->varying->name &&
-                    (subscript == GL_INVALID_INDEX || subscript == registerInfo.varyingArrayIndex))
+                if (registerInfo.tfVaryingName() == tfVaryingName)
                 {
                     mStreamOutVaryings.push_back(D3DVarying(
                         varyingSemantic, registerInfo.semanticIndex, componentCount, outputSlot));

src/tests/gl_tests/TransformFeedbackTest.cpp

@@ -1129,6 +1129,66 @@ TEST_P(TransformFeedbackTestES31, SameArrayElementVaryings)
     ASSERT_EQ(0u, mProgram);
 }
 
+// Test that program link fails in case to capture array element on a non-array varying.
+TEST_P(TransformFeedbackTestES31, ElementCaptureOnNonArrayVarying)
+{
+    const std::string &vertexShaderSource =
+        "#version 310 es\n"
+        "in vec3 position;\n"
+        "out vec3 outAttrib;\n"
+        "void main() {"
+        "  outAttrib = position;\n"
+        "  gl_Position = vec4(position, 1);\n"
+        "}";
+
+    const std::string &fragmentShaderSource =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "out vec4 color;\n"
+        "in vec3 outAttrib;\n"
+        "void main() {\n"
+        "  color = vec4(0);\n"
+        "}";
+
+    std::vector<std::string> tfVaryings;
+    tfVaryings.push_back("outAttrib[1]");
+
+    mProgram = CompileProgramWithTransformFeedback(vertexShaderSource, fragmentShaderSource,
+                                                   tfVaryings, GL_INTERLEAVED_ATTRIBS);
+    ASSERT_EQ(0u, mProgram);
+}
+
+// Test that program link fails in case to capure an outbound array element.
+TEST_P(TransformFeedbackTestES31, CaptureOutboundElement)
+{
+    const std::string &vertexShaderSource =
+        "#version 310 es\n"
+        "in vec3 position;\n"
+        "out vec3 outAttribs[3];\n"
+        "void main() {"
+        "  outAttribs[0] = position;\n"
+        "  outAttribs[1] = vec3(0, 0, 0);\n"
+        "  outAttribs[2] = position;\n"
+        "  gl_Position = vec4(position, 1);\n"
+        "}";
+
+    const std::string &fragmentShaderSource =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "out vec4 color;\n"
+        "in vec3 outAttribs[3];\n"
+        "void main() {\n"
+        "  color = vec4(0);\n"
+        "}";
+
+    std::vector<std::string> tfVaryings;
+    tfVaryings.push_back("outAttribs[3]");
+
+    mProgram = CompileProgramWithTransformFeedback(vertexShaderSource, fragmentShaderSource,
+                                                   tfVaryings, GL_INTERLEAVED_ATTRIBS);
+    ASSERT_EQ(0u, mProgram);
+}
+
 // Test transform feedback names can be specified using array element.
 TEST_P(TransformFeedbackTestES31, DifferentArrayElementVaryings)
 {
@@ -1192,6 +1252,125 @@ TEST_P(TransformFeedbackTestES31, DifferentArrayElementVaryings)
     ASSERT_GL_NO_ERROR();
 }
 
+// Test transform feedback varying for base-level members of struct.
+TEST_P(TransformFeedbackTestES31, StructMemberVaryings)
+{
+    const std::string &vertexShaderSource =
+        R"(#version 310 es
+
+        in vec3 position;
+        struct S {
+          vec3 field0;
+          vec3 field1;
+          vec3 field2;
+        };
+        out S s;
+
+        void main() {
+          s.field0 = position;
+          s.field1 = vec3(0, 0, 0);
+          s.field2 = position;
+          gl_Position = vec4(position, 1);
+        })";
+
+    const std::string &fragmentShaderSource =
+        R"(#version 310 es
+
+        precision mediump float;
+        struct S {
+          vec3 field0;
+          vec3 field1;
+          vec3 field2;
+        };
+        out vec4 color;
+        in S s;
+
+        void main() {
+          color = vec4(s.field1, 1);
+        })";
+
+    std::vector<std::string> tfVaryings;
+    tfVaryings.push_back("s.field0");
+    tfVaryings.push_back("s.field2");
+
+    mProgram = CompileProgramWithTransformFeedback(vertexShaderSource, fragmentShaderSource,
+                                                   tfVaryings, GL_INTERLEAVED_ATTRIBS);
+    ASSERT_NE(0u, mProgram);
+
+    glBindBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, mTransformFeedbackBuffer);
+    glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, sizeof(Vector3) * 2 * 6, nullptr, GL_STREAM_DRAW);
+
+    glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, mTransformFeedback);
+    glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, mTransformFeedbackBuffer);
+
+    glUseProgram(mProgram);
+    glBeginTransformFeedback(GL_TRIANGLES);
+    drawQuad(mProgram, "position", 0.5f);
+    glEndTransformFeedback();
+    glUseProgram(0);
+    ASSERT_GL_NO_ERROR();
+
+    const GLvoid *mapPointer =
+        glMapBufferRange(GL_TRANSFORM_FEEDBACK_BUFFER, 0, sizeof(Vector3) * 2 * 6, GL_MAP_READ_BIT);
+    ASSERT_NE(nullptr, mapPointer);
+
+    const auto &quadVertices = GetQuadVertices();
+
+    const Vector3 *vecPointer = static_cast<const Vector3 *>(mapPointer);
+    for (unsigned int vectorIndex = 0; vectorIndex < 3; ++vectorIndex)
+    {
+        unsigned int stream1Index = vectorIndex * 2;
+        unsigned int stream2Index = vectorIndex * 2 + 1;
+        EXPECT_EQ(quadVertices[vectorIndex], vecPointer[stream1Index]);
+        EXPECT_EQ(quadVertices[vectorIndex], vecPointer[stream2Index]);
+    }
+    glUnmapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER);
+
+    ASSERT_GL_NO_ERROR();
+}
+
+// Test transform feedback varying for struct is not allowed.
+TEST_P(TransformFeedbackTestES31, InvalidStructVaryings)
+{
+    const std::string &vertexShaderSource =
+        R"(#version 310 es
+
+        in vec3 position;
+        struct S {
+          vec3 field0;
+          vec3 field1;
+        };
+        out S s;
+
+        void main() {
+          s.field0 = position;
+          s.field1 = vec3(0, 0, 0);
+          gl_Position = vec4(position, 1);
+        })";
+
+    const std::string &fragmentShaderSource =
+        R"(#version 310 es
+
+        precision mediump float;
+        struct S {
+          vec3 field0;
+          vec3 field1;
+        };
+        out vec4 color;
+        in S s;
+
+        void main() {
+          color = vec4(s.field1, 1);
+        })";
+
+    std::vector<std::string> tfVaryings;
+    tfVaryings.push_back("s");
+
+    mProgram = CompileProgramWithTransformFeedback(vertexShaderSource, fragmentShaderSource,
+                                                   tfVaryings, GL_INTERLEAVED_ATTRIBS);
+    ASSERT_EQ(0u, mProgram);
+}
+
 // Test that nonexistent transform feedback varyings don't assert when linking.
 TEST_P(TransformFeedbackTest, NonExistentTransformFeedbackVarying)
 {