Fix more confusion with strong one-to-one effects and compute shaders.

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.

diff --git a/effect_chain.cpp b/effect_chain.cpp
index 7389efdcb18f14eaee9e6f0b94aefeb44136b154..deeea9187f719b1ca3e0f5f340197fab8b2e59c5 100644 (file)
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -432,6 +432,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
                                // strong one-to-one).
                                frag_shader += "(tc) CS_OUTPUT_VAL\n";
                        } else {
                                // 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";
                        }
                }
                                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) {
                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.
                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 {
                        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";
        }
 
                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;
                                        // 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];
                                }
                                        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");
 
 
        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;
        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(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");
 
                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) {
                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);
                                glBindFramebuffer(GL_FRAMEBUFFER, dest_fbo);
                                check_error();
                                GLenum status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER_EXT);

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index aa574fca795ae364477ef1bbd56aec58dc18655c..c06622d8f0e029d5fa9e9ae42150c356d4f1d1e3 100644 (file)
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -1885,4 +1885,44 @@ TEST(ComputeShaderTest, BounceTextureFromTwoComputeShaders) {
        expect_equal(expected_data, out_data, 2, 1);
 }
 
        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
 }  // namespace movit