From c79d94d05d560fa59b23d542bca6d7a94b907b6f Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Mon, 27 May 2019 23:47:48 +0200 Subject: [PATCH] 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. --- effect_chain.cpp | 7 +++++++ effect_chain_test.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/effect_chain.cpp b/effect_chain.cpp index deeea91..0775042 100644 --- a/effect_chain.cpp +++ b/effect_chain.cpp @@ -698,6 +698,8 @@ Phase *EffectChain::construct_phase(Node *output, map *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 *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 6f930da..1bcaf72 100644 --- 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 *identity = new RewritingEffect(); + + RewritingEffect *downscale = new RewritingEffect(); // 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 -- 2.39.2