[Impeller] add support for COLR fonts and bitmap emojis #36161
Conversation
| glyph_atlas_sampler, | ||
| v_unit_vertex * scale_perspective + offset | ||
| ).aaaa * v_text_color; | ||
| if (frag_info.font_has_color == 1.0) { |
There was a problem hiding this comment.
I think this still might not be right, because we can combine COLR and other fonts into a single atlas. When I made this unconditional I noticed that several fonts that were previously specified as white were rendered as black instead in the wonders app
jonahwilliams
changed the title
| SkStrikeSpec strikeSpec = SkStrikeSpec::MakeWithNoDevice(sk_font); | ||
| SkBulkGlyphMetricsAndPaths paths{strikeSpec}; | ||
| auto sk_glyph = paths.glyph(font_glyph.glyph.index); | ||
| *has_color |= sk_glyph->isColor(); |
There was a problem hiding this comment.
We should ask Skia to expose API for this.
| static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas, | ||
| size_t atlas_size) { | ||
| size_t atlas_size, | ||
| bool* has_color) { |
There was a problem hiding this comment.
Rather than having this be an out param, we could figure it out based on the colorType of the bitmap.
There was a problem hiding this comment.
I put this back into the atlas, because I need to track each font + glyph pair anyway
| // Step 6: Upload the atlas as a texture. | ||
| // --------------------------------------------------------------------------- | ||
| auto format = | ||
| has_color ? PixelFormat::kR8G8B8A8UNormInt : PixelFormat::kA8UNormInt; |
There was a problem hiding this comment.
So here something like: FromSk(bitmap->colorType())
|
I've actually already re-written most of this, because I need to track it at the level of the font-glyph pair too so the shader is correct |
|
This needs tests, I thiink we should be able to use emojis in playground. and other things... |
| vec2 scale_perspective = v_atlas_glyph_size / frag_info.atlas_size; | ||
| vec2 offset = v_atlas_position / frag_info.atlas_size; | ||
| if (v_color_glyph == 1.0) { | ||
| frag_color = texture( |
There was a problem hiding this comment.
We need to ignore text color if we're using a bitmap font or colr font
|
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. |
|
I'm pretty sure we have a playground that has an emoji that wasn't rendering right, and now it should .. so this is tested, right? |
|
|
||
| SkStrikeSpec strikeSpec = SkStrikeSpec::MakeWithNoDevice(sk_font); | ||
| SkBulkGlyphMetricsAndPaths paths{strikeSpec}; | ||
| return paths.glyph(font_glyph.glyph.index)->isColor(); |
There was a problem hiding this comment.
nit: file an issue and a TODO here to use some public API from Skia for this.
also nit: maybe this belongs on impeller's Glyph type.
There was a problem hiding this comment.
Todo added. I agree this could probably be refactored. Lets see what we need to do for SDF and large glyphs.
|
Confusingly the test using NotoColorEmoji still doesn't work as expected |
jonahwilliams
added
autosubmit
…)" This reverts commit 61896b3.
2 participants
Conditionally switch the text alas to R8G8B8A8 format if a font/glyph pair claims to have a color. This implies either a bitmap emoji or a color font. Each font-glyph pair also must track whether or not it contains color so we can adjust the vertex shader behavior to drop any specified text color
Before
After
Fixes flutter/flutter#111599
Fixes flutter/flutter#111699