Fix an issue where a (cached) shader program could be used from multiple

threads at a time.

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

diff --git a/effect_chain.cpp b/effect_chain.cpp
index 60c65f81b743fe48148080d21a829aa16eb4163a..2ba42e94ae7cc0aa4fd35c6854b2556f8c6f7ee4 100644 (file)
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -1882,9 +1882,6 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase,
                output_textures->insert(make_pair(phase, tex_num));
        }
 
                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);
        // 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);
        }
 
                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;
        // 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()) {
                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();
        }
 
                node->effect->clear_gl_state();
        }
 

+       resource_pool->unuse_glsl_program(instance_program_num);
+

        if (!last_phase) {
                resource_pool->release_fbo(fbo);
        }
        if (!last_phase) {
                resource_pool->release_fbo(fbo);
        }

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index 5ffb105cdb75ccd805caa1f9f25bfd322dac6f1e..cb30d4d2e8a736c009c1381d5953fd41da4eb6e0 100644 (file)
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -18,6 +18,7 @@
 #include "mirror_effect.h"
 #include "multiply_effect.h"
 #include "resize_effect.h"
 #include "mirror_effect.h"
 #include "multiply_effect.h"
 #include "resize_effect.h"

+#include "resource_pool.h"

 #include "test_util.h"
 #include "util.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);
 }
 
        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
 }  // namespace movit

diff --git a/resource_pool.cpp b/resource_pool.cpp
index 6939fe01d50a61131d4357858c6d82a4d9361da2..b560f1530bf4599692fdb1f63e026cc3cfa07994 100644 (file)
--- a/resource_pool.cpp
+++ b/resource_pool.cpp
@@ -89,14 +89,24 @@ void ResourcePool::delete_program(GLuint glsl_program_num)
                }
        }
        assert(found_program);
                }
        }
        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());
 
                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);
 }
 
        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.
                }
        } 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();
                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.
 
                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));
 
                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;
 }
 
        }
        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);
 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);
 }
 
        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);
 GLuint ResourcePool::create_2d_texture(GLint internal_format, GLsizei width, GLsizei height)
 {
        assert(width > 0);

diff --git a/resource_pool.h b/resource_pool.h
index 5cc2e829e8f836635b04980978372973d3618639..8503b29b6e0020d0111e83c459905a57694e9cd3 100644 (file)
--- a/resource_pool.h
+++ b/resource_pool.h
@@ -27,6 +27,7 @@
 #include <stddef.h>
 #include <list>
 #include <map>
 #include <stddef.h>
 #include <list>
 #include <map>

+#include <stack>

 #include <string>
 #include <utility>
 #include <vector>
 #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);
 
                                    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
        // 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);
 
        // 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;
 
        // 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, 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
 
        // A list of programs that are no longer in use, most recently freed first.
        // Once this reaches <program_freelist_max_length>, the last element

diff --git a/version.h b/version.h
index aad8b87a7f3c126aab45375af73f1e2cfc293323..3c3989ac9752d755154079a6cd78facaee89f7ef 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.
 
 // 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)
 
 #endif // !defined(_MOVIT_VERSION_H)