[Impeller] Workaround for mismatched transform in preroll vs paint for text frames. #164931
Conversation
github-actions
bot
added
a: text input
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
This is looking good. This thing has been a bugger trying to track down. I wish we could come up with a better solution for the TextFrame PerFrameData. Keeping that state around is just asking for bugs.
| Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale); | ||
| Scalar rounded_scale = frame->GetScale(); | ||
| const Matrix transform = | ||
| frame->GetTransform() * Matrix::MakeTranslation(offset); |
There was a problem hiding this comment.
Should this be frame->offset to match TypographerContextSkia::CollectNewGlyphs?
| !TextPropertiesEquals(properties_, properties)) { | ||
| bound_values_.clear(); | ||
| } | ||
| bound_values_.clear(); |
There was a problem hiding this comment.
How is this supposed to work? Should bounds_values_ just be added to SetFrameData?
There was a problem hiding this comment.
Essentially what bound_values_ does is it lets us do a single lookup for the glyph locations of glyphs (instead of multiple). When we check if the glyph atlas is populated with a glyph, we stuff the resulting data into bound_values so that the text frame can re-use it without doing another slow hash lookup.
There was a problem hiding this comment.
What about the second question, should we just always be setting it at the same time instead of having a different method then?
There was a problem hiding this comment.
We don't know the bound_values until after we populate the glyph atlas, which happens necessarily after preroll when this method is called.
| Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale); | ||
| Scalar rounded_scale = frame->GetScale(); | ||
| const Matrix transform = | ||
| frame->GetTransform() * Matrix::MakeTranslation(offset); |
There was a problem hiding this comment.
Can we pull this out into a function that could be shared with TypographerContextSkia::CollectNewGlyphs?
|
The per frame data is pretty important for performance, independent of bugs. |
| const Matrix transform = | ||
| frame->GetTransform() * Matrix::MakeTranslation(frame->GetOffset()); |
There was a problem hiding this comment.
frame->GetOffsetTransform() right?
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM! thanks
This is going to collide with my refactor of scale. You'll probably get this through first, but heads up.
jonahwilliams
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/164931, because - The status or check suite Mac mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
|
looks like a real test failure, need to investigate |
|
So this is potentially drawing the text wrong though, right? We have competing ideas about what the subpixel alignment should be. This code considers the one calculated when creating the glyph atlas is the right one, and the one in TextContents is the wrong one. This will be very difficult to debug since there seems to be a race condition related to it and it will result in text being drawn a subpixel off. I'm not saying we shouldn't have the workaround. I'm just trying to game out trying to track down the root cause. |
|
I don't agree that this is a race condition as this code is all single threaded, its more of a logical invariant error. Currently, we have a need to collect all glyphs (and as such all of their transforms/scales) before we start rasterization. And when we subsequently rasterize, we need the glyph data we ask for to match what is in the atlas. The atlas is always the correct data, because we can't possibly support anything else - we can't on the fly add to the glyph atlas. |
I don't understand it either but I don't think there is another valid explanation for the fact that if I start adding in print statements I can make the bug unable to be reproduced. |
|
When you added in print statements, were you running the application + interacting with it? If so, is it possible it changed the timing such that particular frames weren't rendered? Imagine a scenario where we have some sort of rounding bug and a scale transform is computed as 0.23 in preroll and 0.24 in paint. |
|
Yea, that's plausible. I can tell you it's not some rounding problem though. When the bug happens the offsets that are used to calculate the subpixel alignment are wildly different. Like 10s of pixels different, not some subpixel difference. |
|
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. |
|
I wonder if this is a rotation thing. The matrix used in the |
jonahwilliams
added
the
autosubmit
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…paint for text frames. (flutter/flutter#164931)
…r text frames. (flutter#164931) Fixes flutter#164606 When we preroll the text frame, we record the scale/transform used to compute the size and subpixel position. Use this same size + transform for the subsequent paint so that it is not possible for it to mismatch. This is not really a fix for the underlying issue where the subpixel position may be mismatched.
2 participants
Fixes #164606
When we preroll the text frame, we record the scale/transform used to compute the size and subpixel position. Use this same size + transform for the subsequent paint so that it is not possible for it to mismatch.
This is not really a fix for the underlying issue where the subpixel position may be mismatched.