Cache transform feedback varying names in the executable
Currently, ANGLE actually does a full link of the programs inside PPOs.
This was never the intention of the spec (hence why an explicit link
doesn't exist). During this link operation, the transform feedback
varying names are used, and they are retrieved from the program itself.
This is not correct, because the transform feedback varyings may have
changed, the program may have failed to relink, and the program pipeline
is expected to continue functioning using the "installed" executable.
Bug: angleproject:5486
Bug: angleproject:8297
Change-Id: I583dbd2abcc51e8536b4c460b92211bdddebda16
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4834055
Reviewed-by: Charlie Lao <cclao@google.com>
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp
index 8e77547..5caab25 100644
--- a/src/libANGLE/Program.cpp
+++ b/src/libANGLE/Program.cpp
@@ -1173,11 +1173,17 @@
auto *platform = ANGLEPlatformCurrent();
double startTime = platform->currentTime(platform);
- // Make sure the executable state is in sync with the program. Only the transform feedback
- // buffer mode is duplicated in the executable as is the only link-input that is also needed at
- // draw time.
+ // Make sure the executable state is in sync with the program.
+ //
+ // The transform feedback buffer mode is duplicated in the executable as is the only link-input
+ // that is also needed at draw time.
+ //
+ // The transform feedback varying names are duplicated because the program pipeline link is not
+ // currently able to use the link result of the program directly (and redoes the link, using
+ // these names).
mState.mExecutable->mPODStruct.transformFeedbackBufferMode =
mState.mTransformFeedbackBufferMode;
+ mState.mExecutable->mTransformFeedbackVaryingNames = mState.mTransformFeedbackVaryingNames;
// Unlink the program, but do not clear the validation-related caching yet, since we can still
// use the previously linked program if linking the shaders fails.
@@ -1335,10 +1341,9 @@
}
mergedVaryings = GetMergedVaryingsFromLinkingVariables(linkingVariables);
- if (!mState.mExecutable->linkMergedVaryings(
- caps, limitations, clientVersion, isWebGL, mergedVaryings,
- mState.mTransformFeedbackVaryingNames, linkingVariables, isSeparable(),
- &resources.varyingPacking))
+ if (!mState.mExecutable->linkMergedVaryings(caps, limitations, clientVersion, isWebGL,
+ mergedVaryings, linkingVariables, isSeparable(),
+ &resources.varyingPacking))
{
return angle::Result::Continue;
}
@@ -3573,6 +3578,12 @@
stream.writeBool(mState.mSeparable);
stream.writeInt(mState.mTransformFeedbackBufferMode);
+ stream.writeInt(mState.mTransformFeedbackVaryingNames.size());
+ for (const std::string &name : mState.mTransformFeedbackVaryingNames)
+ {
+ stream.writeString(name);
+ }
+
mState.mExecutable->save(mState.mSeparable, &stream);
// Warn the app layer if saving a binary with unsupported transform feedback.
@@ -3670,6 +3681,12 @@
mState.mSeparable = stream.readBool();
mState.mTransformFeedbackBufferMode = stream.readInt<GLenum>();
+ mState.mTransformFeedbackVaryingNames.resize(stream.readInt<size_t>());
+ for (std::string &name : mState.mTransformFeedbackVaryingNames)
+ {
+ name = stream.readString();
+ }
+
mState.mExecutable->load(mState.mSeparable, &stream);
static_assert(static_cast<unsigned long>(ShaderType::EnumCount) <= sizeof(unsigned long) * 8,
@@ -3686,6 +3703,7 @@
if (!mState.mAttachedShaders[ShaderType::Compute])
{
mState.mExecutable->updateTransformFeedbackStrides();
+ mState.mExecutable->mTransformFeedbackVaryingNames = mState.mTransformFeedbackVaryingNames;
}
if (context->getShareGroup()->getFrameCaptureShared()->enabled())
diff --git a/src/libANGLE/ProgramExecutable.cpp b/src/libANGLE/ProgramExecutable.cpp
index d93ce3d..2a18d62 100644
--- a/src/libANGLE/ProgramExecutable.cpp
+++ b/src/libANGLE/ProgramExecutable.cpp
@@ -882,21 +882,18 @@
}
}
-bool ProgramExecutable::linkMergedVaryings(
- const Caps &caps,
- const Limitations &limitations,
- const Version &clientVersion,
- bool webglCompatibility,
- const ProgramMergedVaryings &mergedVaryings,
- const std::vector<std::string> &transformFeedbackVaryingNames,
- const LinkingVariables &linkingVariables,
- bool isSeparable,
- ProgramVaryingPacking *varyingPacking)
+bool ProgramExecutable::linkMergedVaryings(const Caps &caps,
+ const Limitations &limitations,
+ const Version &clientVersion,
+ bool webglCompatibility,
+ const ProgramMergedVaryings &mergedVaryings,
+ const LinkingVariables &linkingVariables,
+ bool isSeparable,
+ ProgramVaryingPacking *varyingPacking)
{
ShaderType tfStage = GetLastPreFragmentStage(linkingVariables.isShaderStageUsedBitset);
- if (!linkValidateTransformFeedback(caps, clientVersion, mergedVaryings, tfStage,
- transformFeedbackVaryingNames))
+ if (!linkValidateTransformFeedback(caps, clientVersion, mergedVaryings, tfStage))
{
return false;
}
@@ -930,28 +927,26 @@
}
if (!varyingPacking->collectAndPackUserVaryings(*mInfoLog, caps, packMode, activeShadersMask,
- mergedVaryings, transformFeedbackVaryingNames,
+ mergedVaryings, mTransformFeedbackVaryingNames,
isSeparable))
{
return false;
}
- gatherTransformFeedbackVaryings(mergedVaryings, tfStage, transformFeedbackVaryingNames);
+ gatherTransformFeedbackVaryings(mergedVaryings, tfStage);
updateTransformFeedbackStrides();
return true;
}
-bool ProgramExecutable::linkValidateTransformFeedback(
- const Caps &caps,
- const Version &clientVersion,
- const ProgramMergedVaryings &varyings,
- ShaderType stage,
- const std::vector<std::string> &transformFeedbackVaryingNames)
+bool ProgramExecutable::linkValidateTransformFeedback(const Caps &caps,
+ const Version &clientVersion,
+ const ProgramMergedVaryings &varyings,
+ ShaderType stage)
{
// Validate the tf names regardless of the actual program varyings.
std::set<std::string> uniqueNames;
- for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
+ for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
if (clientVersion < Version(3, 1) && tfVaryingName.find('[') != std::string::npos)
{
@@ -982,7 +977,7 @@
// From OpneGLES spec. 11.1.2.1: A program will fail to link if:
// the count specified by TransformFeedbackVaryings is non-zero, but the
// program object has no vertex, tessellation evaluation, or geometry shader
- if (transformFeedbackVaryingNames.size() > 0 &&
+ if (mTransformFeedbackVaryingNames.size() > 0 &&
!gl::ShaderTypeSupportsTransformFeedback(getLinkedTransformFeedbackStage()))
{
*mInfoLog << "Linked transform feedback stage " << getLinkedTransformFeedbackStage()
@@ -992,7 +987,7 @@
// Validate against program varyings.
size_t totalComponents = 0;
- for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
+ for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
std::vector<unsigned int> subscripts;
std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
@@ -1068,14 +1063,12 @@
return true;
}
-void ProgramExecutable::gatherTransformFeedbackVaryings(
- const ProgramMergedVaryings &varyings,
- ShaderType stage,
- const std::vector<std::string> &transformFeedbackVaryingNames)
+void ProgramExecutable::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings,
+ ShaderType stage)
{
// Gather the linked varyings that are used for transform feedback, they should all exist.
mLinkedTransformFeedbackVaryings.clear();
- for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
+ for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
std::vector<unsigned int> subscripts;
std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
diff --git a/src/libANGLE/ProgramExecutable.h b/src/libANGLE/ProgramExecutable.h
index 038f6a7..57402e8 100644
--- a/src/libANGLE/ProgramExecutable.h
+++ b/src/libANGLE/ProgramExecutable.h
@@ -529,22 +529,16 @@
const Version &clientVersion,
bool webglCompatibility,
const ProgramMergedVaryings &mergedVaryings,
- const std::vector<std::string> &transformFeedbackVaryingNames,
const LinkingVariables &linkingVariables,
bool isSeparable,
ProgramVaryingPacking *varyingPacking);
- bool linkValidateTransformFeedback(
- const Caps &caps,
- const Version &clientVersion,
- const ProgramMergedVaryings &varyings,
- ShaderType stage,
- const std::vector<std::string> &transformFeedbackVaryingNames);
+ bool linkValidateTransformFeedback(const Caps &caps,
+ const Version &clientVersion,
+ const ProgramMergedVaryings &varyings,
+ ShaderType stage);
- void gatherTransformFeedbackVaryings(
- const ProgramMergedVaryings &varyings,
- ShaderType stage,
- const std::vector<std::string> &transformFeedbackVaryingNames);
+ void gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings, ShaderType stage);
void updateTransformFeedbackStrides();
@@ -661,6 +655,15 @@
// Vertex attributes, Fragment input varyings, etc.
std::vector<ProgramInput> mProgramInputs;
std::vector<TransformFeedbackVarying> mLinkedTransformFeedbackVaryings;
+ // Duplicate of ProgramState::mTransformFeedbackVaryingNames. This is cached here because the
+ // xfb names may change, relink may fail, yet program pipeline link should be able to function
+ // with the last installed executable. In truth, program pipeline link should have been able to
+ // hoist transform feedback varyings directly from the executable, among most other things, but
+ // that is currently not done.
+ //
+ // This array is not serialized, it's already done by the program, and will be duplicated during
+ // deserialization.
+ std::vector<std::string> mTransformFeedbackVaryingNames;
// The size of the data written to each transform feedback buffer per vertex.
std::vector<GLsizei> mTransformFeedbackStrides;
// Uniforms are sorted in order:
diff --git a/src/libANGLE/ProgramPipeline.cpp b/src/libANGLE/ProgramPipeline.cpp
index e74cd99..aa43cdc 100644
--- a/src/libANGLE/ProgramPipeline.cpp
+++ b/src/libANGLE/ProgramPipeline.cpp
@@ -540,12 +540,12 @@
tfProgram = mState.mPrograms[ShaderType::Vertex];
}
- const std::vector<std::string> &transformFeedbackVaryingNames =
- tfProgram->getState().getTransformFeedbackVaryingNames();
+ mState.mExecutable->mTransformFeedbackVaryingNames =
+ tfProgram->getExecutable().mTransformFeedbackVaryingNames;
if (!mState.mExecutable->linkMergedVaryings(caps, limitations, clientVersion, isWebGL,
- mergedVaryings, transformFeedbackVaryingNames,
- linkingVariables, false, &varyingPacking))
+ mergedVaryings, linkingVariables, false,
+ &varyingPacking))
{
return angle::Result::Stop;
}
diff --git a/src/tests/gl_tests/ProgramPipelineTest.cpp b/src/tests/gl_tests/ProgramPipelineTest.cpp
index 78fd46c..1e90d7f 100644
--- a/src/tests/gl_tests/ProgramPipelineTest.cpp
+++ b/src/tests/gl_tests/ProgramPipelineTest.cpp
@@ -151,8 +151,16 @@
const std::vector<std::string> &tfVaryings,
GLenum bufferMode)
{
- mVertProg = glCreateShaderProgramv(GL_VERTEX_SHADER, 1, &vertString);
- ASSERT_NE(mVertProg, 0u);
+ GLShader vertShader(GL_VERTEX_SHADER);
+ mVertProg = glCreateProgram();
+
+ glShaderSource(vertShader, 1, &vertString, nullptr);
+ glCompileShader(vertShader);
+ glProgramParameteri(mVertProg, GL_PROGRAM_SEPARABLE, GL_TRUE);
+ glAttachShader(mVertProg, vertShader);
+ glLinkProgram(mVertProg);
+ EXPECT_GL_NO_ERROR();
+
mFragProg = glCreateShaderProgramv(GL_FRAGMENT_SHADER, 1, &fragString);
ASSERT_NE(mFragProg, 0u);
@@ -1245,8 +1253,6 @@
// Only the Vulkan backend supports PPOs
ANGLE_SKIP_TEST_IF(!IsVulkan());
ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
- // http://anglebug.com/5486
- ANGLE_SKIP_TEST_IF(IsVulkan());
constexpr char kVS[] =
R"(#version 310 es
@@ -1254,12 +1260,13 @@
precision highp float;
in vec4 inputAttribute;
- out Block_inout { vec4 value; } user_out;
+ out Block_inout { vec4 value; vec4 value2; } user_out;
void main()
{
gl_Position = inputAttribute;
user_out.value = vec4(4.0, 5.0, 6.0, 7.0);
+ user_out.value2 = vec4(8.0, 9.0, 10.0, 11.0);
})";
constexpr char kFS[] =
@@ -1268,7 +1275,7 @@
precision highp float;
layout(location = 0) out mediump vec4 color;
- in Block_inout { vec4 value; } user_in;
+ in Block_inout { vec4 value; vec4 value2; } user_in;
void main()
{
@@ -1276,9 +1283,16 @@
})";
std::vector<std::string> tfVaryings;
tfVaryings.push_back("Block_inout.value");
+ tfVaryings.push_back("Block_inout.value2");
bindProgramPipelineWithXFBVaryings(kVS, kFS, tfVaryings, GL_INTERLEAVED_ATTRIBS);
glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, mTransformFeedbackBuffer);
+ // Make sure reconfiguring the vertex shader's transform feedback varyings without a link does
+ // not affect the pipeline. Same with changing buffer modes
+ std::vector<const char *> tfVaryingsBogus = {"some", "invalid[0]", "names"};
+ glTransformFeedbackVaryings(mVertProg, static_cast<GLsizei>(tfVaryingsBogus.size()),
+ tfVaryingsBogus.data(), GL_SEPARATE_ATTRIBS);
+
glBeginTransformFeedback(GL_TRIANGLES);
drawQuadWithPPO("inputAttribute", 0.5f, 1.0f);
glEndTransformFeedback();
@@ -1287,11 +1301,11 @@
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
void *mappedBuffer =
- glMapBufferRange(GL_TRANSFORM_FEEDBACK_BUFFER, 0, sizeof(float) * 4, GL_MAP_READ_BIT);
+ glMapBufferRange(GL_TRANSFORM_FEEDBACK_BUFFER, 0, sizeof(float) * 8, GL_MAP_READ_BIT);
ASSERT_NE(nullptr, mappedBuffer);
float *mappedFloats = static_cast<float *>(mappedBuffer);
- for (unsigned int cnt = 0; cnt < 4; ++cnt)
+ for (unsigned int cnt = 0; cnt < 8; ++cnt)
{
EXPECT_EQ(4 + cnt, mappedFloats[cnt]);
}
@@ -1480,9 +1494,9 @@
my_FragColor = texture(tex, texCoord);
})";
- std::array<GLColor, kWidth *kHeight> redColor = {
+ std::array<GLColor, kWidth * kHeight> redColor = {
{GLColor::red, GLColor::red, GLColor::red, GLColor::red}};
- std::array<GLColor, kWidth *kHeight> greenColor = {
+ std::array<GLColor, kWidth * kHeight> greenColor = {
{GLColor::green, GLColor::green, GLColor::green, GLColor::green}};
// Create a red texture and bind to texture unit 0
