Fix more confusion with strong one-to-one effects and compute shaders.
authorSteinar H. Gunderson <sgunderson@bigfoot.com>
Wed, 14 Mar 2018 21:56:42 +0000 (22:56 +0100)
committerSteinar H. Gunderson <sgunderson@bigfoot.com>
Wed, 14 Mar 2018 21:56:42 +0000 (22:56 +0100)
The resulting unit test also found an issue where we could add a
dummy effect at the end but then end up not having a compute shader
there, which would cause OpenGL errors as the internal state got
confused, so fix that as well.

effect_chain.cpp
effect_chain_test.cpp

index 7389efd..deeea91 100644 (file)
@@ -432,6 +432,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
                                // strong one-to-one).
                                frag_shader += "(tc) CS_OUTPUT_VAL\n";
                        } else {
+                               assert(phase->effect_ids.count(make_pair(input, link_type)));
                                frag_shader += string(" ") + phase->effect_ids[make_pair(input, link_type)] + "\n";
                        }
                }
@@ -455,6 +456,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
                frag_shader += "\n";
        }
        if (phase->is_compute_shader) {
+               assert(phase->effect_ids.count(make_pair(phase->compute_shader_node, IN_SAME_PHASE)));
                frag_shader += string("#define INPUT ") + phase->effect_ids[make_pair(phase->compute_shader_node, IN_SAME_PHASE)] + "\n";
                if (phase->compute_shader_node == phase->effects.back()) {
                        // No postprocessing.
@@ -463,6 +465,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
                        frag_shader += string("#define CS_POSTPROC ") + phase->effect_ids[make_pair(phase->effects.back(), IN_SAME_PHASE)] + "\n";
                }
        } else {
+               assert(phase->effect_ids.count(make_pair(phase->effects.back(), IN_SAME_PHASE)));
                frag_shader += string("#define INPUT ") + phase->effect_ids[make_pair(phase->effects.back(), IN_SAME_PHASE)] + "\n";
        }
 
@@ -766,7 +769,7 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                                        // If all nodes so far are strong one-to-one, we can put them after
                                        // the compute shader (ie., process them on the output).
                                        start_new_phase = true;
-                               } else {
+                               } else if (!start_new_phase) {
                                        phase->is_compute_shader = true;
                                        phase->compute_shader_node = deps[i];
                                }
@@ -1916,6 +1919,22 @@ void EffectChain::finalize()
 
        output_dot("step21-split-to-phases.dot");
 
+       // There are some corner cases where we thought we needed to add a dummy
+       // effect, but then it turned out later we didn't (e.g. induces_compute_shader()
+       // didn't see a mipmap conflict coming, which would cause the compute shader
+       // to be split off from the inal phase); if so, remove the extra phase
+       // at the end, since it will give us some trouble during execution.
+       //
+       // TODO: Remove induces_compute_shader() and replace it with precise tracking.
+       if (has_dummy_effect && !phases[phases.size() - 2]->is_compute_shader) {
+               resource_pool->release_glsl_program(phases.back()->glsl_program_num);
+               delete phases.back();
+               phases.pop_back();
+               has_dummy_effect = false;
+       }
+
+       output_dot("step22-dummy-phase-removal.dot");
+
        assert(phases[0]->inputs.empty());
        
        finalized = true;
@@ -2006,6 +2025,7 @@ void EffectChain::render(GLuint dest_fbo, const vector<DestinationTexture> &dest
                assert(y == 0);
                assert(num_phases >= 2);
                assert(!phases.back()->is_compute_shader);
+               assert(phases[phases.size() - 2]->is_compute_shader);
                assert(phases.back()->effects.size() == 1);
                assert(phases.back()->effects[0]->effect->effect_type_id() == "ComputeShaderOutputDisplayEffect");
 
@@ -2032,6 +2052,7 @@ void EffectChain::render(GLuint dest_fbo, const vector<DestinationTexture> &dest
                if (last_phase) {
                        // Last phase goes to the output the user specified.
                        if (!phase->is_compute_shader) {
+                               assert(dest_fbo != (GLuint)-1);
                                glBindFramebuffer(GL_FRAMEBUFFER, dest_fbo);
                                check_error();
                                GLenum status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER_EXT);
index aa574fc..c06622d 100644 (file)
@@ -1885,4 +1885,44 @@ TEST(ComputeShaderTest, BounceTextureFromTwoComputeShaders) {
        expect_equal(expected_data, out_data, 2, 1);
 }
 
+// Requires mipmaps, but is otherwise like IdentityEffect.
+class MipmapNeedingIdentityEffect : public IdentityEffect {
+public:
+       MipmapNeedingIdentityEffect() {}
+       MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; }
+       string effect_type_id() const override { return "MipmapNeedingIdentityEffect"; }
+       bool strong_one_to_one_sampling() const override { return true; }
+};
+
+TEST(ComputeShaderTest, StrongOneToOneButStillNotChained) {
+       float data[] = {
+               0.0f, 0.25f, 0.3f, 0.8f,
+               0.75f, 1.0f, 1.0f, 0.2f,
+       };
+       float out_data[8];
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       EffectChainTester tester(nullptr, 4, 2);
+
+       FlatInput *input1 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input1->set_pixel_data(data);
+       tester.get_chain()->add_input(input1);
+       Effect *compute_effect = tester.get_chain()->add_effect(new IdentityComputeEffect());
+
+       FlatInput *input2 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input2->set_pixel_data(data);
+       tester.get_chain()->add_input(input2);
+
+       // Not chained with the compute shader because MipmapNeedingIdentityEffect comes in
+       // the same phase, and compute shaders cannot supply mipmaps.
+       tester.get_chain()->add_effect(new StrongOneToOneAddEffect(), compute_effect, input2);
+       tester.get_chain()->add_effect(new MipmapNeedingIdentityEffect());
+
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(data, out_data, 4, 2);
+}
+
 }  // namespace movit