Break phases when a node needs both to supply mipmaps and _not_ supply mipmaps.
authorSteinar H. Gunderson <sgunderson@bigfoot.com>
Thu, 18 Jan 2018 00:07:26 +0000 (01:07 +0100)
committerSteinar H. Gunderson <sgunderson@bigfoot.com>
Thu, 18 Jan 2018 00:16:27 +0000 (01:16 +0100)
In most cases, this means that we can keep a local copy that supplies
mipmaps and an RTT input that doesn't, or the other way around.

This is not a complete solution, since it doesn't take into account
the case where _both_ inputs will be RTT, but it's good enough for now.

Also requires us to distinguish between links between nodes in the same phase
and in different phases, or we'd get GLSL compile errors as we had both the
local and the RTT input being called e.g. “in0”.

effect_chain.cpp
effect_chain.h
effect_chain_test.cpp

index 95bf1c0..c20f769 100644 (file)
@@ -376,7 +376,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
                Node *input = phase->inputs[i]->output_node;
                char effect_id[256];
                sprintf(effect_id, "in%u", i);
-               phase->effect_ids.insert(make_pair(input, effect_id));
+               phase->effect_ids.insert(make_pair(make_pair(input, IN_ANOTHER_PHASE), effect_id));
        
                frag_shader += string("uniform sampler2D tex_") + effect_id + ";\n";
                frag_shader += string("vec4 ") + effect_id + "(vec2 tc) {\n";
@@ -405,26 +405,29 @@ void EffectChain::compile_glsl_program(Phase *phase)
                Node *node = phase->effects[i];
                char effect_id[256];
                sprintf(effect_id, "eff%u", i);
-               phase->effect_ids.insert(make_pair(node, effect_id));
+               bool inserted = phase->effect_ids.insert(make_pair(make_pair(node, IN_SAME_PHASE), effect_id)).second;
+               assert(inserted);
        }
 
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
-               const string effect_id = phase->effect_ids[node];
+               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");
                        } else {
-                               frag_shader += string("#define INPUT ") + phase->effect_ids[input] + "\n";
+                               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];
-                               sprintf(buf, "#define INPUT%d %s\n", j + 1, phase->effect_ids[node->incoming_links[j]].c_str());
+                               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());
                                frag_shader += buf;
                        }
                }
@@ -448,15 +451,15 @@ void EffectChain::compile_glsl_program(Phase *phase)
                frag_shader += "\n";
        }
        if (phase->is_compute_shader) {
-               frag_shader += string("#define INPUT ") + phase->effect_ids[phase->compute_shader_node] + "\n";
+               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 += "#define CS_POSTPROC(tc) CS_OUTPUT_VAL\n";
                } else {
-                       frag_shader += string("#define CS_POSTPROC ") + phase->effect_ids[phase->effects.back()] + "\n";
+                       frag_shader += string("#define CS_POSTPROC ") + phase->effect_ids[make_pair(phase->effects.back(), IN_SAME_PHASE)] + "\n";
                }
        } else {
-               frag_shader += string("#define INPUT ") + phase->effect_ids[phase->effects.back()] + "\n";
+               frag_shader += string("#define INPUT ") + phase->effect_ids[make_pair(phase->effects.back(), IN_SAME_PHASE)] + "\n";
        }
 
        // If we're the last phase, add the right #defines for Y'CbCr multi-output as needed.
@@ -540,7 +543,7 @@ void EffectChain::compile_glsl_program(Phase *phase)
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
                Effect *effect = node->effect;
-               const string effect_id = phase->effect_ids[node];
+               const string effect_id = phase->effect_ids[make_pair(node, IN_SAME_PHASE)];
                extract_uniform_declarations(effect->uniforms_image2d, "image2D", effect_id, &phase->uniforms_image2d, &frag_shader_uniforms);
                extract_uniform_declarations(effect->uniforms_sampler2d, "sampler2D", effect_id, &phase->uniforms_sampler2d, &frag_shader_uniforms);
                extract_uniform_declarations(effect->uniforms_bool, "bool", effect_id, &phase->uniforms_bool, &frag_shader_uniforms);
@@ -699,12 +702,27 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                        // because it needs information about where the phases end
                        // (we should not propagate the flag across phases).
                        if (node->needs_mipmaps != Effect::DOES_NOT_NEED_MIPMAPS) {
-                               if (deps[i]->effect->num_inputs() == 0 && node->needs_mipmaps == Effect::NEEDS_MIPMAPS) {
-                                       Input *input = static_cast<Input *>(deps[i]->effect);
-                                       start_new_phase |= !input->can_supply_mipmaps();
-                               } else if (deps[i]->effect->needs_mipmaps() == Effect::DOES_NOT_NEED_MIPMAPS) {
+                               // The node can have a value set (ie. not DOES_NOT_NEED_MIPMAPS)
+                               // if we have diamonds in the graph; if so, choose that.
+                               // If not, the effect on the node can also decide (this is the
+                               // more common case).
+                               Effect::MipmapRequirements dep_mipmaps = deps[i]->needs_mipmaps;
+                               if (dep_mipmaps == Effect::DOES_NOT_NEED_MIPMAPS) {
+                                       if (deps[i]->effect->num_inputs() == 0) {
+                                               Input *input = static_cast<Input *>(deps[i]->effect);
+                                               dep_mipmaps = input->can_supply_mipmaps() ? Effect::DOES_NOT_NEED_MIPMAPS : Effect::CANNOT_ACCEPT_MIPMAPS;
+                                       } else {
+                                               dep_mipmaps = deps[i]->effect->needs_mipmaps();
+                                       }
+                               }
+                               if (dep_mipmaps == Effect::DOES_NOT_NEED_MIPMAPS) {
                                        deps[i]->needs_mipmaps = node->needs_mipmaps;
-                               } else if (deps[i]->effect->needs_mipmaps() != node->needs_mipmaps) {
+                               } else if (dep_mipmaps != node->needs_mipmaps) {
+                                       // The dependency cannot supply our mipmap demands
+                                       // (either because it's an input that can't do mipmaps,
+                                       // or because there's a conflict between mipmap-needing
+                                       // and mipmap-refusing effects somewhere in the graph),
+                                       // so they cannot be in the same phase.
                                        start_new_phase = true;
                                }
                        }
@@ -764,6 +782,8 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                                deps[i]->strong_one_to_one_sampling = node->strong_one_to_one_sampling &&
                                        deps[i]->effect->strong_one_to_one_sampling();
                        }
+
+                       node->incoming_link_type.push_back(start_new_phase ? IN_ANOTHER_PHASE : IN_SAME_PHASE);
                }
        }
 
@@ -791,19 +811,13 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
        phase->effects = topological_sort(phase->effects);
 
        // Figure out if we need mipmaps or not, and if so, tell the inputs that.
-       phase->input_needs_mipmaps = false;
-       for (unsigned i = 0; i < phase->effects.size(); ++i) {
-               Node *node = phase->effects[i];
-               if (node->effect->needs_mipmaps() == Effect::NEEDS_MIPMAPS) {
-                       phase->input_needs_mipmaps = true;
-               }
-       }
+       // (RTT inputs have different logic, which is checked in execute_phase().)
        for (unsigned i = 0; i < phase->effects.size(); ++i) {
                Node *node = phase->effects[i];
                if (node->effect->num_inputs() == 0) {
                        Input *input = static_cast<Input *>(node->effect);
-                       assert(!phase->input_needs_mipmaps || input->can_supply_mipmaps());
-                       CHECK(input->set_int("needs_mipmaps", phase->input_needs_mipmaps));
+                       assert(node->needs_mipmaps != Effect::NEEDS_MIPMAPS || input->can_supply_mipmaps());
+                       CHECK(input->set_int("needs_mipmaps", node->needs_mipmaps == Effect::NEEDS_MIPMAPS));
                }
        }
 
@@ -2131,7 +2145,7 @@ void EffectChain::print_phase_timing()
 
 void EffectChain::execute_phase(Phase *phase,
                                 const map<Phase *, GLuint> &output_textures,
-                                const std::vector<DestinationTexture> &destinations,
+                                const vector<DestinationTexture> &destinations,
                                 set<Phase *> *generated_mipmaps)
 {
        // Set up RTT inputs for this phase.
@@ -2143,12 +2157,36 @@ void EffectChain::execute_phase(Phase *phase,
                assert(it != output_textures.end());
                glBindTexture(GL_TEXTURE_2D, it->second);
                check_error();
-               if (phase->input_needs_mipmaps && generated_mipmaps->count(input) == 0) {
+
+               // See if anything using this RTT input (in this phase) needs mipmaps.
+               // TODO: It could be that we get conflicting logic here, if we have
+               // multiple effects with incompatible mipmaps using the same
+               // RTT input. However, that is obscure enough that we can deal
+               // with it at some future point (preferably when we have
+               // universal support for separate sampler objects!). For now,
+               // an assert is good enough. See also the TODO at bound_sampler_num.
+               bool any_needs_mipmaps = false, any_refuses_mipmaps = false;
+               for (Node *node : phase->effects) {
+                       assert(node->incoming_links.size() == node->incoming_link_type.size());
+                       for (size_t i = 0; i < node->incoming_links.size(); ++i) {
+                               if (node->incoming_links[i] == input->output_node &&
+                                   node->incoming_link_type[i] == IN_ANOTHER_PHASE) {
+                                       if (node->needs_mipmaps == Effect::NEEDS_MIPMAPS) {
+                                               any_needs_mipmaps = true;
+                                       } else if (node->needs_mipmaps == Effect::CANNOT_ACCEPT_MIPMAPS) {
+                                               any_refuses_mipmaps = true;
+                                       }
+                               }
+                       }
+               }
+               assert(!(any_needs_mipmaps && any_refuses_mipmaps));
+
+               if (any_needs_mipmaps && generated_mipmaps->count(input) == 0) {
                        glGenerateMipmap(GL_TEXTURE_2D);
                        check_error();
                        generated_mipmaps->insert(input);
                }
-               setup_rtt_sampler(sampler, phase->input_needs_mipmaps);
+               setup_rtt_sampler(sampler, any_needs_mipmaps);
                phase->input_samplers[sampler] = sampler;  // Bind the sampler to the right uniform.
        }
 
@@ -2183,7 +2221,7 @@ void EffectChain::execute_phase(Phase *phase,
        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(instance_program_num, phase->effect_ids[node], &sampler_num);
+               node->effect->set_gl_state(instance_program_num, phase->effect_ids[make_pair(node, IN_SAME_PHASE)], &sampler_num);
                check_error();
 
                if (node->effect->is_single_texture()) {
index a36840d..09b9aed 100644 (file)
@@ -112,6 +112,12 @@ enum FramebufferTransformation {
        SQUARE_ROOT_FRAMEBUFFER_TRANSFORMATION,
 };
 
+// Whether a link is into another phase or not; see Node::incoming_link_type.
+enum NodeLinkType {
+       IN_ANOTHER_PHASE,
+       IN_SAME_PHASE
+};
+
 // A node in the graph; basically an effect and some associated information.
 class Node {
 public:
@@ -143,11 +149,21 @@ private:
        // sampler state here.
        int bound_sampler_num;
 
+       // For each node in incoming_links, whether it comes from another phase
+       // or not. This is required because in some rather obscure cases,
+       // it is possible to have an input twice in the same phase; both by
+       // itself and as a bounced input.
+       //
+       // TODO: It is possible that we might even need to bounce multiple
+       // times and thus disambiguate also between different external phases,
+       // but we'll deal with that when we need to care about it, if ever.
+       std::vector<NodeLinkType> incoming_link_type;
+
        // Used during the building of the effect chain.
        Colorspace output_color_space;
        GammaCurve output_gamma_curve;
        AlphaType output_alpha_type;
-       bool needs_mipmaps;  // Directly or indirectly.
+       Effect::MipmapRequirements needs_mipmaps;  // Directly or indirectly.
 
        // Set if this effect, and all effects consuming output from this node
        // (in the same phase) have one_to_one_sampling() set.
@@ -169,8 +185,6 @@ struct Phase {
        // which is which, because they contain the same data.
        std::set<GLint> attribute_indexes;
 
-       bool input_needs_mipmaps;
-
        // Inputs are only inputs from other phases (ie., those that come from RTT);
        // input textures are counted as part of <effects>.
        std::vector<Phase *> inputs;
@@ -197,7 +211,7 @@ struct Phase {
 
        // Identifier used to create unique variables in GLSL.
        // Unique per-phase to increase cacheability of compiled shaders.
-       std::map<Node *, std::string> effect_ids;
+       std::map<std::pair<Node *, NodeLinkType>, std::string> effect_ids;
 
        // Uniforms for this phase; combined from all the effects.
        std::vector<Uniform<int>> uniforms_image2d;
index ecf0b69..f51f988 100644 (file)
@@ -826,17 +826,17 @@ TEST(EffectChainTest, MipmapChainGetsSplit) {
        float out_data[4 * 4];
 
        float offset[] = { -0.5f / 4.0f, -0.5f / 4.0f };
-       RewritingEffect<Downscale2xEffect> *pick_out_top_left = new RewritingEffect<Downscale2xEffect>(Effect::CANNOT_ACCEPT_MIPMAPS);
-       ASSERT_TRUE(pick_out_top_left->effect->set_vec2("offset", offset));
+       RewritingEffect<Downscale2xEffect> *pick_out_bottom_left = new RewritingEffect<Downscale2xEffect>(Effect::CANNOT_ACCEPT_MIPMAPS);
+       ASSERT_TRUE(pick_out_bottom_left->effect->set_vec2("offset", offset));
 
        RewritingEffect<Downscale2xEffect> *downscale2x = new RewritingEffect<Downscale2xEffect>(Effect::NEEDS_MIPMAPS);
 
        EffectChainTester tester(data, 4, 4, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_LINEAR);
-       tester.get_chain()->add_effect(pick_out_top_left);
+       tester.get_chain()->add_effect(pick_out_bottom_left);
        tester.get_chain()->add_effect(downscale2x);
        tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
 
-       EXPECT_NE(pick_out_top_left->replaced_node->containing_phase,
+       EXPECT_NE(pick_out_bottom_left->replaced_node->containing_phase,
                  downscale2x->replaced_node->containing_phase);
 
        expect_equal(expected_data, out_data, 4, 4);
@@ -951,6 +951,78 @@ TEST(EffectChainTest, DiamondGraphWithOneInputUsedInTwoPhases) {
        expect_equal(expected_data, out_data, 2, 2);
 }
 
+// Constructs the graph
+//
+//                        FlatInput                               |
+//                       /         \                              |
+//  Downscale2xEffect (mipmaps)  Downscale2xEffect (no mipmaps)   |
+//                      |           |                             |
+//  Downscale2xEffect (mipmaps)  Downscale2xEffect (no mipmaps)   |
+//                       \         /                              |
+//                        AddEffect                               |
+//
+// and verifies that it gives the correct output. Due to the conflicting
+// mipmap demands, EffectChain needs to make two phases; exactly where it's
+// split is less important, though (this is a fairly obscure situation that
+// is unlikely to happen in practice).
+TEST(EffectChainTest, DiamondGraphWithConflictingMipmaps) {
+       float data[] = {
+               0.0f, 0.0f, 0.0f, 0.0f,
+               1.0f, 0.0f, 1.0f, 0.0f,
+               0.0f, 0.0f, 0.0f, 0.0f,
+               1.0f, 0.0f, 1.0f, 0.0f,
+       };
+
+       // Same situation as MipmapChainGetsSplit. The output of the two
+       // downscales with no mipmaps looks like this:
+       //
+       //    0 0 0 0
+       //    0 0 0 0
+       //    0 0 0 0
+       //    1 0 0 0
+       //
+       // and the one with mipmaps is 0.25 everywhere. Due to postmultiplied
+       // alpha, we get the average even though we are using AddEffect.
+       float expected_data[] = {
+               0.125f, 0.125f, 0.125f, 0.125f,
+               0.125f, 0.125f, 0.125f, 0.125f,
+               0.125f, 0.125f, 0.125f, 0.125f,
+               0.625f, 0.125f, 0.125f, 0.125f,
+       };
+       float out_data[4 * 4];
+
+       float offset[] = { -0.5f / 4.0f, -0.5f / 4.0f };
+       Downscale2xEffect *nomipmap1 = new Downscale2xEffect(Effect::CANNOT_ACCEPT_MIPMAPS);
+       Downscale2xEffect *nomipmap2 = new Downscale2xEffect(Effect::CANNOT_ACCEPT_MIPMAPS);
+       ASSERT_TRUE(nomipmap1->set_vec2("offset", offset));
+       ASSERT_TRUE(nomipmap2->set_vec2("offset", offset));
+
+       Downscale2xEffect *mipmap1 = new Downscale2xEffect(Effect::NEEDS_MIPMAPS);
+       Downscale2xEffect *mipmap2 = new Downscale2xEffect(Effect::NEEDS_MIPMAPS);
+
+       EffectChainTester tester(nullptr, 4, 4);
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       FlatInput *input = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 4);
+       input->set_pixel_data(data);
+
+       tester.get_chain()->add_input(input);
+
+       tester.get_chain()->add_effect(nomipmap1, input);
+       tester.get_chain()->add_effect(nomipmap2, nomipmap1);
+
+       tester.get_chain()->add_effect(mipmap1, input);
+       tester.get_chain()->add_effect(mipmap2, mipmap1);
+
+       tester.get_chain()->add_effect(new AddEffect(), nomipmap2, mipmap2);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+       expect_equal(expected_data, out_data, 4, 4);
+}
+
 TEST(EffectChainTest, EffectUsedTwiceOnlyGetsOneGammaConversion) {
        float data[] = {
                0.735f, 0.0f,