From: Steinar H. Gunderson Date: Wed, 2 Sep 2015 23:46:58 +0000 (+0200) Subject: Collapse passes more aggressively in the face of size changes. X-Git-Tag: 1.2.0~29 X-Git-Url: https://git.sesse.net/?p=movit;a=commitdiff_plain;h=b564238fa1293c01c77bcabe7b2de267f146ab24 Collapse passes more aggressively in the face of size changes. The motivating chain for this change was a case where we had a SinglePassResampleEffect (the second half of a ResampleEffect) feeding into a PaddingEffect, feeding into an OverlayEffect. Currently, since the two former change output size, we'd bounce to a temporary texture twice (output size changes would always cause bounces). However, this is needlessly conservative. The reason for bouncing when changing output size is really if you want to get rid of data by downscaling and then later upsampling, e.g. for a blur. (It could also be useful for cropping, but we don't really use that right now; PaddingEffect, which does crop, explicitly checks the borders anyway to set the border color manually.) But in this case, we are not downscaling at all, so we could just drop the bounce, saving tons of texture bandwidth. Thus, we add yet more parameters that effects can specify; first, that an effect uses _one-to-one_ sampling; that is, that it will only use its input as-is without sampling between texels or outside the border (so the different interpolation and border behavior will be irrelevant). (Actually, almost all of our effects fall into this category.) Second, a flag saying that even if an effect changes size, it doesn't use virtual sizes (otherwise even a one-to-one effect would de-facto be sampling between texels). If these flags are set on the input and the output respectively, we can avoid the bounce, at least unless there's an effect that's _not_ one-to-one further up the chain. For my motivating case, this folded eight phases into four, changing ~16.0 ms into ~10.6 ms rendering time. Seemingly memory bandwidth is a really precious resource on my laptop's GPU. --- diff --git a/alpha_division_effect.h b/alpha_division_effect.h index ae8f786..2fd9082 100644 --- a/alpha_division_effect.h +++ b/alpha_division_effect.h @@ -14,6 +14,7 @@ public: AlphaDivisionEffect() {} virtual std::string effect_type_id() const { return "AlphaDivisionEffect"; } std::string output_fragment_shader(); + virtual bool one_to_one_sampling() const { return true; } }; } // namespace movit diff --git a/alpha_multiplication_effect.h b/alpha_multiplication_effect.h index bc66d72..f342719 100644 --- a/alpha_multiplication_effect.h +++ b/alpha_multiplication_effect.h @@ -14,6 +14,7 @@ public: AlphaMultiplicationEffect() {} virtual std::string effect_type_id() const { return "AlphaMultiplicationEffect"; } std::string output_fragment_shader(); + virtual bool one_to_one_sampling() const { return true; } }; } // namespace movit diff --git a/blur_effect.h b/blur_effect.h index f531eb6..eb35790 100644 --- a/blur_effect.h +++ b/blur_effect.h @@ -81,6 +81,8 @@ public: } } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return true; } + virtual bool one_to_one_sampling() const { return false; } // Can sample outside the border. virtual void get_output_size(unsigned *width, unsigned *height, unsigned *virtual_width, unsigned *virtual_height) const { *width = this->width; diff --git a/colorspace_conversion_effect.h b/colorspace_conversion_effect.h index 76246d6..92bad1f 100644 --- a/colorspace_conversion_effect.h +++ b/colorspace_conversion_effect.h @@ -28,6 +28,7 @@ public: virtual bool needs_srgb_primaries() const { return false; } virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } // Get a conversion matrix from the given color space to XYZ. static Eigen::Matrix3d get_xyz_matrix(Colorspace space); diff --git a/complex_modulate_effect.h b/complex_modulate_effect.h index fd4eb8b..e2bf50d 100644 --- a/complex_modulate_effect.h +++ b/complex_modulate_effect.h @@ -41,6 +41,7 @@ public: // no way of expressing that currently. virtual bool needs_texture_bounce() const { return true; } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } virtual void inform_input_size(unsigned input_num, unsigned width, unsigned height); virtual void get_output_size(unsigned *width, unsigned *height, diff --git a/diffusion_effect.h b/diffusion_effect.h index 64b94dd..77625ae 100644 --- a/diffusion_effect.h +++ b/diffusion_effect.h @@ -57,6 +57,7 @@ public: virtual AlphaHandling alpha_handling() const { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } unsigned num_inputs() const { return 2; } + virtual bool one_to_one_sampling() const { return true; } private: float blurred_mix_amount; diff --git a/dither_effect.h b/dither_effect.h index 9818603..e580586 100644 --- a/dither_effect.h +++ b/dither_effect.h @@ -66,6 +66,7 @@ public: // premultiplied error. However, we need to do dithering in the same // space as quantization, whether that be pre- or postmultiply. virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } void set_gl_state(GLuint glsl_program_num, const std::string &prefix, unsigned *sampler_num); diff --git a/effect.h b/effect.h index 9d95705..fbe39d7 100644 --- a/effect.h +++ b/effect.h @@ -161,12 +161,38 @@ public: // needs mipmaps, you will also get them). virtual bool needs_mipmaps() const { return false; } + // Whether there is a direct correspondence between input and output + // texels. Specifically, the effect must not: + // + // 1. Try to sample in the border (ie., outside the 0.0 to 1.0 area). + // 2. Try to sample between texels. + // 3. Sample with an x- or y-derivative different from -1 or 1. + // (This also means needs_mipmaps() and one_to_one_sampling() + // together would make no sense.) + // + // The most common case for this would be an effect that has an exact + // 1:1-correspondence between input and output texels, e.g. SaturationEffect. + // However, more creative things, like mirroring/flipping or padding, + // would also be allowed. + // + // The primary gain from setting this is that you can sample directly + // from an effect that changes output size (see changes_output_size() below), + // without going through a bounce texture. It won't work for effects that + // set sets_virtual_output_size(), though. + // + // Does not make a lot of sense together with needs_texture_bounce(). + virtual bool one_to_one_sampling() const { return false; } + // Whether this effect wants to output to a different size than - // its input(s) (see inform_input_size(), below). If you set this to - // true, the output will be bounced to a texture (similarly to if the - // next effect set needs_texture_bounce()). + // its input(s) (see inform_input_size(), below). See also + // sets_virtual_output_size() below. virtual bool changes_output_size() const { return false; } + // Whether your get_output_size() function (see below) intends to ever set + // virtual_width different from width, or similar for height. + // It does not make sense to set this to true if changes_output_size() is false. + virtual bool sets_virtual_output_size() const { return changes_output_size(); } + // Whether this effect is effectively sampling from a a single texture. // If so, it will override needs_texture_bounce(); however, there are also // two demands it needs to fulfill: diff --git a/effect_chain.cpp b/effect_chain.cpp index 048cc1e..519cb2c 100644 --- a/effect_chain.cpp +++ b/effect_chain.cpp @@ -89,6 +89,7 @@ Node *EffectChain::add_node(Effect *effect) node->output_gamma_curve = GAMMA_INVALID; node->output_alpha_type = ALPHA_INVALID; node->needs_mipmaps = false; + node->one_to_one_sampling = false; nodes.push_back(node); node_map[effect] = node; @@ -289,8 +290,8 @@ void EffectChain::compile_glsl_program(Phase *phase) // Construct GLSL programs, starting at the given effect and following // the chain from there. We end a program every time we come to an effect // marked as "needs texture bounce", one that is used by multiple other -// effects, every time an effect wants to change the output size, -// and of course at the end. +// effects, every time we need to bounce due to output size change +// (not all size changes require ending), and of course at the end. // // We follow a quite simple depth-first search from the output, although // without recursing explicitly within each phase. @@ -303,6 +304,13 @@ Phase *EffectChain::construct_phase(Node *output, map *complete Phase *phase = new Phase; phase->output_node = output; + // If the output effect has one-to-one sampling, we try to trace this + // status down through the dependency chain. This is important in case + // we hit an effect that changes output size (and not sets a virtual + // output size); if we have one-to-one sampling, we don't have to break + // the phase. + output->one_to_one_sampling = output->effect->one_to_one_sampling(); + // Effects that we have yet to calculate, but that we know should // be in the current phase. stack effects_todo_this_phase; @@ -380,7 +388,14 @@ Phase *EffectChain::construct_phase(Node *output, map *complete } } - if (deps[i]->effect->changes_output_size()) { + if (deps[i]->effect->sets_virtual_output_size()) { + assert(deps[i]->effect->changes_output_size()); + // If the next effect sets a virtual size to rely on OpenGL's + // bilinear sampling, we'll really need to break the phase here. + start_new_phase = true; + } else if (deps[i]->effect->changes_output_size() && !node->one_to_one_sampling) { + // If the next effect changes size and we don't have one-to-one sampling, + // we also need to break here. start_new_phase = true; } @@ -388,6 +403,10 @@ Phase *EffectChain::construct_phase(Node *output, map *complete phase->inputs.push_back(construct_phase(deps[i], completed_effects)); } else { effects_todo_this_phase.push(deps[i]); + + // Propagate the one-to-one status down through the dependency. + deps[i]->one_to_one_sampling = node->one_to_one_sampling && + deps[i]->effect->one_to_one_sampling(); } } } @@ -419,6 +438,14 @@ Phase *EffectChain::construct_phase(Node *output, map *complete } } + // Tell each node which phase it ended up in, so that the unit test + // can check that the phases were split in the right place. + // Note that this ignores that effects may be part of multiple phases; + // if the unit tests need to test such cases, we'll reconsider. + for (unsigned i = 0; i < phase->effects.size(); ++i) { + phase->effects[i]->containing_phase = phase; + } + // Actually make the shader for this phase. compile_glsl_program(phase); @@ -649,9 +676,12 @@ void EffectChain::inform_input_sizes(Phase *phase) if (node->effect->changes_output_size()) { // We cannot call get_output_size() before we've done inform_input_size() // on all inputs. - unsigned real_width_ignored, real_height_ignored; - node->effect->get_output_size(&real_width_ignored, &real_height_ignored, + unsigned real_width, real_height; + node->effect->get_output_size(&real_width, &real_height, &node->output_width, &node->output_height); + assert(node->effect->sets_virtual_output_size() || + (real_width == node->output_width && + real_height == node->output_height)); } else { node->output_width = this_output_width; node->output_height = this_output_height; @@ -669,6 +699,9 @@ void EffectChain::find_output_size(Phase *phase) if (output_node->effect->changes_output_size()) { output_node->effect->get_output_size(&phase->output_width, &phase->output_height, &phase->virtual_output_width, &phase->virtual_output_height); + assert(output_node->effect->sets_virtual_output_size() || + (phase->output_width == phase->virtual_output_width && + phase->output_height == phase->virtual_output_height)); return; } diff --git a/effect_chain.h b/effect_chain.h index b87fce7..2f088b6 100644 --- a/effect_chain.h +++ b/effect_chain.h @@ -62,6 +62,10 @@ public: std::vector outgoing_links; std::vector incoming_links; + // For unit tests only. Do not use from other code. + // Will contain an arbitrary choice if the node is in multiple phases. + Phase *containing_phase; + private: // Logical size of the output of this effect, ie. the resolution // you would get if you sampled it as a texture. If it is undefined @@ -85,6 +89,10 @@ private: AlphaType output_alpha_type; bool needs_mipmaps; // Directly or indirectly. + // Set if this effect, and all effects consuming output from this node + // (in the same phase) have one_to_one_sampling() set. + bool one_to_one_sampling; + friend class EffectChain; }; diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp index 516dd54..28ccc70 100644 --- a/effect_chain_test.cpp +++ b/effect_chain_test.cpp @@ -1053,6 +1053,93 @@ TEST(EffectChainTest, VirtualSizeIsSentOnToInputs) { expect_equal(data, out_data, size, size); } +// An effect that is like VirtualResizeEffect, but always has virtual and real +// sizes the same (and promises this). +class NonVirtualResizeEffect : public VirtualResizeEffect { +public: + NonVirtualResizeEffect(int width, int height) + : VirtualResizeEffect(width, height, width, height) {} + virtual string effect_type_id() const { return "NonVirtualResizeEffect"; } + virtual bool sets_virtual_output_size() const { return false; } +}; + +// An effect that promises one-to-one sampling (unlike IdentityEffect). +class OneToOneEffect : public Effect { +public: + OneToOneEffect() {} + virtual string effect_type_id() const { return "OneToOneEffect"; } + string output_fragment_shader() { return read_file("identity.frag"); } + virtual bool one_to_one_sampling() const { return true; } +}; + +TEST(EffectChainTest, NoBounceWithOneToOneSampling) { + const int size = 2; + float data[size * size] = { + 1.0f, 0.0f, + 0.0f, 1.0f, + }; + float out_data[size * size]; + + EffectChainTester tester(data, size, size, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_LINEAR); + + RewritingEffect *effect1 = new RewritingEffect(); + RewritingEffect *effect2 = new RewritingEffect(); + + tester.get_chain()->add_effect(new NonVirtualResizeEffect(size, size)); + tester.get_chain()->add_effect(effect1); + tester.get_chain()->add_effect(effect2); + tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR); + + expect_equal(data, out_data, size, size); + + // The first OneToOneEffect should be in the same phase as its input. + ASSERT_EQ(1, effect1->replaced_node->incoming_links.size()); + EXPECT_EQ(effect1->replaced_node->incoming_links[0]->containing_phase, + effect1->replaced_node->containing_phase); + + // The second OneToOneEffect, too. + EXPECT_EQ(effect1->replaced_node->containing_phase, + effect2->replaced_node->containing_phase); +} + +TEST(EffectChainTest, BounceWhenOneToOneIsBroken) { + const int size = 2; + float data[size * size] = { + 1.0f, 0.0f, + 0.0f, 1.0f, + }; + float out_data[size * size]; + + EffectChainTester tester(data, size, size, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_LINEAR); + + RewritingEffect *effect1 = new RewritingEffect(); + RewritingEffect *effect2 = new RewritingEffect(); + RewritingEffect *effect3 = new RewritingEffect(); + RewritingEffect *effect4 = new RewritingEffect(); + + tester.get_chain()->add_effect(new NonVirtualResizeEffect(size, size)); + tester.get_chain()->add_effect(effect1); + tester.get_chain()->add_effect(effect2); + tester.get_chain()->add_effect(effect3); + tester.get_chain()->add_effect(effect4); + tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR); + + expect_equal(data, out_data, size, size); + + // The NonVirtualResizeEffect should be in a different phase from + // the IdentityEffect (since the latter is not one-to-one), + // ie., the chain should be broken somewhere between them, but exactly + // where doesn't matter. + ASSERT_EQ(1, effect1->replaced_node->incoming_links.size()); + EXPECT_NE(effect1->replaced_node->incoming_links[0]->containing_phase, + effect3->replaced_node->containing_phase); + + // The last OneToOneEffect should also be in the same phase as the + // IdentityEffect (the phase was already broken). + EXPECT_EQ(effect3->replaced_node->containing_phase, + effect4->replaced_node->containing_phase); +} + // Does not use EffectChainTest, so that it can construct an EffectChain without // a shared ResourcePool (which is also properly destroyed afterwards). // Also turns on debugging to test that code path. diff --git a/fft_pass_effect.h b/fft_pass_effect.h index fbf4511..561bc6d 100644 --- a/fft_pass_effect.h +++ b/fft_pass_effect.h @@ -85,6 +85,7 @@ public: // in our own phase, which is exactly what we want. virtual bool needs_texture_bounce() const { return true; } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } virtual void inform_input_size(unsigned input_num, unsigned width, unsigned height) { diff --git a/gamma_compression_effect.h b/gamma_compression_effect.h index ee3985f..52a8654 100644 --- a/gamma_compression_effect.h +++ b/gamma_compression_effect.h @@ -29,6 +29,7 @@ public: virtual void set_gl_state(GLuint glsl_program_num, const std::string &prefix, unsigned *sampler_num); virtual bool needs_srgb_primaries() const { return false; } + virtual bool one_to_one_sampling() const { return true; } // Actually needs postmultiplied input as well as outputting it. // EffectChain will take care of that. diff --git a/gamma_expansion_effect.h b/gamma_expansion_effect.h index 81f42d1..1025695 100644 --- a/gamma_expansion_effect.h +++ b/gamma_expansion_effect.h @@ -30,6 +30,7 @@ public: virtual bool needs_linear_light() const { return false; } virtual bool needs_srgb_primaries() const { return false; } + virtual bool one_to_one_sampling() const { return true; } // Actually processes its input in a nonlinear fashion, // but does not touch alpha, and we are a special case anyway. diff --git a/glow_effect.h b/glow_effect.h index 2a2af16..02520d7 100644 --- a/glow_effect.h +++ b/glow_effect.h @@ -52,6 +52,7 @@ public: std::string output_fragment_shader(); virtual AlphaHandling alpha_handling() const { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } + virtual bool one_to_one_sampling() const { return true; } private: float cutoff; diff --git a/lift_gamma_gain_effect.h b/lift_gamma_gain_effect.h index c93724d..9183c58 100644 --- a/lift_gamma_gain_effect.h +++ b/lift_gamma_gain_effect.h @@ -32,6 +32,7 @@ public: LiftGammaGainEffect(); virtual std::string effect_type_id() const { return "LiftGammaGainEffect"; } virtual AlphaHandling alpha_handling() const { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } + virtual bool one_to_one_sampling() const { return true; } std::string output_fragment_shader(); void set_gl_state(GLuint glsl_program_num, const std::string &prefix, unsigned *sampler_num); diff --git a/luma_mix_effect.h b/luma_mix_effect.h index ce890df..c4b2476 100644 --- a/luma_mix_effect.h +++ b/luma_mix_effect.h @@ -26,6 +26,7 @@ public: virtual bool needs_srgb_primaries() const { return false; } virtual unsigned num_inputs() const { return 3; } + virtual bool one_to_one_sampling() const { return true; } virtual AlphaHandling alpha_handling() const { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } private: diff --git a/mirror_effect.h b/mirror_effect.h index c4b46db..d9c3f71 100644 --- a/mirror_effect.h +++ b/mirror_effect.h @@ -18,6 +18,7 @@ public: virtual bool needs_linear_light() const { return false; } virtual bool needs_srgb_primaries() const { return false; } virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } }; } // namespace movit diff --git a/mix_effect.h b/mix_effect.h index 9abe601..d891eb5 100644 --- a/mix_effect.h +++ b/mix_effect.h @@ -18,6 +18,7 @@ public: virtual bool needs_srgb_primaries() const { return false; } virtual unsigned num_inputs() const { return 2; } + virtual bool one_to_one_sampling() const { return true; } // TODO: In the common case where a+b=1, it would be useful to be able to set // alpha_handling() to INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK. However, right now diff --git a/multiply_effect.h b/multiply_effect.h index 92e98e3..c017e06 100644 --- a/multiply_effect.h +++ b/multiply_effect.h @@ -18,6 +18,7 @@ public: MultiplyEffect(); virtual std::string effect_type_id() const { return "MultiplyEffect"; } std::string output_fragment_shader(); + virtual bool one_to_one_sampling() const { return true; } private: RGBATuple factor; diff --git a/overlay_effect.h b/overlay_effect.h index 489d8d7..c11569f 100644 --- a/overlay_effect.h +++ b/overlay_effect.h @@ -24,6 +24,7 @@ public: virtual bool needs_srgb_primaries() const { return false; } virtual unsigned num_inputs() const { return 2; } + virtual bool one_to_one_sampling() const { return true; } // Actually, if _either_ image has blank alpha, our output will have // blank alpha, too (this only tells the framework that having _both_ diff --git a/padding_effect.h b/padding_effect.h index 901f892..13dc5c0 100644 --- a/padding_effect.h +++ b/padding_effect.h @@ -31,6 +31,8 @@ public: virtual AlphaHandling alpha_handling() const; virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } + virtual bool one_to_one_sampling() const { return true; } virtual void get_output_size(unsigned *width, unsigned *height, unsigned *virtual_width, unsigned *virtual_height) const; virtual void inform_input_size(unsigned input_num, unsigned width, unsigned height); diff --git a/resample_effect.h b/resample_effect.h index 05ef365..669d15b 100644 --- a/resample_effect.h +++ b/resample_effect.h @@ -85,6 +85,7 @@ public: } } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } virtual void get_output_size(unsigned *width, unsigned *height, unsigned *virtual_width, unsigned *virtual_height) const { *virtual_width = *width = this->output_width; diff --git a/resize_effect.h b/resize_effect.h index dff6dcc..77dfdd4 100644 --- a/resize_effect.h +++ b/resize_effect.h @@ -24,6 +24,7 @@ public: virtual AlphaHandling alpha_handling() const { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } virtual void get_output_size(unsigned *width, unsigned *height, unsigned *virtual_width, unsigned *virtual_height) const; private: diff --git a/saturation_effect.h b/saturation_effect.h index a80740f..bf97572 100644 --- a/saturation_effect.h +++ b/saturation_effect.h @@ -18,6 +18,7 @@ public: SaturationEffect(); virtual std::string effect_type_id() const { return "SaturationEffect"; } virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } std::string output_fragment_shader(); private: diff --git a/slice_effect.h b/slice_effect.h index 89aeb0e..ccca527 100644 --- a/slice_effect.h +++ b/slice_effect.h @@ -24,6 +24,7 @@ public: std::string output_fragment_shader(); virtual bool needs_texture_bounce() const { return true; } virtual bool changes_output_size() const { return true; } + virtual bool sets_virtual_output_size() const { return false; } virtual void inform_input_size(unsigned input_num, unsigned width, unsigned height); virtual void get_output_size(unsigned *width, unsigned *height, unsigned *virtual_width, unsigned *virtual_height) const; diff --git a/vignette_effect.h b/vignette_effect.h index fdf1a11..582e676 100644 --- a/vignette_effect.h +++ b/vignette_effect.h @@ -19,6 +19,7 @@ public: virtual bool needs_srgb_primaries() const { return false; } virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } virtual void inform_input_size(unsigned input_num, unsigned width, unsigned height); void set_gl_state(GLuint glsl_program_num, const std::string &prefix, unsigned *sampler_num); diff --git a/white_balance_effect.h b/white_balance_effect.h index f438b91..342426b 100644 --- a/white_balance_effect.h +++ b/white_balance_effect.h @@ -15,6 +15,7 @@ public: WhiteBalanceEffect(); virtual std::string effect_type_id() const { return "WhiteBalanceEffect"; } virtual AlphaHandling alpha_handling() const { return DONT_CARE_ALPHA_TYPE; } + virtual bool one_to_one_sampling() const { return true; } std::string output_fragment_shader(); void set_gl_state(GLuint glsl_program_num, const std::string &prefix, unsigned *sampler_num);