Add FilterQuality parameter to FragmentShader.setImageSampler #179760
Conversation
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
There was a problem hiding this comment.
The code looks great, just a few nits I picked out. The only thing that needs to be changed is that the fragment_shader_sampler with filter quality tests are difficult to look at and know if they are correct. There are too many of them too, so I'm not sure if that's the best place to add them. Can we make them more like the ones you added in fragment_shader_test.dart?
| out vec4 frag_color; | ||
|
|
||
| void main() { | ||
| frag_color = texture(u_texture, FlutterFragCoord().xy / u_size); |
There was a problem hiding this comment.
Do we have this already as a fixture?
There was a problem hiding this comment.
Couldn't find one in this directory, unless it's contained somewhere else.
The closest I could find is https://github.com/flutter/flutter/blob/6e1aa823523dcb4fdcde9b7de669015c2cf5d3e3/engine/src/flutter/lib/ui/fixtures/shaders/general_shaders/filter_shader.frag, which almost outputs the texture directly but swaps colors.
| await matchGoldenFile('${name}_fragment_shader_sampler.png', region: drawRegion); | ||
| }); | ||
|
|
||
| test('fragment_shader_sampler with filter quality', () async { |
There was a problem hiding this comment.
What I'd like to see in this test is blowing up a tiny pixel art image (like a checkerboard) with the 2 different settings, none and low. That way we can easily visually validate wether the filter quality is being respected or not. This is hard to look at and know if it is correct.
There was a problem hiding this comment.
Changed it to match the non-web test, using the new oval gradient image. I also moved this test from image_golden_test.dart to fragment_shader_test.dart, which seems like a more specific place for it.
| shader.setFloat(0, 300); | ||
| shader.setFloat(1, 300); |
There was a problem hiding this comment.
Please use the getUniformFloat api that gets uniforms by name.
| ..setFloat(0, 100) | ||
| ..setFloat(1, 100); |
|
|
||
| test('FragmentShader renders sampler with filter quality default', () async { | ||
| final FragmentProgram program = await FragmentProgram.fromAsset('texture.frag.iplr'); | ||
| final Image blueGreenImage = await _createBlueGreenImage(); |
There was a problem hiding this comment.
nit: I think a checkerboard would be a bit easier to see. I was able to see the filtering fine though like this.
There was a problem hiding this comment.
I changed it to use a oval with a gradient. I think this shows the differences much better than the blue/green image or a checkerboard, because it has both a variation of colors and also has curves.
The None, Low/Medium, and High settings produce noticeably different results. The Low and Medium images are identical though. I couldn't find anything that would make them produce different results.
| final FragmentProgram program = await FragmentProgram.fromAsset('texture.frag.iplr'); | ||
| final Image blueGreenImage = await _createBlueGreenImage(); | ||
| final FragmentShader shader = program.fragmentShader() | ||
| ..setImageSampler(0, blueGreenImage, filterQuality: FilterQuality.low) | ||
| ..setFloat(0, 100) | ||
| ..setFloat(1, 100); | ||
|
|
||
| final Image shaderImage = await _imageFromShader(shader: shader, imageDimension: 100); | ||
|
|
||
| await comparer.addGoldenImage(shaderImage, 'render_sampler_low.png'); | ||
| shader.dispose(); | ||
| blueGreenImage.dispose(); |
There was a problem hiding this comment.
nit: These tests share all the same code except the filter quality. A helper function could be factored out.
There was a problem hiding this comment.
I changed it to use a for-lopp.
gaaclarke
left a comment
There was a problem hiding this comment.
code looks good to me, I want to see the golden output to visualize it before approval
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, let's just rename the tests so they they better communicate they are the same tests but some of them are for the web. Sorry we don't have a better existing convention for this.
| (ui.FilterQuality.none, 'fragment_shader_texture_with_quality_none.png'), | ||
| (ui.FilterQuality.low, 'fragment_shader_texture_with_quality_low.png'), | ||
| (ui.FilterQuality.medium, 'fragment_shader_texture_with_quality_medium.png'), | ||
| (ui.FilterQuality.high, 'fragment_shader_texture_with_quality_high.png'), |
There was a problem hiding this comment.
can you prefix these tests with web_
| (FilterQuality.none, 'render_sampler_none.png'), | ||
| (FilterQuality.low, 'render_sampler_low.png'), | ||
| (FilterQuality.medium, 'render_sampler_medium.png'), | ||
| (FilterQuality.high, 'render_sampler_high.png'), |
There was a problem hiding this comment.
can you change these tests names to match the web ones (without the web_ prefix)?
Roll Flutter from 6e1aa823523d to d81baabfec4c (47 revisions) flutter/flutter@6e1aa82...d81baab 2025-12-16 [email protected] Revert "chore: linux fuchsia tests are flaking" (flutter/flutter#179897) 2025-12-15 [email protected] add default-linux-sysroot to gn frontend args (flutter/flutter#179303) 2025-12-15 [email protected] Roll pub packages (flutter/flutter#179751) 2025-12-15 [email protected] Roll Skia from 783ce2077e46 to 6903a4e65c3f (2 revisions) (flutter/flutter#179903) 2025-12-15 [email protected] Add `Adwaita Sans` as a font fallback on Linux (flutter/flutter#179144) 2025-12-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Unmodified android sdk bundle (#179647)" (flutter/flutter#179904) 2025-12-15 [email protected] Update window settings as they change rather than the more outdated "Apply" pattern. (flutter/flutter#179861) 2025-12-15 49699333+dependabot[bot]@users.noreply.github.com Bump dessant/lock-threads from 5.0.1 to 6.0.0 in the all-github-actions group (flutter/flutter#179901) 2025-12-15 [email protected] Roll Skia from 9decbd4c216b to 783ce2077e46 (2 revisions) (flutter/flutter#179896) 2025-12-15 [email protected] Unmodified android sdk bundle (flutter/flutter#179647) 2025-12-15 [email protected] [web] Fix `resizeToAvoidBottomInset` on Android web (flutter/flutter#179581) 2025-12-15 [email protected] Suppress deprecation warning for AChoreographer_postFrameCallback (flutter/flutter#178580) 2025-12-15 [email protected] remove unnecessary virtual destructor from VertexDescriptor (flutter/flutter#178682) 2025-12-15 [email protected] Roll Dart SDK from b245f6022727 to 20d114f951db (1 revision) (flutter/flutter#179893) 2025-12-15 [email protected] [Impeller] Delete GLES framebuffer objects only on the thread where they were created (flutter/flutter#179768) 2025-12-15 [email protected] [Impeller] Convert paths in ImpellerC command line flags to std::filesystem::path objects (flutter/flutter#179717) 2025-12-15 [email protected] Roll Skia from 5e1ff611b36b to 9decbd4c216b (2 revisions) (flutter/flutter#179890) 2025-12-15 [email protected] Roll Fuchsia Linux SDK from DtfDWAx6t_CsD2e1W... to 433KtmJvbMyaDMJvD... (flutter/flutter#179889) 2025-12-15 [email protected] Remove unnecessary unboxing in `FlutterLoader.java` (flutter/flutter#179869) 2025-12-15 [email protected] Roll Skia from f04f8ca3b284 to 5e1ff611b36b (1 revision) (flutter/flutter#179882) 2025-12-15 [email protected] Roll Packages from 0ac7a03 to 2cd921c (1 revision) (flutter/flutter#179885) 2025-12-15 [email protected] [ Widget Preview ] Pass DTD URI as a constant in a generated file (flutter/flutter#179821) 2025-12-15 [email protected] Bump minSdk to `24` in `engine` (flutter/flutter#175508) 2025-12-15 [email protected] Roll Dart SDK from bca8bb37d95f to b245f6022727 (1 revision) (flutter/flutter#179879) 2025-12-15 [email protected] Roll Skia from eb1347d18957 to f04f8ca3b284 (1 revision) (flutter/flutter#179876) 2025-12-15 [email protected] Roll Skia from 41bcfb848b13 to eb1347d18957 (1 revision) (flutter/flutter#179871) 2025-12-15 [email protected] Roll Skia from d194c3b7a9a9 to 41bcfb848b13 (6 revisions) (flutter/flutter#179866) 2025-12-14 [email protected] Remove obsolete windowing channel (flutter/flutter#179718) 2025-12-14 [email protected] Roll Fuchsia Linux SDK from SCLq31whvg8nJvuYE... to DtfDWAx6t_CsD2e1W... (flutter/flutter#179856) 2025-12-14 [email protected] Roll Skia from 5d98ce0af031 to d194c3b7a9a9 (1 revision) (flutter/flutter#179854) 2025-12-13 [email protected] Roll Skia from f9b242d9a365 to 5d98ce0af031 (1 revision) (flutter/flutter#179843) 2025-12-13 [email protected] Roll Skia from abe90c72cf56 to f9b242d9a365 (1 revision) (flutter/flutter#179838) 2025-12-13 [email protected] Roll Fuchsia Linux SDK from fppT9ZrwbFx7iYrIh... to SCLq31whvg8nJvuYE... (flutter/flutter#179836) 2025-12-13 [email protected] Roll Dart SDK from 34654f2a3baa to bca8bb37d95f (1 revision) (flutter/flutter#179835) 2025-12-13 [email protected] Roll Skia from 9e7c893bb593 to abe90c72cf56 (1 revision) (flutter/flutter#179830) 2025-12-13 [email protected] Roll Dart SDK from f105c2168574 to 34654f2a3baa (1 revision) (flutter/flutter#179823) 2025-12-13 [email protected] Add FilterQuality parameter to FragmentShader.setImageSampler (flutter/flutter#179760) 2025-12-13 [email protected] Add profiling counts to pipeline uses (flutter/flutter#179374) 2025-12-13 [email protected] Roll Skia from edf52cb27f29 to 9e7c893bb593 (3 revisions) (flutter/flutter#179819) 2025-12-12 [email protected] Update Skwasm to engine style guidelines. (flutter/flutter#179756) 2025-12-12 [email protected] Roll Dart SDK from 9a65db770758 to f105c2168574 (5 revisions) (flutter/flutter#179814) 2025-12-12 [email protected] Roll Skia from f23b00a407a4 to edf52cb27f29 (5 revisions) (flutter/flutter#179807) 2025-12-12 [email protected] Use depth buffer to implement geometry overdraw protection (flutter/flutter#179426) 2025-12-12 [email protected] [flutter_tool] Force UTF-8 character set for dev (flutter/flutter#179419) 2025-12-12 [email protected] Roll Skia from e66816c3645e to f23b00a407a4 (2 revisions) (flutter/flutter#179798) 2025-12-12 [email protected] Implements decodeImageFromPixelsSync (flutter/flutter#179519) ...
Add FilterQuality parameter to FragmentShader.setImageSampler
Fixes #133944
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.