From: Steinar H. Gunderson Date: Sat, 5 Nov 2016 10:32:56 +0000 (+0100) Subject: Fix an issue where a (cached) shader program could be used from multiple X-Git-Tag: 1.4.0~2 X-Git-Url: https://git.sesse.net/?p=movit;a=commitdiff_plain;h=f216b7bef5a968c89f6fc78e83cc26a91e504a8a 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 60c65f8..2ba42e9 100644 --- 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)); } - 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); } diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp index 5ffb105..cb30d4d 100644 --- 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 "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 diff --git a/resource_pool.cpp b/resource_pool.cpp index 6939fe0..b560f15 100644 --- a/resource_pool.cpp +++ b/resource_pool.cpp @@ -89,14 +89,24 @@ void ResourcePool::delete_program(GLuint glsl_program_num) } } assert(found_program); - glDeleteProgram(glsl_program_num); - map >::iterator shader_it = + map >::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::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 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& 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 &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::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::const_iterator master_it = program_masters.find(instance_program_num); + assert(master_it != program_masters.end()); + + assert(program_instances.count(master_it->second)); + stack &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); diff --git a/resource_pool.h b/resource_pool.h index 5cc2e82..8503b29 100644 --- a/resource_pool.h +++ b/resource_pool.h @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,18 @@ public: const std::vector& 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 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 . + static GLuint link_program(GLuint vs_obj, + GLuint fs_obj, + const std::vector& fragment_shader_outputs); + // Protects all the other elements in the class. pthread_mutex_t lock; @@ -124,7 +143,22 @@ private: std::map program_refcount; // A mapping from program number to vertex and fragment shaders. - std::map > program_shaders; + // Contains everything needed to re-link the program. + struct ShaderSpec { + GLuint vs_obj, fs_obj; + std::vector fragment_shader_outputs; + }; + std::map 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 > program_instances; + + // For each program, the master program that created it + // (inverse of program_instances). + std::map program_masters; // A list of programs that are no longer in use, most recently freed first. // Once this reaches , the last element diff --git a/version.h b/version.h index aad8b87..3c3989a 100644 --- 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)