Loosen up some restrictions on strong one-to-one-effects.

In particular, this allows strong one-to-one-effects with multiple
inputs; this was forbidden by the comment, but not enforced anywhere,
and MixEffect was inadvertedly put as strong one-to-one. This actually
triggered at least three distinct bugs (thus three new tests).

diff --git a/effect.h b/effect.h
index 79e4020fc0fa1bd3f188ba2aa2b5fd1f131a88c5..1dab990e75738664e685742d6b77390f7e80b9bf 100644 (file)
--- a/effect.h
+++ b/effect.h
@@ -225,9 +225,9 @@ public:
        virtual bool one_to_one_sampling() const { return strong_one_to_one_sampling(); }
 
        // Similar in use to one_to_one_sampling(), but even stricter:
        virtual bool one_to_one_sampling() const { return strong_one_to_one_sampling(); }
 
        // Similar in use to one_to_one_sampling(), but even stricter:

-       // The effect must not use texture coordinate in any way beyond
-       // giving it unmodified to its (single) input. This allows it to
-       // also be used after a compute shader, in the same phase.
+       // The effect must modify texture coordinate in any way when
+       // calling its input(s). This allows it to also be used after
+       // a compute shader, in the same phase.

        //
        // An effect that it strong one-to-one must also be one-to-one.
        virtual bool strong_one_to_one_sampling() const { return false; }
        //
        // An effect that it strong one-to-one must also be one-to-one.
        virtual bool strong_one_to_one_sampling() const { return false; }

diff --git a/effect_chain.cpp b/effect_chain.cpp
index 5cbb7f9208ad9a375df716d23820f1dd8deb87a0..6d6886cc0d881b95a4ac9972e2b4629d406a385c 100644 (file)
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -412,24 +412,28 @@ void EffectChain::compile_glsl_program(Phase *phase)
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
                const string effect_id = phase->effect_ids[make_pair(node, IN_SAME_PHASE)];
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
                const string effect_id = phase->effect_ids[make_pair(node, IN_SAME_PHASE)];

-               if (node->incoming_links.size() == 1) {
-                       Node *input = node->incoming_links[0];
-                       NodeLinkType link_type = node->incoming_link_type[0];
-                       if (i != 0 && input->effect->is_compute_shader()) {
-                               // First effect after the compute shader reads the value
-                               // that cs_output() wrote to a global variable.
-                               frag_shader += string("#define INPUT(tc) CS_OUTPUT_VAL\n");
+               for (unsigned j = 0; j < node->incoming_links.size(); ++j) {
+                       if (node->incoming_links.size() == 1) {
+                               frag_shader += "#define INPUT";

                        } else {
                        } else {

-                               frag_shader += string("#define INPUT ") + phase->effect_ids[make_pair(input, link_type)] + "\n";
-                       }
-               } else {
-                       for (unsigned j = 0; j < node->incoming_links.size(); ++j) {
-                               assert(!node->incoming_links[j]->effect->is_compute_shader());

                                char buf[256];
                                char buf[256];

-                               string effect_id = phase->effect_ids[make_pair(node->incoming_links[j], node->incoming_link_type[j])];
-                               sprintf(buf, "#define INPUT%d %s\n", j + 1, effect_id.c_str());
+                               sprintf(buf, "#define INPUT%d", j + 1);

                                frag_shader += buf;
                        }
                                frag_shader += buf;
                        }

+
+                       Node *input = node->incoming_links[j];
+                       NodeLinkType link_type = node->incoming_link_type[j];
+                       if (i != 0 &&
+                           input->effect->is_compute_shader() &&
+                           node->incoming_link_type[j] == IN_SAME_PHASE) {
+                               // First effect after the compute shader reads the value
+                               // that cs_output() wrote to a global variable,
+                               // ignoring the tc (since all such effects have to be
+                               // strong one-to-one).
+                               frag_shader += "(tc) CS_OUTPUT_VAL\n";
+                       } else {
+                               frag_shader += string(" ") + phase->effect_ids[make_pair(input, link_type)] + "\n";
+                       }

                }
        
                frag_shader += "\n";
                }
        
                frag_shader += "\n";


@@ -679,6 +683,8 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
 
                phase->effects.push_back(node);
                if (node->effect->is_compute_shader()) {
 
                phase->effects.push_back(node);
                if (node->effect->is_compute_shader()) {

+                       assert(phase->compute_shader_node == nullptr ||
+                              phase->compute_shader_node == node);

                        phase->is_compute_shader = true;
                        phase->compute_shader_node = node;
                }
                        phase->is_compute_shader = true;
                        phase->compute_shader_node = node;
                }


@@ -753,13 +759,17 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                        }
 
                        if (deps[i]->effect->is_compute_shader()) {
                        }
 
                        if (deps[i]->effect->is_compute_shader()) {

-                               // Only one compute shader per phase; we should have been stopped
-                               // already due to the fact that compute shaders are not one-to-one.
-                               assert(!phase->is_compute_shader);
-
-                               // 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 = !node->strong_one_to_one_sampling;
+                               if (phase->is_compute_shader) {
+                                       // Only one compute shader per phase.
+                                       start_new_phase = true;
+                               } else if (!node->strong_one_to_one_sampling) {
+                                       // 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 = !node->strong_one_to_one_sampling;
+                               } else {
+                                       phase->is_compute_shader = true;
+                                       phase->compute_shader_node = deps[i];
+                               }

                        } else if (deps[i]->effect->sets_virtual_output_size()) {
                                assert(deps[i]->effect->changes_output_size());
                                // If the next effect sets a virtual size to rely on OpenGL's
                        } else if (deps[i]->effect->sets_virtual_output_size()) {
                                assert(deps[i]->effect->changes_output_size());
                                // If the next effect sets a virtual size to rely on OpenGL's


@@ -1783,20 +1793,38 @@ void EffectChain::add_dither_if_needed()
        dither_effect = dither->effect;
 }
 
        dither_effect = dither->effect;
 }
 

+namespace {
+
+// Whether this effect will cause the phase it is in to become a compute shader phase.
+bool induces_compute_shader(Node *node)
+{
+       if (node->effect->is_compute_shader()) {
+               return true;
+       }
+       if (!node->effect->strong_one_to_one_sampling()) {
+               // This effect can't be chained after a compute shader.
+               return false;
+       }
+       // If at least one of the effects we depend on is a compute shader,
+       // one of them will be put in the same phase as us (the other ones,
+       // if any, will be bounced).
+       for (Node *dep : node->incoming_links) {
+               if (induces_compute_shader(dep)) {
+                       return true;
+               }
+       }
+       return false;
+}
+
+}  // namespace
+

 // Compute shaders can't output to the framebuffer, so if the last
 // phase ends in a compute shader, add a dummy phase at the end that
 // only blits directly from the temporary texture.
 void EffectChain::add_dummy_effect_if_needed()
 {
        Node *output = find_output_node();
 // Compute shaders can't output to the framebuffer, so if the last
 // phase ends in a compute shader, add a dummy phase at the end that
 // only blits directly from the temporary texture.
 void EffectChain::add_dummy_effect_if_needed()
 {
        Node *output = find_output_node();

-
-       // See if the last effect that's not strong one-to-one is a compute shader.
-       Node *last_effect = output;
-       while (last_effect->effect->num_inputs() == 1 &&
-              last_effect->effect->strong_one_to_one_sampling()) {
-               last_effect = last_effect->incoming_links[0];
-       }
-       if (last_effect->effect->is_compute_shader()) {
+       if (induces_compute_shader(output)) {

                Node *dummy = add_node(new ComputeShaderOutputDisplayEffect());
                connect_nodes(output, dummy);
                has_dummy_effect = true;
                Node *dummy = add_node(new ComputeShaderOutputDisplayEffect());
                connect_nodes(output, dummy);
                has_dummy_effect = true;

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index f51f988bdad220a3b4c259ec55a9eaea477bc844..aa574fca795ae364477ef1bbd56aec58dc18655c 100644 (file)
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -1776,4 +1776,113 @@ TEST(ComputeShaderTest, ResizingComputeThenOneToOne) {
        EXPECT_EQ(1u, phase->output_height);
 }
 
        EXPECT_EQ(1u, phase->output_height);
 }
 

+class StrongOneToOneAddEffect : public AddEffect {
+public:
+       StrongOneToOneAddEffect() {}
+       string effect_type_id() const override { return "StrongOneToOneAddEffect"; }
+       bool strong_one_to_one_sampling() const override { return true; }
+};
+
+TEST(ComputeShaderTest, NoTwoComputeInSamePhase) {
+       float data[] = {
+               0.0f, 0.25f, 0.3f, 0.8f,
+               0.75f, 1.0f, 1.0f, 0.2f,
+       };
+       float expected_data[] = {
+               0.0f, 0.3f,
+       };
+       float out_data[2];
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       EffectChainTester tester(nullptr, 2, 1);
+
+       FlatInput *input1 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input1->set_pixel_data(data);
+       tester.get_chain()->add_input(input1);
+       Effect *downscale1 = tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+
+       FlatInput *input2 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input2->set_pixel_data(data);
+       tester.get_chain()->add_input(input2);
+       Effect *downscale2 = tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+
+       tester.get_chain()->add_effect(new StrongOneToOneAddEffect(), downscale1, downscale2);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(expected_data, out_data, 2, 1);
+}
+
+// Like the previous test, but the adder effect is not directly connected
+// to the compute shaders (so the status has to be propagated through those effects).
+TEST(ComputeShaderTest, NoTwoComputeInSamePhaseIndirect) {
+       float data[] = {
+               0.0f, 0.25f, 0.3f, 0.8f,
+               0.75f, 1.0f, 1.0f, 0.2f,
+       };
+       float expected_data[] = {
+               0.0f, 0.3f,
+       };
+       float out_data[2];
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       EffectChainTester tester(nullptr, 2, 1);
+
+       FlatInput *input1 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input1->set_pixel_data(data);
+       tester.get_chain()->add_input(input1);
+       tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+       Effect *identity1 = tester.get_chain()->add_effect(new OneToOneEffect());
+
+       FlatInput *input2 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input2->set_pixel_data(data);
+       tester.get_chain()->add_input(input2);
+       tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+       Effect *identity2 = tester.get_chain()->add_effect(new OneToOneEffect());
+
+       tester.get_chain()->add_effect(new StrongOneToOneAddEffect(), identity1, identity2);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(expected_data, out_data, 2, 1);
+}
+
+// Like the previous test, but the adder is not strong one-to-one
+// (so there are two different compute shader inputs, but none of them
+// are in the same phase).
+TEST(ComputeShaderTest, BounceTextureFromTwoComputeShaders) {
+       float data[] = {
+               0.0f, 0.25f, 0.3f, 0.8f,
+               0.75f, 1.0f, 1.0f, 0.2f,
+       };
+       float expected_data[] = {
+               0.0f, 0.3f,
+       };
+       float out_data[2];
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       EffectChainTester tester(nullptr, 2, 1);
+
+       FlatInput *input1 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input1->set_pixel_data(data);
+       tester.get_chain()->add_input(input1);
+       tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+       Effect *identity1 = tester.get_chain()->add_effect(new OneToOneEffect());
+
+       FlatInput *input2 = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 2);
+       input2->set_pixel_data(data);
+       tester.get_chain()->add_input(input2);
+       tester.get_chain()->add_effect(new Downscale2xComputeEffect());
+       Effect *identity2 = tester.get_chain()->add_effect(new OneToOneEffect());
+
+       tester.get_chain()->add_effect(new AddEffect(), identity1, identity2);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+       expect_equal(expected_data, out_data, 2, 1);
+}
+

 }  // namespace movit
 }  // namespace movit

diff --git a/footer.comp b/footer.comp
index 17175d9c717777665c0ec537d91cc95e6524c835..c59a93de6f3eb271c26fa4d05f99df7a629b357f 100644 (file)
--- a/footer.comp
+++ b/footer.comp
@@ -26,9 +26,12 @@ void cs_output(uvec2 coord, vec4 val)
 
 void cs_output(ivec2 coord, vec4 val)
 {
 
 void cs_output(ivec2 coord, vec4 val)
 {

-       // Run the value through any preprocessing steps we might have.
+       // Run the value through any postprocessing steps we might have.
+       // Note that we need to give in the actual coordinates, since the
+       // effect could have multiple (non-compute) inputs, and would also
+       // be allowed to make effects based on the texture coordinate alone.

        CS_OUTPUT_VAL = val;
        CS_OUTPUT_VAL = val;

-       val = CS_POSTPROC(vec2(0.0, 0.0));
+       val = CS_POSTPROC(NORMALIZE_TEXTURE_COORDS(coord));

 
 #if SQUARE_ROOT_TRANSFORMATION
        // Make sure we don't give negative values to sqrt.
 
 #if SQUARE_ROOT_TRANSFORMATION
        // Make sure we don't give negative values to sqrt.