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. :-)

diff --git a/effect_chain.cpp b/effect_chain.cpp
index 8767f7e28a2bdcfa757ae45e8b9ca3bd9f0a6570..a8c2e52c564679872ad92d31c48f391a45e699be 100644 (file)
--- 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
                        }
 
                        // 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);
                        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;
                                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.
                        }
 
                        // 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;
                                        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.
                        }
 
                        // 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,
                        }
 
                        // 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);
                        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;
                                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.
                        }
 
                        // Re-sort topologically, and propagate the new information.

diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index 75491030b05b66f27fef22a17ca8e5f7ccf66dcd..614bffc3dd5ecfe08143062af0ebda767ffb9970 100644 (file)
--- 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);
 }
 
        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<MirrorEffect> *effect = new RewritingEffect<MirrorEffect>();
+       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<MirrorEffect> *effect = new RewritingEffect<MirrorEffect>();
+       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());
+}