[Impeller] skip mip generation if blur downsample is factor is 0.5 or greater #53635
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -356,6 +351,9 @@ fml::StatusOr<RenderTarget> MakeDownsampleSubpass( | |||
| SetTileMode(&linear_sampler_descriptor, renderer, tile_mode); | |||
| linear_sampler_descriptor.mag_filter = MinMagFilter::kLinear; | |||
| linear_sampler_descriptor.min_filter = MinMagFilter::kLinear; | |||
| if (min_lod_clamp) { | |||
There was a problem hiding this comment.
With a downsample factor of 0.5, by default we will try to sample from mip level 1 instead of 0. But forcing 0 and using linear filtering is sufficient to not lose quality.
|
It looks like this causes a validation failure on metal, even if we won't end up reading from a higher mip level. Potentially more fuel for separating out bdfs and image filter blurs, since at least in the later case we can compute exactly how many mip levels (if any) we need. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
| @@ -326,7 +320,8 @@ fml::StatusOr<RenderTarget> MakeDownsampleSubpass( | |||
| std::shared_ptr<Texture> input_texture, | |||
| const SamplerDescriptor& sampler_descriptor, | |||
| const DownsamplePassArgs& pass_args, | |||
| Entity::TileMode tile_mode) { | |||
| Entity::TileMode tile_mode, | |||
| bool min_lod_clamp) { | |||
There was a problem hiding this comment.
| bool min_lod_clamp) { | |
| bool use_base_mip) { |
| input_snapshot->sampler_descriptor, downsample_pass_args, tile_mode_); | ||
| // If a blur scalar is less than 0.5, generate mipmaps. Otherwise, the | ||
| // bilinear filtering of the base mip level is sufficient to preserve quality. | ||
| bool generated_mips = false; |
There was a problem hiding this comment.
| bool generated_mips = false; | |
| bool use_base_mip = true; |
|
Tests did hit this, hence the failure and my comment :) |
|
Closing in favor of #53760 |
THe downsample uses bilinear filtering to read from the input texture. To avoid dropping data, we generate mipmaps. However if the downscale factor is 0.5 or 1.0, then we don't need to read from any of the mip levels besides the base. Skip mip generation in this case, since its very expensive.
Additionally, mip generation is moved into the blur, rather than checking for mips and erroring - we generate mips if needed - removing an error condition.