From 6e03d8c6dec4b4639137893fe4e1aff224836f59 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Sun, 20 Jan 2013 00:31:23 +0100 Subject: [PATCH] Optimize away duplicate conversion nodes. Sometimes an effect can be used by two other effects that both demand the same conversion. If so, we should simply insert a conversion after that effect and connect _all_ outputs to that conversion, since conversions to linear/sRGB/premultiplied never hurt for us. Add unit tests for the gamma and colorspace cases. I could have added for the alpha case, too, but it got very tedious. :-) --- effect_chain.cpp | 13 +++++---- effect_chain_test.cpp | 66 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/effect_chain.cpp b/effect_chain.cpp index 8767f7e..a8c2e52 100644 --- a/effect_chain.cpp +++ b/effect_chain.cpp @@ -947,7 +947,7 @@ void EffectChain::fix_internal_color_spaces() } // Go through each input that is not sRGB, and insert - // a colorspace conversion before it. + // a colorspace conversion after it. for (unsigned j = 0; j < node->incoming_links.size(); ++j) { Node *input = node->incoming_links[j]; assert(input->output_color_space != COLORSPACE_INVALID); @@ -958,7 +958,8 @@ void EffectChain::fix_internal_color_spaces() CHECK(conversion->effect->set_int("source_space", input->output_color_space)); CHECK(conversion->effect->set_int("destination_space", COLORSPACE_sRGB)); conversion->output_color_space = COLORSPACE_sRGB; - insert_node_between(input, conversion, node); + replace_sender(input, conversion); + connect_nodes(input, conversion); } // Re-sort topologically, and propagate the new information. @@ -1038,7 +1039,8 @@ void EffectChain::fix_internal_alpha(unsigned step) conversion = add_node(new AlphaDivisionEffect()); } conversion->output_alpha_type = desired_type; - insert_node_between(input, conversion, node); + replace_sender(input, conversion); + connect_nodes(input, conversion); } // Re-sort topologically, and propagate the new information. @@ -1221,7 +1223,7 @@ void EffectChain::fix_internal_gamma_by_inserting_nodes(unsigned step) } // If not, go through each input that is not linear gamma, - // and insert a gamma conversion before it. + // and insert a gamma conversion after it. for (unsigned j = 0; j < node->incoming_links.size(); ++j) { Node *input = node->incoming_links[j]; assert(input->output_gamma_curve != GAMMA_INVALID); @@ -1231,7 +1233,8 @@ void EffectChain::fix_internal_gamma_by_inserting_nodes(unsigned step) Node *conversion = add_node(new GammaExpansionEffect()); CHECK(conversion->effect->set_int("source_curve", input->output_gamma_curve)); conversion->output_gamma_curve = GAMMA_LINEAR; - insert_node_between(input, conversion, node); + replace_sender(input, conversion); + connect_nodes(input, conversion); } // Re-sort topologically, and propagate the new information. diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp index 7549103..614bffc 100644 --- a/effect_chain_test.cpp +++ b/effect_chain_test.cpp @@ -690,3 +690,69 @@ TEST(EffectChainTest, DiamondGraphWithOneInputUsedInTwoPhases) { expect_equal(expected_data, out_data, 2, 2); } + +TEST(EffectChainTest, EffectUsedTwiceOnlyGetsOneGammaConversion) { + float data[] = { + 0.735f, 0.0f, + 0.735f, 0.0f, + }; + float expected_data[] = { + 0.0f, 0.5f, // 0.5 and not 1.0, since AddEffect doesn't clamp alpha properly. + 0.0f, 0.5f, + }; + float out_data[2 * 2]; + + EffectChainTester tester(NULL, 2, 2); + tester.add_input(data, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_sRGB); + + // MirrorEffect does not get linear light, so the conversions will be + // inserted after it, not before. + RewritingEffect *effect = new RewritingEffect(); + tester.get_chain()->add_effect(effect); + + Effect *identity1 = tester.get_chain()->add_effect(new IdentityEffect(), effect); + Effect *identity2 = tester.get_chain()->add_effect(new IdentityEffect(), effect); + 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, 2); + + Node *node = effect->replaced_node; + ASSERT_EQ(1, node->incoming_links.size()); + ASSERT_EQ(1, node->outgoing_links.size()); + EXPECT_EQ("FlatInput", node->incoming_links[0]->effect->effect_type_id()); + EXPECT_EQ("GammaExpansionEffect", node->outgoing_links[0]->effect->effect_type_id()); +} + +TEST(EffectChainTest, EffectUsedTwiceOnlyGetsOneColorspaceConversion) { + float data[] = { + 0.5f, 0.0f, + 0.5f, 0.0f, + }; + float expected_data[] = { + 0.0f, 0.5f, // 0.5 and not 1.0, since AddEffect doesn't clamp alpha properly. + 0.0f, 0.5f, + }; + float out_data[2 * 2]; + + EffectChainTester tester(NULL, 2, 2); + tester.add_input(data, FORMAT_GRAYSCALE, COLORSPACE_REC_601_625, GAMMA_LINEAR); + + // MirrorEffect does not get linear light, so the conversions will be + // inserted after it, not before. + RewritingEffect *effect = new RewritingEffect(); + tester.get_chain()->add_effect(effect); + + Effect *identity1 = tester.get_chain()->add_effect(new IdentityEffect(), effect); + Effect *identity2 = tester.get_chain()->add_effect(new IdentityEffect(), effect); + 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, 2); + + Node *node = effect->replaced_node; + ASSERT_EQ(1, node->incoming_links.size()); + ASSERT_EQ(1, node->outgoing_links.size()); + EXPECT_EQ("FlatInput", node->incoming_links[0]->effect->effect_type_id()); + EXPECT_EQ("ColorspaceConversionEffect", node->outgoing_links[0]->effect->effect_type_id()); +} -- 2.39.2