Revert "Reuse the VAO across all phases."

The patch trickles a bug where, if the first phase doesn't need texture
coordinates, the rest of the phases don't get it either. (Or more generally,
if the vertex shader varying indices are not predictable, the patch does
the wrong thing.) Add a unit test and revert it for now; in time, we'll find a
way that's both low-overhead (the patch fixes a real problem) _and_ correct in
these cases.

This reverts patch 5e34f7a8969f4afc169f034d34fb908019b3a389.

Reported by Christophe Thommeret.

diff --git a/effect_chain.cpp b/effect_chain.cpp
index 30dc3e499a7c334f4d0a8e867e9ebbf2d97dd901..385a87b9f458bd4ed417de9737f709475d0a214a 100644 (file)
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -1699,23 +1699,6 @@ void EffectChain::render_to_fbo(GLuint dest_fbo, unsigned width, unsigned height
        glDepthMask(GL_FALSE);
        check_error();
 
-       // Generate a VAO. All the phases should have exactly the same vertex attributes,
-       // so it's safe to reuse this.
-       float vertices[] = {
-               0.0f, 2.0f,
-               0.0f, 0.0f,
-               2.0f, 0.0f
-       };
-
-       GLuint vao;
-       glGenVertexArrays(1, &vao);
-       check_error();
-       glBindVertexArray(vao);
-       check_error();
-
-       GLuint position_vbo = fill_vertex_attribute(phases[0]->glsl_program_num, "position", 2, GL_FLOAT, sizeof(vertices), vertices);
-       GLuint texcoord_vbo = fill_vertex_attribute(phases[0]->glsl_program_num, "texcoord", 2, GL_FLOAT, sizeof(vertices), vertices);  // Same as vertices.
-
        set<Phase *> generated_mipmaps;
 
        // We choose the simplest option of having one texture per output,
@@ -1757,12 +1740,6 @@ void EffectChain::render_to_fbo(GLuint dest_fbo, unsigned width, unsigned height
        glUseProgram(0);
        check_error();
 
-       cleanup_vertex_attribute(phases[0]->glsl_program_num, "position", position_vbo);
-       cleanup_vertex_attribute(phases[0]->glsl_program_num, "texcoord", texcoord_vbo);
-
-       glDeleteVertexArrays(1, &vao);
-       check_error();
-
        if (do_phase_timing) {
                // Get back the timer queries.
                for (unsigned phase_num = 0; phase_num < phases.size(); ++phase_num) {
@@ -1876,9 +1853,28 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase, map<Phase *, GLui
        // from there.
        setup_uniforms(phase);
 
+       // Now draw!
+       float vertices[] = {
+               0.0f, 2.0f,
+               0.0f, 0.0f,
+               2.0f, 0.0f
+       };
+
+       GLuint vao;
+       glGenVertexArrays(1, &vao);
+       check_error();
+       glBindVertexArray(vao);
+       check_error();
+
+       GLuint position_vbo = fill_vertex_attribute(glsl_program_num, "position", 2, GL_FLOAT, sizeof(vertices), vertices);
+       GLuint texcoord_vbo = fill_vertex_attribute(glsl_program_num, "texcoord", 2, GL_FLOAT, sizeof(vertices), vertices);  // Same as vertices.
+
        glDrawArrays(GL_TRIANGLES, 0, 3);
        check_error();
 
+       cleanup_vertex_attribute(glsl_program_num, "position", position_vbo);
+       cleanup_vertex_attribute(glsl_program_num, "texcoord", texcoord_vbo);
+       
        glUseProgram(0);
        check_error();
 
@@ -1890,6 +1886,9 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase, map<Phase *, GLui
        if (!last_phase) {
                resource_pool->release_fbo(fbo);
        }
+
+       glDeleteVertexArrays(1, &vao);
+       check_error();
 }
 
 void EffectChain::setup_uniforms(Phase *phase)

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index 32beb41b45df0c4e67c7490f884f3581eb2b2963..6ab874d2e4e32507beb5cc9eeb546fac69e9f546 100644 (file)
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -1022,6 +1022,38 @@ TEST(EffectChainTest, AspectRatioConversion) {
        EXPECT_EQ(7, input_store->input_height);
 }
 
+// Tests that putting a BlueInput (constant color) into its own pass,
+// which creates a phase that doesn't need texture coordinates,
+// doesn't mess up a second phase that actually does.
+TEST(EffectChainTest, FirstPhaseWithNoTextureCoordinates) {
+       const int size = 2;
+       float data[] = {
+               1.0f,
+               0.0f,
+       };
+       float expected_data[] = {
+               1.0f, 1.0f, 2.0f, 2.0f,
+               0.0f, 0.0f, 1.0f, 2.0f,
+       };
+       float out_data[size * 4];
+       // First say that we have sRGB, linear input.
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+       FlatInput *input = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 1, size);
+
+       input->set_pixel_data(data);
+       EffectChainTester tester(NULL, 1, size);
+       tester.get_chain()->add_input(new BlueInput());
+       Effect *phase1_end = tester.get_chain()->add_effect(new BouncingIdentityEffect());
+       tester.get_chain()->add_input(input);
+       tester.get_chain()->add_effect(new AddEffect(), phase1_end, input);
+
+       tester.run(out_data, GL_RGBA, COLORSPACE_sRGB, GAMMA_LINEAR, OUTPUT_ALPHA_FORMAT_POSTMULTIPLIED);
+
+       expect_equal(expected_data, out_data, 4, size);
+}
+
 // An effect that does nothing except changing its output sizes.
 class VirtualResizeEffect : public Effect {
 public: