Fix an issue where we'd add an unneeded bounce for mipmaps in some cases.

Specifically, if we didn't need to bounce for mipmaps, but bounced for
some other reason (e.g. resizing), we'd still propagate the mipmap
requirements. This could lead to yet another bounce earlier up the chain.
A typical case would be YCbCrInput -> GammaExpansionEffect -> ResizeEffect,
where we'd have a completely unneccessary bounce between the input and the
gamma expansion.

diff --git a/effect_chain.cpp b/effect_chain.cpp
index deeea9187f719b1ca3e0f5f340197fab8b2e59c5..0775042c5ccb114c2f0ed2e4bf7d1a023e061ec2 100644 (file)
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -698,6 +698,8 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                for (unsigned i = 0; i < deps.size(); ++i) {
                        bool start_new_phase = false;
 
+                       Effect::MipmapRequirements save_needs_mipmaps = deps[i]->needs_mipmaps;
+
                        if (node->effect->needs_texture_bounce() &&
                            !deps[i]->effect->is_single_texture() &&
                            !deps[i]->effect->override_disable_bounce()) {
@@ -785,6 +787,11 @@ Phase *EffectChain::construct_phase(Node *output, map<Node *, Phase *> *complete
                        }
 
                        if (start_new_phase) {
+                               // Since we're starting a new phase here, we don't need to impose any
+                               // new demands on this effect. Restore the status we had before we
+                               // started looking at it.
+                               deps[i]->needs_mipmaps = save_needs_mipmaps;
+
                                phase->inputs.push_back(construct_phase(deps[i], completed_effects));
                        } else {
                                effects_todo_this_phase.push(deps[i]);

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index 6f930dafbc642781a9c48863f70175d96e9323a6..1bcaf72a9d9d7f7149c0c83239508d517375e69f 100644 (file)
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -1924,4 +1924,42 @@ TEST(ComputeShaderTest, StrongOneToOneButStillNotChained) {
        expect_equal(data, out_data, 4, 2);
 }
 
+TEST(EffectChainTest, BounceResetsMipmapNeeds) {
+       float data[] = {
+               0.0f, 0.25f,
+               0.75f, 1.0f,
+       };
+       float out_data[1];
+
+       ImageFormat format;
+       format.color_space = COLORSPACE_sRGB;
+       format.gamma_curve = GAMMA_LINEAR;
+
+       NonMipmapCapableInput *input = new NonMipmapCapableInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 2, 2);
+       input->set_pixel_data(data);
+
+       RewritingEffect<IdentityEffect> *identity = new RewritingEffect<IdentityEffect>();
+
+       RewritingEffect<ResizeEffect> *downscale = new RewritingEffect<ResizeEffect>();  // Needs mipmaps.
+       ASSERT_TRUE(downscale->effect->set_int("width", 1));
+       ASSERT_TRUE(downscale->effect->set_int("height", 1));
+
+       EffectChainTester tester(nullptr, 1, 1);
+       tester.get_chain()->add_input(input);
+       tester.get_chain()->add_effect(identity);
+       tester.get_chain()->add_effect(downscale);
+       tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+       Node *input_node = identity->replaced_node->incoming_links[0];
+
+       // The ResizeEffect needs mipmaps. Normally, that would mean that it should
+       // propagate this tatus down through the IdentityEffect. However, since we
+       // bounce (due to the resize), the dependency breaks there, and we don't
+       // need to bounce again between the input and the IdentityEffect.
+       EXPECT_EQ(input_node->containing_phase,
+                 identity->replaced_node->containing_phase);
+       EXPECT_NE(identity->replaced_node->containing_phase,
+                 downscale->replaced_node->containing_phase);
+}
+
 }  // namespace movit