Fix an issue where a (cached) shader program could be used from multiple
authorSteinar H. Gunderson <sgunderson@bigfoot.com>
Sat, 5 Nov 2016 10:32:56 +0000 (11:32 +0100)
committerSteinar H. Gunderson <sgunderson@bigfoot.com>
Sat, 5 Nov 2016 10:43:20 +0000 (11:43 +0100)
threads at a time.

This isn't allowed, since uniforms belong to the program, not to the
context. Found by running Helgrind over Nageru.

effect_chain.cpp
effect_chain_test.cpp
resource_pool.cpp
resource_pool.h
version.h

index 60c65f8..2ba42e9 100644 (file)
@@ -1882,9 +1882,6 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase,
                output_textures->insert(make_pair(phase, tex_num));
        }
 
-       glUseProgram(phase->glsl_program_num);
-       check_error();
-
        // Set up RTT inputs for this phase.
        for (unsigned sampler = 0; sampler < phase->inputs.size(); ++sampler) {
                glActiveTexture(GL_TEXTURE0 + sampler);
@@ -1908,12 +1905,15 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase,
                glViewport(0, 0, phase->output_width, phase->output_height);
        }
 
+       GLuint instance_program_num = resource_pool->use_glsl_program(phase->glsl_program_num);
+       check_error();
+
        // Give the required parameters to all the effects.
        unsigned sampler_num = phase->inputs.size();
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
                unsigned old_sampler_num = sampler_num;
-               node->effect->set_gl_state(phase->glsl_program_num, phase->effect_ids[node], &sampler_num);
+               node->effect->set_gl_state(instance_program_num, phase->effect_ids[node], &sampler_num);
                check_error();
 
                if (node->effect->is_single_texture()) {
@@ -1961,6 +1961,8 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase,
                node->effect->clear_gl_state();
        }
 
+       resource_pool->unuse_glsl_program(instance_program_num);
+
        if (!last_phase) {
                resource_pool->release_fbo(fbo);
        }
index 5ffb105..cb30d4d 100644 (file)
@@ -18,6 +18,7 @@
 #include "mirror_effect.h"
 #include "multiply_effect.h"
 #include "resize_effect.h"
+#include "resource_pool.h"
 #include "test_util.h"
 #include "util.h"
 
@@ -1416,4 +1417,51 @@ TEST(EffectChainTest, SquareRootIntermediateIsTurnedOffForNonLinearData) {
        expect_equal(data, out_data, size, 1, 1e-6, 1e-6);
 }
 
+// An effect that stores which program number was last run under.
+class RecordingIdentityEffect : public Effect {
+public:
+       RecordingIdentityEffect() {}
+       virtual string effect_type_id() const { return "RecordingIdentityEffect"; }
+       string output_fragment_shader() { return read_file("identity.frag"); }
+
+       GLuint last_glsl_program_num;
+       void set_gl_state(GLuint glsl_program_num, const std::string& prefix, unsigned *sampler_num)
+       {
+               last_glsl_program_num = glsl_program_num;
+       }
+};
+
+TEST(EffectChainTest, ProgramsAreClonedForMultipleThreads) {
+       float data[] = {
+               0.0f, 0.25f, 0.3f,
+               0.75f, 1.0f, 1.0f,
+       };
+       float out_data[6];
+       EffectChainTester tester(data, 3, 2, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_LINEAR);
+       RecordingIdentityEffect *effect = new RecordingIdentityEffect();
+       tester.get_chain()->add_effect(effect);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+       expect_equal(data, out_data, 3, 2);
+
+       ASSERT_NE(0, effect->last_glsl_program_num);
+
+       // Now pretend some other effect is using this program number;
+       // ResourcePool will then need to clone it.
+       ResourcePool *resource_pool = tester.get_chain()->get_resource_pool();
+       GLuint master_program_num = resource_pool->use_glsl_program(effect->last_glsl_program_num);
+       EXPECT_EQ(effect->last_glsl_program_num, master_program_num);
+
+       // Re-run should still give the correct data, but it should have run
+       // with a different program.
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(data, out_data, 3, 2);
+       EXPECT_NE(effect->last_glsl_program_num, master_program_num);
+
+       // Release the program, and check one final time.
+       resource_pool->unuse_glsl_program(master_program_num);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(data, out_data, 3, 2);
+}
+
 }  // namespace movit
index 6939fe0..b560f15 100644 (file)
@@ -89,14 +89,24 @@ void ResourcePool::delete_program(GLuint glsl_program_num)
                }
        }
        assert(found_program);
-       glDeleteProgram(glsl_program_num);
 
-       map<GLuint, pair<GLuint, GLuint> >::iterator shader_it =
+       map<GLuint, stack<GLuint> >::iterator instance_list_it = program_instances.find(glsl_program_num);
+       assert(instance_list_it != program_instances.end());
+
+       while (!instance_list_it->second.empty()) {
+               GLuint instance_program_num = instance_list_it->second.top();
+               instance_list_it->second.pop();
+               glDeleteProgram(instance_program_num);
+               program_masters.erase(instance_program_num);
+       }
+       program_instances.erase(instance_list_it);
+
+       map<GLuint, ShaderSpec>::iterator shader_it =
                program_shaders.find(glsl_program_num);
        assert(shader_it != program_shaders.end());
 
-       glDeleteShader(shader_it->second.first);
-       glDeleteShader(shader_it->second.second);
+       glDeleteShader(shader_it->second.vs_obj);
+       glDeleteShader(shader_it->second.fs_obj);
        program_shaders.erase(shader_it);
 }
 
@@ -133,36 +143,11 @@ GLuint ResourcePool::compile_glsl_program(const string& vertex_shader,
                }
        } else {
                // Not in the cache. Compile the shaders.
-               glsl_program_num = glCreateProgram();
-               check_error();
                GLuint vs_obj = compile_shader(vertex_shader, GL_VERTEX_SHADER);
                check_error();
                GLuint fs_obj = compile_shader(fragment_shader_processed, GL_FRAGMENT_SHADER);
                check_error();
-               glAttachShader(glsl_program_num, vs_obj);
-               check_error();
-               glAttachShader(glsl_program_num, fs_obj);
-               check_error();
-
-               // Bind the outputs, if we have multiple ones.
-               if (fragment_shader_outputs.size() > 1) {
-                       for (unsigned output_index = 0; output_index < fragment_shader_outputs.size(); ++output_index) {
-                               glBindFragDataLocation(glsl_program_num, output_index,
-                                                      fragment_shader_outputs[output_index].c_str());
-                       }
-               }
-
-               glLinkProgram(glsl_program_num);
-               check_error();
-
-               GLint success;
-               glGetProgramiv(glsl_program_num, GL_LINK_STATUS, &success);
-               if (success == GL_FALSE) {
-                       GLchar error_log[1024] = {0};
-                       glGetProgramInfoLog(glsl_program_num, 1024, NULL, error_log);
-                       fprintf(stderr, "Error linking program: %s\n", error_log);
-                       exit(1);
-               }
+               glsl_program_num = link_program(vs_obj, fs_obj, fragment_shader_outputs);
 
                if (movit_debug_level == MOVIT_DEBUG_ON) {
                        // Output shader to a temporary file, for easier debugging.
@@ -180,12 +165,54 @@ GLuint ResourcePool::compile_glsl_program(const string& vertex_shader,
 
                programs.insert(make_pair(key, glsl_program_num));
                program_refcount.insert(make_pair(glsl_program_num, 1));
-               program_shaders.insert(make_pair(glsl_program_num, make_pair(vs_obj, fs_obj)));
+               ShaderSpec spec;
+               spec.vs_obj = vs_obj;
+               spec.fs_obj = fs_obj;
+               spec.fragment_shader_outputs = fragment_shader_outputs;
+               program_shaders.insert(make_pair(glsl_program_num, spec));
+               stack<GLuint> instances;
+               instances.push(glsl_program_num);
+               program_instances.insert(make_pair(glsl_program_num, instances));
+               program_masters.insert(make_pair(glsl_program_num, glsl_program_num));
        }
        pthread_mutex_unlock(&lock);
        return glsl_program_num;
 }
 
+GLuint ResourcePool::link_program(GLuint vs_obj,
+                                  GLuint fs_obj,
+                                  const vector<string>& fragment_shader_outputs)
+{
+       GLuint glsl_program_num = glCreateProgram();
+       check_error();
+       glAttachShader(glsl_program_num, vs_obj);
+       check_error();
+       glAttachShader(glsl_program_num, fs_obj);
+       check_error();
+
+       // Bind the outputs, if we have multiple ones.
+       if (fragment_shader_outputs.size() > 1) {
+               for (unsigned output_index = 0; output_index < fragment_shader_outputs.size(); ++output_index) {
+                       glBindFragDataLocation(glsl_program_num, output_index,
+                                              fragment_shader_outputs[output_index].c_str());
+               }
+       }
+
+       glLinkProgram(glsl_program_num);
+       check_error();
+
+       GLint success;
+       glGetProgramiv(glsl_program_num, GL_LINK_STATUS, &success);
+       if (success == GL_FALSE) {
+               GLchar error_log[1024] = {0};
+               glGetProgramInfoLog(glsl_program_num, 1024, NULL, error_log);
+               fprintf(stderr, "Error linking program: %s\n", error_log);
+               exit(1);
+       }
+
+       return glsl_program_num;
+}
+
 void ResourcePool::release_glsl_program(GLuint glsl_program_num)
 {
        pthread_mutex_lock(&lock);
@@ -206,6 +233,51 @@ void ResourcePool::release_glsl_program(GLuint glsl_program_num)
        pthread_mutex_unlock(&lock);
 }
 
+GLuint ResourcePool::use_glsl_program(GLuint glsl_program_num)
+{
+       pthread_mutex_lock(&lock);
+       assert(program_instances.count(glsl_program_num));
+       stack<GLuint> &instances = program_instances[glsl_program_num];
+
+       GLuint instance_program_num;
+       if (!instances.empty()) {
+               // There's an unused instance of this program; just return it.
+               instance_program_num = instances.top();
+               instances.pop();
+       } else {
+               // We need to clone this program. (unuse_glsl_program()
+               // will later put it onto the list.)
+               map<GLuint, ShaderSpec>::iterator shader_it =
+                       program_shaders.find(glsl_program_num);
+               assert(shader_it != program_shaders.end());
+
+               instance_program_num = link_program(
+                       shader_it->second.vs_obj,
+                       shader_it->second.fs_obj,
+                       shader_it->second.fragment_shader_outputs);
+               program_masters.insert(make_pair(instance_program_num, glsl_program_num));
+       }
+       pthread_mutex_unlock(&lock);
+
+       glUseProgram(instance_program_num);
+       return instance_program_num;
+}
+
+void ResourcePool::unuse_glsl_program(GLuint instance_program_num)
+{
+       pthread_mutex_lock(&lock);
+
+       map<GLuint, GLuint>::const_iterator master_it = program_masters.find(instance_program_num);
+       assert(master_it != program_masters.end());
+
+       assert(program_instances.count(master_it->second));
+       stack<GLuint> &instances = program_instances[master_it->second];
+
+       instances.push(instance_program_num);
+
+       pthread_mutex_unlock(&lock);
+}
+
 GLuint ResourcePool::create_2d_texture(GLint internal_format, GLsizei width, GLsizei height)
 {
        assert(width > 0);
index 5cc2e82..8503b29 100644 (file)
@@ -27,6 +27,7 @@
 #include <stddef.h>
 #include <list>
 #include <map>
+#include <stack>
 #include <string>
 #include <utility>
 #include <vector>
@@ -67,6 +68,18 @@ public:
                                    const std::vector<std::string>& frag_shader_outputs);
        void release_glsl_program(GLuint glsl_program_num);
 
+       // Since uniforms belong to the program and not to the context,
+       // a given GLSL program number can't be used by more than one thread
+       // at a time. Thus, if two threads want to use the same program
+       // (usually because two EffectChains share them via caching),
+       // we will need to make a clone. use_glsl_program() makes such
+       // a clone if needed, calls glUseProgram(), and returns the real
+       // program number that was used; this must be given to
+       // unuse_glsl_program() to release it. unuse_glsl_program() does not
+       // actually change any OpenGL state, though.
+       GLuint use_glsl_program(GLuint glsl_program_num);
+       void unuse_glsl_program(GLuint instance_program_num);
+
        // Allocate a 2D texture of the given internal format and dimensions,
        // or fetch a previous used if possible. Unbinds GL_TEXTURE_2D afterwards.
        // Keeps ownership of the texture; you must call release_2d_texture() instead
@@ -110,6 +123,12 @@ private:
        // is no more than <max_length> elements long.
        void shrink_fbo_freelist(void *context, size_t max_length);
 
+       // Link the given vertex and fragment shaders into a full GLSL program.
+       // See compile_glsl_program() for explanation of <fragment_shader_outputs>.
+       static GLuint link_program(GLuint vs_obj,
+                                  GLuint fs_obj,
+                                  const std::vector<std::string>& fragment_shader_outputs);
+
        // Protects all the other elements in the class.
        pthread_mutex_t lock;
 
@@ -124,7 +143,22 @@ private:
        std::map<GLuint, int> program_refcount;
 
        // A mapping from program number to vertex and fragment shaders.
-       std::map<GLuint, std::pair<GLuint, GLuint> > program_shaders;
+       // Contains everything needed to re-link the program.
+       struct ShaderSpec {
+               GLuint vs_obj, fs_obj;
+               std::vector<std::string> fragment_shader_outputs;
+       };
+       std::map<GLuint, ShaderSpec> program_shaders;
+
+       // For each program, a list of other programs that are exactly like it.
+       // By default, will only contain the program itself, but due to cloning
+       // (see use_glsl_program()), may grow. Programs are taken off this list
+       // while they are in use (by use_glsl_program()).
+       std::map<GLuint, std::stack<GLuint> > program_instances;
+
+       // For each program, the master program that created it
+       // (inverse of program_instances).
+       std::map<GLuint, GLuint> program_masters;
 
        // A list of programs that are no longer in use, most recently freed first.
        // Once this reaches <program_freelist_max_length>, the last element
index aad8b87..3c3989a 100644 (file)
--- a/version.h
+++ b/version.h
@@ -5,6 +5,6 @@
 // changes, even within git versions. There is no specific version
 // documentation outside the regular changelogs, though.
 
-#define MOVIT_VERSION 22
+#define MOVIT_VERSION 23
 
 #endif // !defined(_MOVIT_VERSION_H)