[Impeller] Always use RGBA8888 format in the Skia bitmap used to draw a color glyph atlas#185857
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a unit test for emoji rendering in Impeller and updates the Skia typographer context to explicitly use the RGBA_8888 color type for color bitmaps. A suggestion was made to use SkISize with explicit int32_t casts in the typographer context to maintain consistency with existing code and prevent implicit float-to-int conversions.
| return SkImageInfo::Make(size.width, size.height, kRGBA_8888_SkColorType, | ||
| kPremul_SkAlphaType); |
There was a problem hiding this comment.
For consistency with the kAlpha case (lines 92-93) and to avoid implicit float-to-int conversion of the size components, consider using SkISize with explicit casts to int32_t.
return SkImageInfo::Make(SkISize{static_cast<int32_t>(size.width),
static_cast<int32_t>(size.height)},
kRGBA_8888_SkColorType, kPremul_SkAlphaType);References
- Optimize for readability and maintain consistency in code implementation. (link)
| ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); | ||
| } | ||
|
|
||
| TEST_P(AiksTest, TextDrawEmoji) { |
There was a problem hiding this comment.
Don't we have a test that already does this?
There was a problem hiding this comment.
yeah, it looks like this is redundant with CanRenderEmojiTextFrame. Removed this.
There was a problem hiding this comment.
But it also means this test isn't sufficient, right? It's hardware dependent. Maybe we can add a unit test that calls into one of those static functions directly to make sure that flag is set?
There was a problem hiding this comment.
The goal is to render these emoji with the correct colors, and verifying that involves checking the final output.
Unfortunately the current implementation was relying on Skia behavior that varies based on the host platform. And it looks like tests such as AiksTest.CanRenderEmojiTextFrame are only generating Skia Gold images on macOS, which does not exhibit this issue.
Ideally the infrastructure would provide Skia Gold coverage of a broader range of plaforms including Linux.
Made TypographerContextSkia::GetImageInfo visible for testing and add a test that checks that it returns RGBA.
… a color glyph atlas Impeller declares color bitmap glyph atlas textures as PixelFormat::kR8G8B8A8UNormInt. But it was creating the Skia bitmap for this texture using SkImageInfo::MakeN32Premul. The Skia N32 format may be either BGRA8888 or RGBA8888 depending on what is preferred by the platform. This PR ensures that the Skia bitmap will use RGBA8888 on all platforms. Fixes flutter#185602
Impeller declares color bitmap glyph atlas textures as PixelFormat::kR8G8B8A8UNormInt. But it was creating the Skia bitmap for this texture using SkImageInfo::MakeN32Premul. The Skia N32 format may be either BGRA8888 or RGBA8888 depending on what is preferred by the platform.
This PR ensures that the Skia bitmap will use RGBA8888 on all platforms.
Fixes #185602