From f5e3256da7d8e3a56c002da47bedf8ec1a2133f4 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Wed, 17 Jan 2018 21:06:55 +0100 Subject: [PATCH 1/1] Add explicit support for requesting no mipmaps. This is significantly less hackish than having the few effects that don't want mipmaps having to request texture bounce and fiddling with the sampling parameters themselves. API change, although only for writing custom effects. --- blur_effect.h | 10 ++---- downscale2x.frag | 11 +++++++ effect.h | 37 ++++++++++++++++++---- effect_chain.cpp | 24 +++++++++----- effect_chain_test.cpp | 73 +++++++++++++++++++++++++++++++++++++++++-- flat_input.cpp | 2 ++ flat_input_test.cpp | 71 +++++++++++++++++++++++++++++++++++++++++ resample_effect.cpp | 10 ------ resample_effect.h | 9 +++--- resize_effect.h | 2 +- version.h | 2 +- ycbcr_input.cpp | 2 ++ ycbcr_input_test.cpp | 2 +- 13 files changed, 214 insertions(+), 41 deletions(-) create mode 100644 downscale2x.frag diff --git a/blur_effect.h b/blur_effect.h index 6224ac3..b543ebc 100644 --- a/blur_effect.h +++ b/blur_effect.h @@ -33,12 +33,6 @@ public: std::string effect_type_id() const override { return "BlurEffect"; } - // We want this for the same reason as ResizeEffect; we could end up scaling - // down quite a lot. - bool needs_texture_bounce() const override { return true; } - bool needs_mipmaps() const override { return true; } - bool needs_srgb_primaries() const override { return false; } - void inform_input_size(unsigned input_num, unsigned width, unsigned height) override; std::string output_fragment_shader() override { @@ -71,8 +65,10 @@ public: std::string output_fragment_shader() override; + // We want this for the same reason as ResizeEffect; we could end up scaling + // down quite a lot. bool needs_texture_bounce() const override { return true; } - bool needs_mipmaps() const override { return true; } + MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; } bool needs_srgb_primaries() const override { return false; } AlphaHandling alpha_handling() const override { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } diff --git a/downscale2x.frag b/downscale2x.frag new file mode 100644 index 0000000..ed44afe --- /dev/null +++ b/downscale2x.frag @@ -0,0 +1,11 @@ +// Used only for testing. + +// Implicit uniforms: +// uniform vec2 PREFIX(offset); + +vec4 FUNCNAME(vec2 tc) +{ + return INPUT(tc * 2.0 + PREFIX(offset)); +// vec2 z = tc * 2.0 + PREFIX(offset); +// return vec4(z.y, 0.0f, 0.0f, 1.0f); +} diff --git a/effect.h b/effect.h index a1852fc..79e4020 100644 --- a/effect.h +++ b/effect.h @@ -169,12 +169,37 @@ public: // either will be fine. virtual bool needs_texture_bounce() const { return false; } - // Whether this effect expects mipmaps or not. If you set this to - // true, you will be sampling with bilinear filtering; if not, - // you could be sampling with simple linear filtering and no mipmaps - // (although there is no guarantee; if a different effect in the chain - // needs mipmaps, you will also get them). - virtual bool needs_mipmaps() const { return false; } + // Whether this effect expects mipmaps or not. + enum MipmapRequirements { + // If chosen, you will be sampling with bilinear filtering, + // ie. the closest mipmap will be chosen, and then there will be + // bilinear interpolation inside it (GL_LINEAR_MIPMAP_NEAREST). + NEEDS_MIPMAPS, + + // Whether the effect doesn't really care whether input textures + // are with or without mipmaps. You could get the same effect + // as NEEDS_MIPMAPS or CANNOT_ACCEPT_MIPMAPS; normally, you won't + // get them, but if a different effect in the same phase needs mipmaps, + // you will also get them. + DOES_NOT_NEED_MIPMAPS, + + // The opposite of NEEDS_MIPMAPS; you will always be sampling from + // the most detailed mip level (GL_LINEAR). Effects with NEEDS_MIPMAPS + // and CANNOT_ACCEPT_MIPMAPS can not coexist within the same phase; + // such phases will be split. + // + // This is the only choice that makes sense for a compute shader, + // given that it doesn't have screen-space derivatives and thus + // always will sample the most detailed mip level. + CANNOT_ACCEPT_MIPMAPS, + }; + virtual MipmapRequirements needs_mipmaps() const { + if (is_compute_shader()) { + return CANNOT_ACCEPT_MIPMAPS; + } else { + return DOES_NOT_NEED_MIPMAPS; + } + } // Whether there is a direct correspondence between input and output // texels. Specifically, the effect must not: diff --git a/effect_chain.cpp b/effect_chain.cpp index 3084930..95bf1c0 100644 --- a/effect_chain.cpp +++ b/effect_chain.cpp @@ -163,7 +163,7 @@ Node *EffectChain::add_node(Effect *effect) node->output_color_space = COLORSPACE_INVALID; node->output_gamma_curve = GAMMA_INVALID; node->output_alpha_type = ALPHA_INVALID; - node->needs_mipmaps = false; + node->needs_mipmaps = Effect::DOES_NOT_NEED_MIPMAPS; node->one_to_one_sampling = false; node->strong_one_to_one_sampling = false; @@ -655,8 +655,12 @@ Phase *EffectChain::construct_phase(Node *output, map *complete assert(node->effect->one_to_one_sampling() >= node->effect->strong_one_to_one_sampling()); - if (node->effect->needs_mipmaps()) { - node->needs_mipmaps = true; + if (node->effect->needs_mipmaps() != Effect::DOES_NOT_NEED_MIPMAPS) { + // Can't have incompatible requirements imposed on us from a dependent effect; + // if so, it should have started a new phase instead. + assert(node->needs_mipmaps == Effect::DOES_NOT_NEED_MIPMAPS || + node->needs_mipmaps == node->effect->needs_mipmaps()); + node->needs_mipmaps = node->effect->needs_mipmaps(); } // This should currently only happen for effects that are inputs @@ -694,12 +698,14 @@ Phase *EffectChain::construct_phase(Node *output, map *complete // Note that we cannot do this propagation as a normal pass, // because it needs information about where the phases end // (we should not propagate the flag across phases). - if (node->needs_mipmaps) { - if (deps[i]->effect->num_inputs() == 0) { + if (node->needs_mipmaps != Effect::DOES_NOT_NEED_MIPMAPS) { + if (deps[i]->effect->num_inputs() == 0 && node->needs_mipmaps == Effect::NEEDS_MIPMAPS) { Input *input = static_cast(deps[i]->effect); start_new_phase |= !input->can_supply_mipmaps(); - } else { - deps[i]->needs_mipmaps = true; + } else if (deps[i]->effect->needs_mipmaps() == Effect::DOES_NOT_NEED_MIPMAPS) { + deps[i]->needs_mipmaps = node->needs_mipmaps; + } else if (deps[i]->effect->needs_mipmaps() != node->needs_mipmaps) { + start_new_phase = true; } } @@ -788,7 +794,9 @@ Phase *EffectChain::construct_phase(Node *output, map *complete phase->input_needs_mipmaps = false; for (unsigned i = 0; i < phase->effects.size(); ++i) { Node *node = phase->effects[i]; - phase->input_needs_mipmaps |= node->effect->needs_mipmaps(); + if (node->effect->needs_mipmaps() == Effect::NEEDS_MIPMAPS) { + phase->input_needs_mipmaps = true; + } } for (unsigned i = 0; i < phase->effects.size(); ++i) { Node *node = phase->effects[i]; diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp index 3a3811d..ecf0b69 100644 --- a/effect_chain_test.cpp +++ b/effect_chain_test.cpp @@ -154,7 +154,8 @@ public: template class RewritingEffect : public Effect { public: - RewritingEffect() : effect(new T()), replaced_node(nullptr) {} + template + RewritingEffect(Args &&... args) : effect(new T(std::forward(args)...)), replaced_node(nullptr) {} string effect_type_id() const override { return "RewritingEffect[" + effect->effect_type_id() + "]"; } string output_fragment_shader() override { EXPECT_TRUE(false); return read_file("identity.frag"); } void rewrite_graph(EffectChain *graph, Node *self) override { @@ -565,7 +566,7 @@ TEST(EffectChainTest, AlphaConversionsWithNonBlankAlphaPreservingEffect) { class MipmapNeedingEffect : public Effect { public: MipmapNeedingEffect() {} - bool needs_mipmaps() const override { return true; } + MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; } // To be allowed to mess with the sampler state. bool needs_texture_bounce() const override { return true; } @@ -773,6 +774,74 @@ TEST(EffectChainTest, ResizeDownByFourThenUpByFour) { expect_equal(expected_data, out_data, 4, 16); } +// An effect to verify that you can turn off mipmaps; it downscales by two, +// which gives blur with mipmaps and aliasing (picks out every other pixel) +// without. +class Downscale2xEffect : public Effect { +public: + explicit Downscale2xEffect(MipmapRequirements mipmap_requirements) + : mipmap_requirements(mipmap_requirements) + { + register_vec2("offset", offset); + } + MipmapRequirements needs_mipmaps() const override { return mipmap_requirements; } + + string effect_type_id() const override { return "Downscale2xEffect"; } + string output_fragment_shader() override { return read_file("downscale2x.frag"); } + +private: + const MipmapRequirements mipmap_requirements; + EffectChain *chain; + float offset[2] { 0.0f, 0.0f }; +}; + +TEST(EffectChainTest, MipmapChainGetsSplit) { + float data[] = { + 0.0f, 0.0f, 0.0f, 0.0f, + 1.0f, 0.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 0.0f, + 1.0f, 0.0f, 1.0f, 0.0f, + }; + + // The intermediate result after the first step looks like this, + // assuming there are no mipmaps (the zeros are due to border behavior): + // + // 0 0 0 0 + // 0 0 0 0 + // 1 1 0 0 + // 1 1 0 0 + // + // so another 2x downscale towards the bottom left will give + // + // 0 0 + // 1 0 + // + // with yet more zeros coming in on the top and the right from the border. + float expected_data[] = { + 0.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 0.0f, + 1.0f, 0.0f, 0.0f, 0.0f, + }; + float out_data[4 * 4]; + + float offset[] = { -0.5f / 4.0f, -0.5f / 4.0f }; + RewritingEffect *pick_out_top_left = new RewritingEffect(Effect::CANNOT_ACCEPT_MIPMAPS); + ASSERT_TRUE(pick_out_top_left->effect->set_vec2("offset", offset)); + + RewritingEffect *downscale2x = new RewritingEffect(Effect::NEEDS_MIPMAPS); + + EffectChainTester tester(data, 4, 4, FORMAT_GRAYSCALE, COLORSPACE_sRGB, GAMMA_LINEAR); + tester.get_chain()->add_effect(pick_out_top_left); + tester.get_chain()->add_effect(downscale2x); + tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR); + + EXPECT_NE(pick_out_top_left->replaced_node->containing_phase, + downscale2x->replaced_node->containing_phase); + + expect_equal(expected_data, out_data, 4, 4); +} + // An effect that adds its two inputs together. Used below. class AddEffect : public Effect { public: diff --git a/flat_input.cpp b/flat_input.cpp index 29165db..6e3db34 100644 --- a/flat_input.cpp +++ b/flat_input.cpp @@ -166,6 +166,8 @@ void FlatInput::set_gl_state(GLuint glsl_program_num, const string& prefix, unsi } else { glBindTexture(GL_TEXTURE_2D, texture_num); check_error(); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, needs_mipmaps ? GL_LINEAR_MIPMAP_NEAREST : GL_LINEAR); + check_error(); } // Bind it to a sampler. diff --git a/flat_input_test.cpp b/flat_input_test.cpp index f01bab8..8358dcf 100644 --- a/flat_input_test.cpp +++ b/flat_input_test.cpp @@ -10,6 +10,8 @@ #include "test_util.h" #include "util.h" +using namespace std; + namespace movit { TEST(FlatInput, SimpleGrayscale) { @@ -320,6 +322,75 @@ TEST(FlatInput, ExternalTexture) { expect_equal(expected_data, out_data, 4, size); } +// Just an IdentityEffect, but marks as needing mipmaps, so that we can use it +// for downscaling to verify mipmaps were used. +class MipmapNeedingEffect : public Effect { +public: + MipmapNeedingEffect() {} + MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; } + + string effect_type_id() const override { return "MipmapNeedingEffect"; } + string output_fragment_shader() override { return read_file("identity.frag"); } + +private: + EffectChain *chain; +}; + +TEST(FlatInput, ExternalTextureMipmapState) { + const int width = 4; + const int height = 4; + + float data[width * height] = { + 1.0, 0.0, 0.0, 0.0, + 0.0, 0.0, 0.0, 0.0, + 0.0, 0.0, 0.0, 0.0, + 0.0, 0.0, 0.0, 0.0, + }; + float expected_data[] = { + 0.0625, + }; + float out_data[1]; + + EffectChainTester tester(nullptr, 1, 1, FORMAT_RGB, COLORSPACE_sRGB, GAMMA_LINEAR); + + ImageFormat format; + format.color_space = COLORSPACE_sRGB; + format.gamma_curve = GAMMA_LINEAR; + + ResourcePool pool; + GLuint tex = pool.create_2d_texture(GL_R8, width, height); + check_error(); + glBindTexture(GL_TEXTURE_2D, tex); + check_error(); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR_MIPMAP_NEAREST); + check_error(); + glPixelStorei(GL_UNPACK_ALIGNMENT, 1); + check_error(); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, width, height, GL_RED, GL_FLOAT, data); + check_error(); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); + check_error(); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); + check_error(); + glGenerateMipmap(GL_TEXTURE_2D); + check_error(); + + // Turn off mipmaps, so that we verify that Movit turns it back on. + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); + check_error(); + + FlatInput *input = new FlatInput(format, FORMAT_GRAYSCALE, GL_FLOAT, width, height); + input->set_texture_num(tex); + tester.get_chain()->add_input(input); + tester.get_chain()->add_effect(new MipmapNeedingEffect); + + tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR); + + pool.release_2d_texture(tex); + + expect_equal(expected_data, out_data, 1, 1); +} + TEST(FlatInput, NoData) { const int width = 2; const int height = 4; diff --git a/resample_effect.cpp b/resample_effect.cpp index 802a896..c688d70 100644 --- a/resample_effect.cpp +++ b/resample_effect.cpp @@ -721,16 +721,6 @@ void SingleResamplePassEffect::set_gl_state(GLuint glsl_program_num, const strin } else { uniform_whole_pixel_offset = lrintf(offset) / float(input_width); } - - // We specifically do not want mipmaps on the input texture; - // they break minification. - Node *self = chain->find_node_for_effect(this); - if (chain->has_input_sampler(self, 0)) { - glActiveTexture(chain->get_input_sampler(self, 0)); - check_error(); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - check_error(); - } } Support2DTexture::Support2DTexture() diff --git a/resample_effect.h b/resample_effect.h index 6a96ded..cf5f3bb 100644 --- a/resample_effect.h +++ b/resample_effect.h @@ -70,11 +70,6 @@ public: std::string effect_type_id() const override { return "ResampleEffect"; } - // We want this for the same reason as ResizeEffect; we could end up scaling - // down quite a lot. - bool needs_texture_bounce() const override { return true; } - bool needs_srgb_primaries() const override { return false; } - void inform_input_size(unsigned input_num, unsigned width, unsigned height) override; std::string output_fragment_shader() override { @@ -117,6 +112,10 @@ public: bool needs_srgb_primaries() const override { return false; } AlphaHandling alpha_handling() const override { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } + // We specifically do not want mipmaps on the input texture; + // they break minification. + MipmapRequirements needs_mipmaps() const override { return CANNOT_ACCEPT_MIPMAPS; } + void inform_added(EffectChain *chain) override { this->chain = chain; } void inform_input_size(unsigned input_num, unsigned width, unsigned height) override { if (parent != nullptr) { diff --git a/resize_effect.h b/resize_effect.h index 9d23806..a921eb0 100644 --- a/resize_effect.h +++ b/resize_effect.h @@ -20,7 +20,7 @@ public: // We want processing done pre-filtering and mipmapped, // in case we need to scale down a lot. bool needs_texture_bounce() const override { return true; } - bool needs_mipmaps() const override { return true; } + MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; } AlphaHandling alpha_handling() const override { return INPUT_PREMULTIPLIED_ALPHA_KEEP_BLANK; } bool changes_output_size() const override { return true; } diff --git a/version.h b/version.h index da4b1ee..bf8390c 100644 --- a/version.h +++ b/version.h @@ -5,6 +5,6 @@ // changes, even within git versions. There is no specific version // documentation outside the regular changelogs, though. -#define MOVIT_VERSION 34 +#define MOVIT_VERSION 35 #endif // !defined(_MOVIT_VERSION_H) diff --git a/ycbcr_input.cpp b/ycbcr_input.cpp index 47f58cc..d6cefe6 100644 --- a/ycbcr_input.cpp +++ b/ycbcr_input.cpp @@ -147,6 +147,8 @@ void YCbCrInput::set_gl_state(GLuint glsl_program_num, const string& prefix, uns } else { glBindTexture(GL_TEXTURE_2D, texture_num[channel]); check_error(); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, needs_mipmaps ? GL_LINEAR_MIPMAP_NEAREST : GL_LINEAR); + check_error(); } } diff --git a/ycbcr_input_test.cpp b/ycbcr_input_test.cpp index f90ffb7..a1415c9 100644 --- a/ycbcr_input_test.cpp +++ b/ycbcr_input_test.cpp @@ -1004,7 +1004,7 @@ TEST(YCbCrInputTest, TenBitPlanar) { class MipmapNeedingEffect : public Effect { public: MipmapNeedingEffect() {} - bool needs_mipmaps() const override { return true; } + MipmapRequirements needs_mipmaps() const override { return NEEDS_MIPMAPS; } // To be allowed to mess with the sampler state. bool needs_texture_bounce() const override { return true; } -- 2.39.2