Fix an issue where we'd add an unneeded bounce for mipmaps in some cases.
authorSteinar H. Gunderson <sgunderson@bigfoot.com>
Mon, 27 May 2019 21:47:48 +0000 (23:47 +0200)
committerSteinar H. Gunderson <sgunderson@bigfoot.com>
Mon, 27 May 2019 21:49:26 +0000 (23:49 +0200)
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.

effect_chain.cpp
effect_chain_test.cpp

index deeea91..0775042 100644 (file)
@@ -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]);
index 6f930da..1bcaf72 100644 (file)
@@ -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