From: Nicolas Capens Date: Wed, 8 Aug 2018 03:57:21 +0000 (-0400) Subject: Fix attribute layout location linking. X-Git-Tag: android-x86-9.0-r1~667 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=378c434627aa62e5f567b05d389c68b84a6af4fb;p=android-x86%2Fexternal-swiftshader.git Fix attribute layout location linking. When a vertex attribute has a GLSL layout location qualifier, it takes precedence over the binding location provided through the glBindAttribLocation API call. OpenGL ES 3.0.5 spec: "If an active attribute has a binding explicitly set within the shader text and a different binding assigned by BindAttribLocation, the assignment in the shader text is used." Change-Id: If0bc0dc01a8ff6189703f2be26f1938fbff5f5ae Reviewed-on: https://swiftshader-review.googlesource.com/20168 Tested-by: Nicolas Capens Reviewed-by: Alexis Hétu --- diff --git a/src/OpenGL/compiler/OutputASM.cpp b/src/OpenGL/compiler/OutputASM.cpp index 9ed34b2f9..c18ecf735 100644 --- a/src/OpenGL/compiler/OutputASM.cpp +++ b/src/OpenGL/compiler/OutputASM.cpp @@ -400,12 +400,12 @@ namespace glsl registerIndex = 0; } - Attribute::Attribute(GLenum type, const std::string &name, int arraySize, int location, int registerIndex) + Attribute::Attribute(GLenum type, const std::string &name, int arraySize, int layoutLocation, int registerIndex) { this->type = type; this->name = name; this->arraySize = arraySize; - this->location = location; + this->layoutLocation = layoutLocation; this->registerIndex = registerIndex; } diff --git a/src/OpenGL/compiler/OutputASM.h b/src/OpenGL/compiler/OutputASM.h index 2f90266a8..b037a671f 100644 --- a/src/OpenGL/compiler/OutputASM.h +++ b/src/OpenGL/compiler/OutputASM.h @@ -144,12 +144,12 @@ namespace glsl struct Attribute { Attribute(); - Attribute(GLenum type, const std::string &name, int arraySize, int location, int registerIndex); + Attribute(GLenum type, const std::string &name, int arraySize, int layoutLocation, int registerIndex); GLenum type; std::string name; int arraySize; - int location; + int layoutLocation; int registerIndex; }; diff --git a/src/OpenGL/libGLESv2/Program.cpp b/src/OpenGL/libGLESv2/Program.cpp index 77912cc46..dc64813a2 100644 --- a/src/OpenGL/libGLESv2/Program.cpp +++ b/src/OpenGL/libGLESv2/Program.cpp @@ -277,19 +277,7 @@ namespace es2 GLint Program::getAttributeLocation(const char *name) { - if(name) - { - std::string strName(name); - for(auto const &it : linkedAttribute) - { - if(it.name == strName) - { - return getAttributeBinding(it); - } - } - } - - return -1; + return name ? getAttributeLocation(std::string(name)) : -1; } int Program::getAttributeStream(int attributeIndex) @@ -1591,80 +1579,55 @@ namespace es2 // Determines the mapping between GL attributes and vertex stream usage indices bool Program::linkAttributes() { + static_assert(MAX_VERTEX_ATTRIBS <= 32, "attribute count exceeds bitfield count"); unsigned int usedLocations = 0; - // Link attributes that have a binding location + // Link attributes that have a GLSL layout location qualifier for(auto const &attribute : vertexShader->activeAttributes) { - int location = (attributeBinding.find(attribute.name) != attributeBinding.end()) ? attributeBinding[attribute.name] : -1; - - if(location != -1) // Set by glBindAttribLocation + if(attribute.layoutLocation != -1) { - int rows = VariableRegisterCount(attribute.type); - - if(rows + location > MAX_VERTEX_ATTRIBS) + if(!linkAttribute(attribute, attribute.layoutLocation, usedLocations)) { - appendToInfoLog("Active attribute (%s) at location %d is too big to fit", attribute.name.c_str(), location); return false; } + } + } - // In GLSL 3.00, attribute aliasing produces a link error - // In GLSL 1.00, attribute aliasing is allowed - if(vertexShader->getShaderVersion() >= 300) - { - for(auto const &it : linkedAttribute) - { - int itLocStart = getAttributeBinding(it); - ASSERT(itLocStart >= 0); - int itLocEnd = itLocStart + VariableRegisterCount(it.type); - for(int i = 0; i < rows; i++) - { - int loc = location + i; - if((loc >= itLocStart) && (loc < itLocEnd)) - { - appendToInfoLog("Attribute '%s' aliases attribute '%s' at location %d", attribute.name.c_str(), it.name.c_str(), location); - return false; - } - } - } - } + // Link attributes that have an API provided binding location but no GLSL layout location + for(auto const &attribute : vertexShader->activeAttributes) + { + int bindingLocation = (attributeBinding.find(attribute.name) != attributeBinding.end()) ? attributeBinding[attribute.name] : -1; - linkedAttributeLocation[attribute.name] = location; - linkedAttribute.push_back(attribute); - for(int i = 0; i < rows; i++) + if(attribute.layoutLocation == -1 && bindingLocation != -1) + { + if(!linkAttribute(attribute, bindingLocation, usedLocations)) { - usedLocations |= 1 << (location + i); + return false; } } } - // Link attributes that don't have a binding location + // Link attributes that don't have a binding location nor a layout location for(auto const &attribute : vertexShader->activeAttributes) { - int location = (attributeBinding.find(attribute.name) != attributeBinding.end()) ? attributeBinding[attribute.name] : -1; - - if(location == -1) // Not set by glBindAttribLocation + if(attribute.layoutLocation == -1 && attributeBinding.find(attribute.name) == attributeBinding.end()) { - int rows = VariableRegisterCount(attribute.type); - int availableIndex = AllocateFirstFreeBits(&usedLocations, rows, MAX_VERTEX_ATTRIBS); - - if(availableIndex == -1 || availableIndex + rows > MAX_VERTEX_ATTRIBS) + if(!linkAttribute(attribute, -1, usedLocations)) { - appendToInfoLog("Too many active attributes (%s)", attribute.name.c_str()); - return false; // Fail to link + return false; } - - linkedAttributeLocation[attribute.name] = availableIndex; - linkedAttribute.push_back(attribute); } } - for(auto const &it : linkedAttribute) + ASSERT(linkedAttribute.size() == vertexShader->activeAttributes.size()); + + for(auto const &attribute : linkedAttribute) { - int location = getAttributeBinding(it); + int location = getAttributeLocation(attribute.name); ASSERT(location >= 0); - int index = vertexShader->getSemanticIndex(it.name); - int rows = std::max(VariableRegisterCount(it.type), 1); + int index = vertexShader->getSemanticIndex(attribute.name); + int rows = VariableRegisterCount(attribute.type); for(int r = 0; r < rows; r++) { @@ -1675,17 +1638,69 @@ namespace es2 return true; } - int Program::getAttributeBinding(const glsl::Attribute &attribute) + bool Program::linkAttribute(const glsl::Attribute &attribute, int location, unsigned int &usedLocations) { - if(attribute.location != -1) + int rows = VariableRegisterCount(attribute.type); + + if(location == -1) { - return attribute.location; + location = AllocateFirstFreeBits(&usedLocations, rows, MAX_VERTEX_ATTRIBS); + + if(location == -1 || location + rows > MAX_VERTEX_ATTRIBS) + { + appendToInfoLog("Too many active attributes (%s)", attribute.name.c_str()); + return false; // Fail to link + } + } + else + { + if(rows + location > MAX_VERTEX_ATTRIBS) + { + appendToInfoLog("Active attribute (%s) at location %d is too big to fit", attribute.name.c_str(), location); + return false; + } + + // In GLSL 3.00, attribute aliasing produces a link error + // In GLSL 1.00, attribute aliasing is allowed + if(vertexShader->getShaderVersion() >= 300) + { + for(auto const &previousAttrib : linkedAttribute) + { + int previousLocation = getAttributeLocation(previousAttrib.name); + int previousRows = VariableRegisterCount(previousAttrib.type); + + if(location >= previousLocation && location < previousLocation + previousRows) + { + appendToInfoLog("Attribute '%s' aliases attribute '%s' at location %d", attribute.name.c_str(), previousAttrib.name.c_str(), location); + return false; + } + + if(location <= previousLocation && location + rows > previousLocation) + { + appendToInfoLog("Attribute '%s' aliases attribute '%s' at location %d", attribute.name.c_str(), previousAttrib.name.c_str(), previousLocation); + return false; + } + } + } + + for(int i = 0; i < rows; i++) + { + usedLocations |= 1 << (location + i); + } } - std::map::const_iterator it = linkedAttributeLocation.find(attribute.name); - if(it != linkedAttributeLocation.end()) + linkedAttributeLocation[attribute.name] = location; + linkedAttribute.push_back(attribute); + + return true; + } + + int Program::getAttributeLocation(const std::string &name) + { + std::map::const_iterator attribute = linkedAttributeLocation.find(name); + if(attribute != linkedAttributeLocation.end()) { - return it->second; + return attribute->second; } return -1; diff --git a/src/OpenGL/libGLESv2/Program.h b/src/OpenGL/libGLESv2/Program.h index 501d081fe..a23292610 100644 --- a/src/OpenGL/libGLESv2/Program.h +++ b/src/OpenGL/libGLESv2/Program.h @@ -228,7 +228,8 @@ namespace es2 bool linkTransformFeedback(); bool linkAttributes(); - int getAttributeBinding(const glsl::Attribute &attribute); + bool linkAttribute(const glsl::Attribute &attribute, int location, unsigned int &usedLocations); + int getAttributeLocation(const std::string &name); Uniform *getUniform(const std::string &name) const; bool linkUniforms(const Shader *shader); diff --git a/src/OpenGL/libGLESv2/libGLESv2.cpp b/src/OpenGL/libGLESv2/libGLESv2.cpp index 789b0c82f..b3f1d201a 100644 --- a/src/OpenGL/libGLESv2/libGLESv2.cpp +++ b/src/OpenGL/libGLESv2/libGLESv2.cpp @@ -2345,7 +2345,6 @@ int GetAttribLocation(GLuint program, const GLchar* name) if(context) { - es2::Program *programObject = context->getProgram(program); if(!programObject) diff --git a/tests/unittests/unittests.cpp b/tests/unittests/unittests.cpp index 80bc8872d..91b34cd83 100644 --- a/tests/unittests/unittests.cpp +++ b/tests/unittests/unittests.cpp @@ -266,6 +266,8 @@ protected: glGetProgramiv(ph.program, GL_LINK_STATUS, &linkStatus); EXPECT_NE(linkStatus, 0); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + return ph; } @@ -274,6 +276,8 @@ protected: glDeleteShader(ph.fragmentShader); glDeleteShader(ph.vertexShader); glDeleteProgram(ph.program); + + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); } void drawQuad(GLuint program, const char* textureName = nullptr) @@ -411,8 +415,6 @@ TEST_F(SwiftShaderTest, DynamicLoop) { Initialize(3, false); - unsigned char green[4] = { 0, 255, 0, 255 }; - const std::string vs = "#version 300 es\n" "in vec4 position;\n" @@ -464,6 +466,7 @@ TEST_F(SwiftShaderTest, DynamicLoop) deleteProgram(ph); + unsigned char green[4] = { 0, 255, 0, 255 }; expectFramebufferColor(green); EXPECT_GLENUM_EQ(GL_NONE, glGetError()); @@ -476,8 +479,6 @@ TEST_F(SwiftShaderTest, DynamicIndexing) { Initialize(3, false); - unsigned char green[4] = { 0, 255, 0, 255 }; - const std::string vs = "#version 300 es\n" "in vec4 position;\n" @@ -521,6 +522,160 @@ TEST_F(SwiftShaderTest, DynamicIndexing) deleteProgram(ph); + unsigned char green[4] = { 0, 255, 0, 255 }; + expectFramebufferColor(green); + + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + Uninitialize(); +} + +// Test vertex attribute location linking +TEST_F(SwiftShaderTest, AttributeLocation) +{ + Initialize(3, false); + + const std::string vs = + "#version 300 es\n" + "layout(location = 0) in vec4 a0;\n" // Explicitly bound in GLSL + "layout(location = 2) in vec4 a2;\n" // Explicitly bound in GLSL + "in vec4 a5;\n" // Bound to location 5 by API + "in mat2 a3;\n" // Implicit location + "in vec4 a1;\n" // Implicit location + "in vec4 a6;\n" // Implicit location + "out vec4 color;\n" + "void main()\n" + "{\n" + " vec4 a34 = vec4(a3[0], a3[1]);\n" + " gl_Position = a0;\n" + " color = (a2 == vec4(1.0, 2.0, 3.0, 4.0) &&\n" + " a34 == vec4(5.0, 6.0, 7.0, 8.0) &&\n" + " a5 == vec4(9.0, 10.0, 11.0, 12.0) &&\n" + " a1 == vec4(13.0, 14.0, 15.0, 16.0) &&\n" + " a6 == vec4(17.0, 18.0, 19.0, 20.0)) ?\n" + " vec4(0.0, 1.0, 0.0, 1.0) :\n" + " vec4(1.0, 0.0, 0.0, 1.0);" + "}\n"; + + const std::string fs = + "#version 300 es\n" + "precision mediump float;\n" + "in vec4 color;\n" + "out vec4 fragColor;\n" + "void main()\n" + "{\n" + " fragColor = color;\n" + "}\n"; + + ProgramHandles ph; + ph.program = glCreateProgram(); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + ph.vertexShader = glCreateShader(GL_VERTEX_SHADER); + const char* vsSource[1] = { vs.c_str() }; + glShaderSource(ph.vertexShader, 1, vsSource, nullptr); + glCompileShader(ph.vertexShader); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + GLint vsCompileStatus = 0; + glGetShaderiv(ph.vertexShader, GL_COMPILE_STATUS, &vsCompileStatus); + EXPECT_EQ(vsCompileStatus, GL_TRUE); + + ph.fragmentShader = glCreateShader(GL_FRAGMENT_SHADER); + const char* fsSource[1] = { fs.c_str() }; + glShaderSource(ph.fragmentShader, 1, fsSource, nullptr); + glCompileShader(ph.fragmentShader); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + GLint fsCompileStatus = 0; + glGetShaderiv(ph.fragmentShader, GL_COMPILE_STATUS, &fsCompileStatus); + EXPECT_EQ(fsCompileStatus, GL_TRUE); + + // Not assigned a layout location in GLSL. Bind it explicitly with the API. + glBindAttribLocation(ph.program, 5, "a5"); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + // Should not override GLSL layout location qualifier + glBindAttribLocation(ph.program, 8, "a2"); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + glAttachShader(ph.program, ph.vertexShader); + glAttachShader(ph.program, ph.fragmentShader); + glLinkProgram(ph.program); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + // Changes after linking should have no effect + glBindAttribLocation(ph.program, 0, "a1"); + glBindAttribLocation(ph.program, 6, "a2"); + glBindAttribLocation(ph.program, 2, "a6"); + + GLint linkStatus = 0; + glGetProgramiv(ph.program, GL_LINK_STATUS, &linkStatus); + EXPECT_NE(linkStatus, 0); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + float vertices[18] = { -1.0f, 1.0f, 0.5f, + -1.0f, -1.0f, 0.5f, + 1.0f, -1.0f, 0.5f, + -1.0f, 1.0f, 0.5f, + 1.0f, -1.0f, 0.5f, + 1.0f, 1.0f, 0.5f }; + + float attributes[5][4] = { 1.0f, 2.0f, 3.0f, 4.0f, + 5.0f, 6.0f, 7.0f, 8.0f, + 9.0f, 10.0f, 11.0f, 12.0f, + 13.0f, 14.0f, 15.0f, 16.0f, + 17.0f, 18.0f, 19.0f, 20.0f }; + + GLint a0 = glGetAttribLocation(ph.program, "a0"); + EXPECT_EQ(a0, 0); + glVertexAttribPointer(a0, 3, GL_FLOAT, GL_FALSE, 0, vertices); + glEnableVertexAttribArray(a0); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + GLint a2 = glGetAttribLocation(ph.program, "a2"); + EXPECT_EQ(a2, 2); + glVertexAttribPointer(a2, 4, GL_FLOAT, GL_FALSE, 0, attributes[0]); + glVertexAttribDivisor(a2, 1); + glEnableVertexAttribArray(a2); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + GLint a3 = glGetAttribLocation(ph.program, "a3"); + EXPECT_EQ(a3, 3); // Note: implementation specific + glVertexAttribPointer(a3 + 0, 2, GL_FLOAT, GL_FALSE, 0, &attributes[1][0]); + glVertexAttribPointer(a3 + 1, 2, GL_FLOAT, GL_FALSE, 0, &attributes[1][2]); + glVertexAttribDivisor(a3 + 0, 1); + glVertexAttribDivisor(a3 + 1, 1); + glEnableVertexAttribArray(a3 + 0); + glEnableVertexAttribArray(a3 + 1); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + GLint a5 = glGetAttribLocation(ph.program, "a5"); + EXPECT_EQ(a5, 5); + glVertexAttribPointer(a5, 4, GL_FLOAT, GL_FALSE, 0, attributes[2]); + glVertexAttribDivisor(a5, 1); + glEnableVertexAttribArray(a5); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + GLint a1 = glGetAttribLocation(ph.program, "a1"); + EXPECT_EQ(a1, 1); // Note: implementation specific + glVertexAttribPointer(a1, 4, GL_FLOAT, GL_FALSE, 0, attributes[3]); + glVertexAttribDivisor(a1, 1); + glEnableVertexAttribArray(a1); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + GLint a6 = glGetAttribLocation(ph.program, "a6"); + EXPECT_EQ(a6, 6); // Note: implementation specific + glVertexAttribPointer(a6, 4, GL_FLOAT, GL_FALSE, 0, attributes[4]); + glVertexAttribDivisor(a6, 1); + glEnableVertexAttribArray(a6); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + glUseProgram(ph.program); + glDrawArrays(GL_TRIANGLES, 0, 6); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + deleteProgram(ph); + + unsigned char green[4] = { 0, 255, 0, 255 }; expectFramebufferColor(green); EXPECT_GLENUM_EQ(GL_NONE, glGetError());