[impeller] adds golden test for jumping glyphs when scaling tests #161445
Conversation
github-actions
bot
added
a: text input
gaaclarke
changed the title
gaaclarke
changed the title
| position = (screen_glyph_position + | ||
| (basis_transform * point * scaled_bounds.GetSize())) | ||
| .Round(); | ||
| position = (screen_glyph_position + (basis_transform * point * |
There was a problem hiding this comment.
Do we still need to round the size somehow? We need to ensure that the size of the area sampled in the shader is exactly the size of sampled texture area, otherwise we can lose a row of pixels or shrink to glyph.
There was a problem hiding this comment.
though maybe rounding of the final position handles that?
There was a problem hiding this comment.
I think we're losing some pixels, here is a side by side on new gallery, pixel 4 pro. Left is patch, right is ToT
There was a problem hiding this comment.
No I don't think so, this has the same problem the other one does. Because the MVP has a scalar in it, rounding here is being performed in the wrong coordinate space. If I put this rounding back the RMSE regresses, lending further evidence to that. We only want to ever round in final pixel space.
There was a problem hiding this comment.
but RMSE aside, it looks wrong unfortunately, so we need to account for that somehow.
There was a problem hiding this comment.
Jonah showed me offline, I can really see the artifact in the "a" in "a list" and the "s" in "list". It looks like one horizontal line has been removed.
There was a problem hiding this comment.
Do we still need to round the size somehow?
Sorry, I looked at this a bit more, the original code is not rounding the size. It was rounding the result if you look at the parenthesis carefully. This should have been the equivalent of the rounding that was happening here, so I'm not sure where this artifact has come from.
The old code would have been rounding in the logical pixel space, whereas the new one is in real pixel space.
There was a problem hiding this comment.
Okay so my intuition on how this works is that we floor the bottom left hand corner and round the other points. A round can be a floor or a ceil depending on the fractional part. By flooring the bottom left we are limiting the chances of a glyph getting compressed when we perform round.
The artifacts are showing up when the glyph is getting compressed, that's why there is something like horizontal line missing in the middle.
I attempted an experiment where I performed the same floor+round approach, but instead of doing it in the current coordinate space I decided to try doing it in the real pixel space. With the debugger I saw that the render pass's orthographic transform had m[0] = 1/512, where the area we are rendering to actually 1024 pixels wide. This seems to suggest the math for all the clamping is in the wrong coordinate space.
The 2x scalar can be derived from the orthographic transform and the render targets size, so I did the following:
--- a/engine/src/flutter/impeller/entity/contents/text_contents.cc
+++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc
@@ -165,6 +165,9 @@ bool TextContents::Render(const ContentContext& renderer,
}
vertex_count *= 6;
+ Matrix logical_to_pixel = Matrix::MakeScale({2, 2, 1});
+ Matrix inv_logical_to_pixel = logical_to_pixel.Invert();
+
BufferView buffer_view = host_buffer.Emplace(
vertex_count * sizeof(VS::PerVertexData), alignof(VS::PerVertexData),
[&](uint8_t* contents) {
@@ -254,14 +257,16 @@ bool TextContents::Render(const ContentContext& renderer,
(glyph_position.position + scaled_bounds.GetLeftTop());
Point screen_glyph_position =
- (screen_offset + unrounded_glyph_position + subpixel_adjustment)
- .Floor();
+ (screen_offset + unrounded_glyph_position +
+ subpixel_adjustment);
for (const Point& point : unit_points) {
Point position;
if (is_translation_scale) {
- position = (screen_glyph_position +
- (basis_transform * point * scaled_bounds.GetSize()))
+ position = inv_logical_to_pixel *
+ ((logical_to_pixel * screen_glyph_position).Floor() +
+ (logical_to_pixel * basis_transform * point *
+ scaled_bounds.GetSize()))
.Round();
} else {
position = entity_transform * (glyph_position.position +This resulting in the artifacts showing up sometimes, and other times not showing up. There's something I'm missing. This however also dropped the RMSE test.
Look at the a in the 2 different "Display" renders.
This comment was marked as outdated.
This comment was marked as outdated.
gaaclarke
force-pushed
the
jitter-text
branch
from
e7dfec2 to
c491fa4
Compare
|
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
changed the title
|
@jonahwilliams I've removed the previous changes since while they help with the jittering, they did so by eliminating pixel snapping erroneously. I'm just going to land in the test for now. This will make se we don't inadvertently regress the issue and when we do land an improvement we'll have the original value in the git history. |
| DlPaint text_paint; | ||
| text_paint.setColor(options.color); | ||
| text_paint.setMaskFilter(options.mask_filter); | ||
| // text_paint.mask_blur_descriptor = options.mask_blur_descriptor; |
There was a problem hiding this comment.
nit: commented out code.
gaaclarke
force-pushed
the
jitter-text
branch
2 times, most recently
from
45d3b04 to
336b2a0
Compare
gaaclarke
force-pushed
the
jitter-text
branch
3 times, most recently
from
12ef084 to
455eb3d
Compare
github-actions
bot
removed
the
a: text input
|
I rebased and deleted all the history for this PR, it was becoming difficult to rebase to |
gaaclarke
force-pushed
the
jitter-text
branch
from
a9b683b to
09f6de2
Compare
github-actions
bot
added
the
a: text input
gaaclarke
force-pushed
the
jitter-text
branch
from
01ea38b to
db3fe29
Compare
|
I'm going to drop this back down to draft. We've seen this test give not clear results. For example, when developing #162049 it gave positive results, then I tweaked the test to have less static pixels and it gave negative results. Most of the work from this PR is landing in #162049 anyways. I'll have to think about this one a big more, it would be nice to have a holistic quantification for this text jitter issue. |
|
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. 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. |
issue: #149652 doc: (currently google only) https://docs.google.com/document/d/1Rulw_noQi0G8Glb47vk17uBbb6sySDxlQe-l-0kHn14/edit?tab=t.0 This increases the RMSE value in the test in #161445 by a slight amount. I do believe this reduces the time where we get non uniform scalars and protects the integrity of relative spacing, thus being more what we expect. There is still a bug that has to do with pixel alignment that does give the illusion of stretching and shrinking though because of hard/soft lines. ## Before https://github.com/user-attachments/assets/e9b80b70-0961-4e02-9053-84d4457348e5 ## After https://github.com/user-attachments/assets/2494a2b1-497d-4a2b-afd7-23064acba293 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…162392) <!-- start_original_pr_link --> Reverts: #162049 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: gaaclarke <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Negatively affected Android rendering ( #162361) <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: gaaclarke <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {jonahwilliams} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: issue: #149652 doc: (currently google only) https://docs.google.com/document/d/1Rulw_noQi0G8Glb47vk17uBbb6sySDxlQe-l-0kHn14/edit?tab=t.0 This increases the RMSE value in the test in #161445 by a slight amount. I do believe this reduces the time where we get non uniform scalars and protects the integrity of relative spacing, thus being more what we expect. There is still a bug that has to do with pixel alignment that does give the illusion of stretching and shrinking though because of hard/soft lines. ## Before https://github.com/user-attachments/assets/e9b80b70-0961-4e02-9053-84d4457348e5 ## After https://github.com/user-attachments/assets/2494a2b1-497d-4a2b-afd7-23064acba293 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
|
Greetings from stale PR triage. 😁 |
|
Ah I see (late) #162049 was reverted. This has a lot of merge conflicts though, will the change move forward here? |
|
Yes, we opted for more precise golden tests. The RMSE value was a bit too fuzzy of an indicator. |
3 participants
issue: #149652
The added RMSE test scales the text then does a save layer scale to un-scale it so that the output between scale values should be the same. This test isn't perfect since there are artifacts introduced when doing the save layer scale.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.