From bff07c51f937a4fdf005d31ef7e064467db67511 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Wed, 29 Jul 2015 01:28:30 +0200 Subject: [PATCH] Allow inputs to say they cannot support mipmaps. Really only FlatInput can easily support mipmaps; for things like YCbCrInput that combine multiple inputs, it's hard (probably not downright impossible, but at least not immediately obvious without thinking about it a bit) and for FFTInput it makes no sense. Thus, we allow an input to say that it can't do this, and then bounce it to a texture if needed. Hopefully this should happen quite rarely. --- effect_chain.cpp | 24 +++++++++++++- effect_chain.h | 1 + effect_chain_test.cpp | 75 +++++++++++++++++++++++++++++++++++++++++++ fft_input.h | 1 + input.h | 4 +++ ycbcr_input.cpp | 3 -- ycbcr_input.h | 3 +- ycbcr_input_test.cpp | 1 - 8 files changed, 105 insertions(+), 7 deletions(-) diff --git a/effect_chain.cpp b/effect_chain.cpp index 21c4a0d..b00f214 100644 --- a/effect_chain.cpp +++ b/effect_chain.cpp @@ -87,6 +87,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; nodes.push_back(node); node_map[effect] = node; @@ -310,6 +311,10 @@ Phase *EffectChain::construct_phase(Node *output, map *complete Node *node = effects_todo_this_phase.top(); effects_todo_this_phase.pop(); + if (node->effect->needs_mipmaps()) { + node->needs_mipmaps = true; + } + // This should currently only happen for effects that are inputs // (either true inputs or phase outputs). We special-case inputs, // and then deduplicate phase outputs below. @@ -334,6 +339,21 @@ Phase *EffectChain::construct_phase(Node *output, map *complete start_new_phase = true; } + // Propagate information about needing mipmaps down the chain, + // breaking the phase if we notice an incompatibility. + // + // 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) { + Input *input = static_cast(deps[i]->effect); + start_new_phase |= !input->can_supply_mipmaps(); + } else { + deps[i]->needs_mipmaps = true; + } + } + if (deps[i]->outgoing_links.size() > 1) { if (!deps[i]->effect->is_single_texture()) { // More than one effect uses this as the input, @@ -392,7 +412,9 @@ Phase *EffectChain::construct_phase(Node *output, map *complete for (unsigned i = 0; i < phase->effects.size(); ++i) { Node *node = phase->effects[i]; if (node->effect->num_inputs() == 0) { - CHECK(node->effect->set_int("needs_mipmaps", phase->input_needs_mipmaps)); + Input *input = static_cast(node->effect); + assert(!phase->input_needs_mipmaps || input->can_supply_mipmaps()); + CHECK(input->set_int("needs_mipmaps", phase->input_needs_mipmaps)); } } diff --git a/effect_chain.h b/effect_chain.h index 593232c..358deed 100644 --- a/effect_chain.h +++ b/effect_chain.h @@ -83,6 +83,7 @@ private: Colorspace output_color_space; GammaCurve output_gamma_curve; AlphaType output_alpha_type; + bool needs_mipmaps; // Directly or indirectly. friend class EffectChain; }; diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp index dd4ffa2..516dd54 100644 --- a/effect_chain_test.cpp +++ b/effect_chain_test.cpp @@ -604,6 +604,81 @@ TEST(EffectChainTest, MipmapGenerationWorks) { expect_equal(expected_data, out_data, 4, 16); } +class NonMipmapCapableInput : public FlatInput { +public: + NonMipmapCapableInput(ImageFormat format, MovitPixelFormat pixel_format, GLenum type, unsigned width, unsigned height) + : FlatInput(format, pixel_format, type, width, height) {} + + virtual bool can_supply_mipmaps() const { return false; } + bool set_int(const std::string& key, int value) { + if (key == "needs_mipmaps") { + assert(value == 0); + } + return FlatInput::set_int(key, value); + } +}; + +// The same test as MipmapGenerationWorks, but with an input that refuses +// to supply mipmaps. +TEST(EffectChainTest, MipmapsWithNonMipmapCapableInput) { + float data[] = { // In 4x4 blocks. + 1.0f, 0.0f, 0.0f, 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, 0.0f, + 0.0f, 0.5f, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 0.0f, + + 1.0f, 1.0f, 1.0f, 1.0f, + 1.0f, 1.0f, 1.0f, 1.0f, + 1.0f, 1.0f, 1.0f, 1.0f, + 1.0f, 1.0f, 1.0f, 1.0f, + + 0.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 1.0f, 0.0f, + 0.0f, 1.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 0.0f, + }; + float expected_data[] = { // Repeated four times each way. + 0.125f, 0.125f, 0.125f, 0.125f, + 0.09375f, 0.09375f, 0.09375f, 0.09375f, + 1.0f, 1.0f, 1.0f, 1.0f, + 0.25f, 0.25f, 0.25f, 0.25f, + + 0.125f, 0.125f, 0.125f, 0.125f, + 0.09375f, 0.09375f, 0.09375f, 0.09375f, + 1.0f, 1.0f, 1.0f, 1.0f, + 0.25f, 0.25f, 0.25f, 0.25f, + + 0.125f, 0.125f, 0.125f, 0.125f, + 0.09375f, 0.09375f, 0.09375f, 0.09375f, + 1.0f, 1.0f, 1.0f, 1.0f, + 0.25f, 0.25f, 0.25f, 0.25f, + + 0.125f, 0.125f, 0.125f, 0.125f, + 0.09375f, 0.09375f, 0.09375f, 0.09375f, + 1.0f, 1.0f, 1.0f, 1.0f, + 0.25f, 0.25f, 0.25f, 0.25f, + }; + float out_data[4 * 16]; + EffectChainTester tester(NULL, 4, 16, FORMAT_GRAYSCALE); + + ImageFormat format; + format.color_space = COLORSPACE_sRGB; + format.gamma_curve = GAMMA_LINEAR; + + NonMipmapCapableInput *input = new NonMipmapCapableInput(format, FORMAT_GRAYSCALE, GL_FLOAT, 4, 16); + input->set_pixel_data(data); + tester.get_chain()->add_input(input); + tester.get_chain()->add_effect(new MipmapNeedingEffect()); + tester.run(out_data, GL_RED, COLORSPACE_sRGB, GAMMA_LINEAR); + + expect_equal(expected_data, out_data, 4, 16); +} + TEST(EffectChainTest, ResizeDownByFourThenUpByFour) { float data[] = { // In 4x4 blocks. 1.0f, 0.0f, 0.0f, 0.0f, diff --git a/fft_input.h b/fft_input.h index b57723c..a75414c 100644 --- a/fft_input.h +++ b/fft_input.h @@ -48,6 +48,7 @@ public: virtual AlphaHandling alpha_handling() const { return INPUT_AND_OUTPUT_PREMULTIPLIED_ALPHA; } virtual bool is_single_texture() const { return true; } virtual bool can_output_linear_gamma() const { return true; } + virtual bool can_supply_mipmaps() const { return false; } // Tells the input where to fetch the actual pixel data. Note that if you change // this data, you must either call set_pixel_data() again (using the same pointer diff --git a/input.h b/input.h index 7652e17..08f18da 100644 --- a/input.h +++ b/input.h @@ -24,6 +24,10 @@ public: // to activate it.) virtual bool can_output_linear_gamma() const = 0; + // Whether this input can supply mipmaps if asked to (by setting + // the "needs_mipmaps" integer parameter set to 1). + virtual bool can_supply_mipmaps() const { return true; } + virtual unsigned get_width() const = 0; virtual unsigned get_height() const = 0; virtual Colorspace get_color_space() const = 0; diff --git a/ycbcr_input.cpp b/ycbcr_input.cpp index 25fb979..cf6632f 100644 --- a/ycbcr_input.cpp +++ b/ycbcr_input.cpp @@ -65,7 +65,6 @@ YCbCrInput::YCbCrInput(const ImageFormat &image_format, unsigned width, unsigned height) : image_format(image_format), ycbcr_format(ycbcr_format), - needs_mipmaps(false), width(width), height(height), resource_pool(NULL) @@ -84,8 +83,6 @@ YCbCrInput::YCbCrInput(const ImageFormat &image_format, heights[2] = height / ycbcr_format.chroma_subsampling_y; pixel_data[0] = pixel_data[1] = pixel_data[2] = NULL; - - register_int("needs_mipmaps", &needs_mipmaps); } YCbCrInput::~YCbCrInput() diff --git a/ycbcr_input.h b/ycbcr_input.h index bf6d800..3049277 100644 --- a/ycbcr_input.h +++ b/ycbcr_input.h @@ -58,6 +58,7 @@ public: unsigned get_height() const { return height; } Colorspace get_color_space() const { return image_format.color_space; } GammaCurve get_gamma_curve() const { return image_format.gamma_curve; } + virtual bool can_supply_mipmaps() const { return false; } // Tells the input where to fetch the actual pixel data. Note that if you change // this data, you must either call set_pixel_data() again (using the same pointer @@ -95,8 +96,6 @@ private: YCbCrFormat ycbcr_format; GLuint pbos[3], texture_num[3]; - int needs_mipmaps; - unsigned width, height, widths[3], heights[3]; const unsigned char *pixel_data[3]; unsigned pitch[3]; diff --git a/ycbcr_input_test.cpp b/ycbcr_input_test.cpp index 6895b38..55bb536 100644 --- a/ycbcr_input_test.cpp +++ b/ycbcr_input_test.cpp @@ -1,5 +1,4 @@ // Unit tests for YCbCrInput. -// FIXME: This class really ought to support mipmaps. #include #include -- 2.39.2