[Impeller] Add filterQuality support to ImageFilter.shader#188544
[Impeller] Add filterQuality support to ImageFilter.shader#188544ahyangnb wants to merge 2 commits into
Conversation
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for specifying FilterQuality when creating an ImageFilter from a custom fragment shader, propagating the sampling configuration through the Dart API, native bindings, and Impeller backend. Feedback on the changes highlights a missing initialization of the u_size uniform in the test shader, which could cause undefined behavior, and suggests refactoring the sampler loop in image_filter.cc to use a boolean flag instead of an index counter for simpler logic.
There was a problem hiding this comment.
The fragment shader filter_shader_fractional_texel.frag declares a uniform vec2 u_size which is used to normalize the texture coordinates. However, in backdropShaderWithFilterQuality, the u_size uniform is never initialized on the shader object. This will result in a division by zero (or uninitialized garbage values) in the shader, leading to undefined behavior and potential test flakiness across different GPUs/backends.
Please set the u_size uniform using shader.setFloat before using it.
final FragmentShader shader = program.fragmentShader();
shader.setFloat(0, widthAsDouble);
shader.setFloat(1, heightAsDouble);There was a problem hiding this comment.
I don't agree with it, because for ImageFilter.shader, the first vec2 uniform is intentionally engine-managed. The API doc says the first uniform is set by the engine to the bound texture size in painting.dart. The actual Impeller render path does it in runtime_effect_filter_contents.cc.
| size_t index = 0; | ||
| for (const std::shared_ptr<flutter::DlColorSource>& sampler : | ||
| runtime_filter->samplers()) { | ||
| if (index == 0 && sampler == nullptr) { | ||
| // Insert placeholder for filter. | ||
| if (index == 0) { | ||
| // The first sampler is always the image filter input. | ||
| texture_inputs.push_back( | ||
| {.sampler_descriptor = skia_conversions::ToSamplerDescriptor({}), | ||
| {.sampler_descriptor = skia_conversions::ToSamplerDescriptor( | ||
| runtime_filter->input_sampling()), | ||
| .texture = nullptr}); | ||
| index++; | ||
| continue; |
There was a problem hiding this comment.
Using a bool is_first flag instead of an index counter simplifies the loop logic and makes it less error-prone, as we don't need to manually increment the index in multiple branches.
| size_t index = 0; | |
| for (const std::shared_ptr<flutter::DlColorSource>& sampler : | |
| runtime_filter->samplers()) { | |
| if (index == 0 && sampler == nullptr) { | |
| // Insert placeholder for filter. | |
| if (index == 0) { | |
| // The first sampler is always the image filter input. | |
| texture_inputs.push_back( | |
| {.sampler_descriptor = skia_conversions::ToSamplerDescriptor({}), | |
| {.sampler_descriptor = skia_conversions::ToSamplerDescriptor( | |
| runtime_filter->input_sampling()), | |
| .texture = nullptr}); | |
| index++; | |
| continue; | |
| bool is_first = true; | |
| for (const std::shared_ptr<flutter::DlColorSource>& sampler : | |
| runtime_filter->samplers()) { | |
| if (is_first) { | |
| is_first = false; | |
| // The first sampler is always the image filter input. | |
| texture_inputs.push_back( | |
| {.sampler_descriptor = skia_conversions::ToSamplerDescriptor( | |
| runtime_filter->input_sampling()), | |
| .texture = nullptr}); | |
| continue; | |
| } |
| texture_inputs.push_back({ | ||
| .sampler_descriptor = | ||
| skia_conversions::ToSamplerDescriptor(image->sampling()), | ||
| .texture = std::move(texture), | ||
| }); | ||
| index++; |
There was a problem hiding this comment.
Remove the index++ increment since we are now using the is_first boolean flag to identify the first sampler.
1 participant
Adds a
filterQualityargument toImageFilter.shaderso the implicitly bound image-filter input sampler can be configured instead of always using nearest-neighbor sampling.This threads the requested sampling quality through
dart:ui, DisplayList runtime-effect image filters, and Impeller’s runtime-effect texture input setup. The default remainsFilterQuality.noneto preserve existing behavior.This also adds regression coverage with a fractional-texel shader to verify that
BackdropFilter+ImageFilter.shaderhonorsFilterQuality, plus equality coverage for shader image filters that differ only by input sampling.Fixes #188365
No changes were required in the
flutter/testsrepo.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.