From 54d47f65e9abac77229636fbaaefea8caf34a4d4 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Sun, 7 Feb 2016 00:11:51 +0100 Subject: [PATCH] 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. --- effect_chain.cpp | 45 +++++++++++++++++++++---------------------- effect_chain_test.cpp | 32 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/effect_chain.cpp b/effect_chain.cpp index 30dc3e4..385a87b 100644 --- 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 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, maprelease_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 32beb41..6ab874d 100644 --- 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: -- 2.39.2