[Impeller] Switch to nearest sampling for the text atlas #39104
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
|
Unfortunately, given the nature of using an atlas, rotated text still looks crunchy.
There are a number of solutions that come to mind. Pick your poison. ;)
|
|
I have some thoughts!
This seems like a good approach at least for now, though hopefully this doesn't need to be done for all text? I'm assuming this won't make us any worse off than we are today.
I'd be really skeptical about any sort of supersampling techniques. Populating the glyph atlas is already very expensive, I'd actually be less worried about the memory usage.
Since rotated text is actually quite rare, I don't think this would be the worst thing. Though we'd go through a lot of churn in the cases where this is animated. You might want something like incremental appends first : #38253 There are a few places in the framework where this happens, like the cupertino picker thingy. Separately:
|
There was a problem hiding this comment.
Turn on linear sampling whenever the transform contains stuff other than a translation.
This seems like the best solution for now. But I'd be curious to see what the Skia behavior is (after we land this).
Does rendering rotated text work better if we're using a signed distance field approach?
We'll have more options but not all text can be rendered using SDF.
| @@ -26,7 +26,8 @@ void main() { | |||
| gl_Position = IPPositionForGlyphPosition( | |||
| frame_info.mvp, unit_position, destination_position, destination_size); | |||
| v_unit_position = unit_position; | |||
| v_source_position = source_position; | |||
| // Pixel snap the source (sampling) start position. | |||
| v_source_position = round(source_position); | |||
There was a problem hiding this comment.
Shouldn't this be floor instead of round?
Eh, nevermind.
|
Right, bitmap emojis and colr fonts. |
SGTM, throwing together a solution for this in a follow-up. We can avoid doing this whenever text is scale/translation only, which is the most common case.
Yes. For SDF-based rendering, we would linear sample the distance from the SDF glyph to determine if we're inside/outside the shape + perform AA ourselves based on the distance. This problem only exists if the AA is pre-rendered by something else and we need to somewhat preserve it. (On this note, we could also turn off AA to lessen the blurriness being added by the non-pixel aligned linear sampling, but this would still have oscillating crunchiness that weaves in an out with the pixel alignment)
I'm not sure if Skia caches things to intermediary atlases and/or SDFs, but what I do know for sure is that it must either path render or generate an SDF as a first step for TTFs, since TTF glyphs are just paths! If it happens to cache path-rendered non-SDF glyphs (which I doubt), then option 3 would seem likely. But then again, it could be doing AA post-op.. |
…119095) * 6aa30b294 Roll Skia from 1bc48bcb1201 to b72fececbdcc (14 revisions) (flutter/engine#39108) * 5405f2c26 [Impeller] Switch to nearest sampling for the text atlas (flutter/engine#39104)
* [Impeller] Switch to nearest sampling for the text atlas (#39104) * [Impeller] Linear sample atlas glyphs when the CTM isn't translation/scale only (#39112) --------- Co-authored-by: Brandon DeRosier <[email protected]>
Resolves flutter/flutter#118613.
Before (top), after (bottom):
Before:
After:
