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 <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/OpenGL/compiler/OutputASM.cpp b/src/OpenGL/compiler/OutputASM.cpp
index 9ed34b2..c18ecf7 100644
--- a/src/OpenGL/compiler/OutputASM.cpp
+++ b/src/OpenGL/compiler/OutputASM.cpp
@@ -400,12 +400,12 @@
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 2f90266..b037a67 100644
--- a/src/OpenGL/compiler/OutputASM.h
+++ b/src/OpenGL/compiler/OutputASM.h
@@ -144,12 +144,12 @@
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 77912cc..dc64813 100644
--- a/src/OpenGL/libGLESv2/Program.cpp
+++ b/src/OpenGL/libGLESv2/Program.cpp
@@ -277,19 +277,7 @@
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 @@
// 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;
- }
- }
- }
- }
-
- linkedAttributeLocation[attribute.name] = location;
- linkedAttribute.push_back(attribute);
- for(int i = 0; i < rows; i++)
- {
- usedLocations |= 1 << (location + i);
- }
}
}
- // Link attributes that don't have a binding location
+ // Link attributes that have an API provided binding location but no GLSL layout location
for(auto const &attribute : vertexShader->activeAttributes)
{
- int location = (attributeBinding.find(attribute.name) != attributeBinding.end()) ? attributeBinding[attribute.name] : -1;
+ int bindingLocation = (attributeBinding.find(attribute.name) != attributeBinding.end()) ? attributeBinding[attribute.name] : -1;
- if(location == -1) // Not set by glBindAttribLocation
+ if(attribute.layoutLocation == -1 && bindingLocation != -1)
{
- int rows = VariableRegisterCount(attribute.type);
- int availableIndex = AllocateFirstFreeBits(&usedLocations, rows, MAX_VERTEX_ATTRIBS);
-
- if(availableIndex == -1 || availableIndex + rows > MAX_VERTEX_ATTRIBS)
+ if(!linkAttribute(attribute, bindingLocation, 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)
+ // Link attributes that don't have a binding location nor a layout location
+ for(auto const &attribute : vertexShader->activeAttributes)
{
- int location = getAttributeBinding(it);
+ if(attribute.layoutLocation == -1 && attributeBinding.find(attribute.name) == attributeBinding.end())
+ {
+ if(!linkAttribute(attribute, -1, usedLocations))
+ {
+ return false;
+ }
+ }
+ }
+
+ ASSERT(linkedAttribute.size() == vertexShader->activeAttributes.size());
+
+ for(auto const &attribute : linkedAttribute)
+ {
+ 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 @@
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<std::string, GLuint>::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<std::string, GLuint>::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 501d081..a232926 100644
--- a/src/OpenGL/libGLESv2/Program.h
+++ b/src/OpenGL/libGLESv2/Program.h
@@ -228,7 +228,8 @@
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 789b0c8..b3f1d20 100644
--- a/src/OpenGL/libGLESv2/libGLESv2.cpp
+++ b/src/OpenGL/libGLESv2/libGLESv2.cpp
@@ -2345,7 +2345,6 @@
if(context)
{
-
es2::Program *programObject = context->getProgram(program);
if(!programObject)
diff --git a/tests/unittests/unittests.cpp b/tests/unittests/unittests.cpp
index 80bc887..91b34cd 100644
--- a/tests/unittests/unittests.cpp
+++ b/tests/unittests/unittests.cpp
@@ -266,6 +266,8 @@
glGetProgramiv(ph.program, GL_LINK_STATUS, &linkStatus);
EXPECT_NE(linkStatus, 0);
+ EXPECT_GLENUM_EQ(GL_NONE, glGetError());
+
return ph;
}
@@ -274,6 +276,8 @@
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 @@
{
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 @@
deleteProgram(ph);
+ unsigned char green[4] = { 0, 255, 0, 255 };
expectFramebufferColor(green);
EXPECT_GLENUM_EQ(GL_NONE, glGetError());
@@ -476,8 +479,6 @@
{
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 @@
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());
