[Impeller] Conditionally use A8 or R8 format glyph atlas based on capabilities. #50534
Conversation
|
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. |
|
@chinmaygarde @bdero WDUT? |
|
Golden file changes are available for triage from new commit, Click here to view. |
| @@ -130,6 +130,11 @@ struct TexImage2DData { | |||
| external_format = GL_ALPHA; | |||
| type = GL_UNSIGNED_BYTE; | |||
| break; | |||
| case PixelFormat::kR8UNormInt: | |||
There was a problem hiding this comment.
GL_RED is ES 3.0 and above only. A8 was chosen based on the ES 2.0 floors cap.
I agree that we should NOT depend on swizzles however. We observed some pretty dire performance cliffs with swizzles on older drivers. Though, to perform a swizzle via GL_TEXTURE_SWIZZLE_ requires ES 3.0 too. So we can't do it on ES 2.0 (and we shouldn't).
I suggest having a kGrayBitmap that maps to the backend native format and have an ifdef in the shader to pick the right format directly.
There was a problem hiding this comment.
ack, will do some more digging on that. I thought I observed Skia consistently using red bitmaps but that might just be on newer devices.
There was a problem hiding this comment.
👍🏽 go/es2-reference and go/es3-reference might come in handy. I also have Dash docsets for the different versions so checking up flag compatibility is easier. If you use Dash though.
| @@ -169,7 +171,7 @@ static void DrawGlyph(SkCanvas* canvas, | |||
| sk_font.setHinting(SkFontHinting::kSlight); | |||
| sk_font.setEmbolden(metrics.embolden); | |||
|
|
|||
| auto glyph_color = has_color ? SK_ColorWHITE : SK_ColorBLACK; | |||
| auto glyph_color = has_color ? SK_ColorWHITE : SK_ColorRED; | |||
There was a problem hiding this comment.
Shouldn't opaque white do the trick always? Why does Skia care what we interpret the single channel to be?
There was a problem hiding this comment.
I think the problem was that I set the colorspace to red too. This should stay as white/greyscale and then like you said - the backend can choose to interpret the one channel bitmap as either grey or red.
|
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. |
|
From experience working on drivers that are probably obsolete, we should absolutely not venture anywhere near swizzles. But the OpenGL ES 2.0 floor restriction is unfortunate. Can't think of anything other than an ifdef. |
|
@jonahwilliams Another instance from Flutter where we should not use GL_RED even if it is supported flutter/flutter#71315 (comment). |
|
Ooh, just remembered. Once you do get rid of the swizzles, nuke this too. |
| @@ -16,5 +16,5 @@ out f16vec4 frag_color; | |||
|
|
|||
| void main() { | |||
| f16vec4 value = texture(glyph_atlas_sampler, v_uv); | |||
| frag_color = value.aaaa * v_text_color; | |||
| frag_color = value.rrrr * v_text_color; | |||
There was a problem hiding this comment.
I suggested a non-swizzling texture read with an ifdef yesterday, but later realized you could also just do vec4(max(value.a, value.r)) instead. I believe those are even branchless.
There was a problem hiding this comment.
This does not work, as when binding to an R8 texture the alpha channel is 1.0. I'm not sure if this is specified behavior or not. We can use a specialization constant to handle this.
jonahwilliams
changed the title
| if (desc->IsES()) { | ||
| default_glyph_atlas_format_ = PixelFormat::kA8UNormInt; | ||
| } else { | ||
| default_glyph_atlas_format_ = PixelFormat::kR8UNormInt; |
There was a problem hiding this comment.
@jason-simmons I think this shoudl be enough to get desktop GL working.
jonahwilliams
added
the
autosubmit
…sed on capabilities. (flutter/engine#50534)
Run engine_tool tests (flutter/engine#50662) Refactor, update, and move around `testing/scenario_app/README.md` (flutter/engine#50659) Starts a command line tool for assisting engine dev workflows (flutter/engine#50642) macOS: add stubs for PlatformView gesture handling (flutter/engine#50630) [Impeller] Conditionally use A8 or R8 format glyph atlas based on capabilities. (flutter/engine#50534) Roll Dart SDK from e7cfba13d375 to 322e34dc53f6 (1 revision) (flutter/engine#50651) Roll Skia from 4235c0421a48 to eae42ea9f7bc (2 revisions) (flutter/engine#50650) Roll Skia from ea2fd25220cb to 4235c0421a48 (1 revision) (flutter/engine#50648) Roll Dart SDK from 9982d96cebb0 to e7cfba13d375 (1 revision) (flutter/engine#50646) Roll Skia from 1e6e2114f15e to ea2fd25220cb (1 revision) (flutter/engine#50643) Roll Dart SDK from 032323fa534b to 9982d96cebb0 (1 revision) (flutter/engine#50644) Roll Skia from 2aec75cda46e to 1e6e2114f15e (2 revisions) (flutter/engine#50641) Roll Skia from b8acfa559db0 to 2aec75cda46e (1 revision) (flutter/engine#50639) Roll Fuchsia Linux SDK from l6mWjvlO1xJg5ZFKK... to mZP8LxbhYHstUxmxd... (flutter/engine#50638) Roll Skia from 79ec267090bd to b8acfa559db0 (1 revision) (flutter/engine#50636) Roll Skia from d650dcaf4b49 to 79ec267090bd (1 revision) (flutter/engine#50634)
R8 texture format is more widely supported than A8, which we emulate on the vulkan backend with a swizzle. Rather than adding more capability or swizzling checks lets just switch to R8 which is fairly trivial. (I think some desktop gl does not support A8 too, but not sure).
Add Capabilities getDefaultAtlasFormat. Set this to R8 in Vulkan an non-ES GL. A8 Elsewhere.