[Impeller] remove CPU clear of atlas. #52746
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
| data.external_format, // external format | ||
| data.type, // type | ||
| tex_data // data | ||
| nullptr // data |
There was a problem hiding this comment.
This should be null because otherwise the driver might attempt to read out of bounds, the texture data is actually initialized below.
There was a problem hiding this comment.
Wow, definitely missed that.
|
I think I can test the GLES bug I introduced, one sec |
|
kay done |
|
actually looks like there are still some rendering issues:
|
|
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 debugging, let me try adding the clear back to see if that fixes the issue. if so, I might try another approach. If not, then its a problem with the canvas ops. |
|
agh that cleaned it up. Okay, back to the drawing board. |
|
Golden file changes are available for triage from new commit, Click here to view. |
| data.external_format, // external format | ||
| data.type, // type | ||
| tex_data // data | ||
| nullptr // data |
There was a problem hiding this comment.
Wow, definitely missed that.
|
Lets do #52791 instead. The concern I have is just the CPU cost of a massive emoji atlas, and that PR fixes that just fine. |
…t black. (#52791) Alternative to #52746 Work towards flutter/flutter#138798 If possible, use a render pass to clear texture to transparent black. The goal is to reduce the CPU cost for larger atlases.
Now that we no longer have a full CPU copy of the glyph atlas, it should be cheaper to make a larger initial atlas to reduce churn from resizing seen in applications like flutter/flutter#138798 Requires #52746
2 participants
Linearly sampled glyphs can grab pixels outside of the glyph bounds. if we're only updating smaller regions of the glyph atlas, that texture data could be uninitialized - and the resulting junk would leak into glyphs.
One way to solve this would be to initialize each glyph atlas texture in a render pass. However, there are restrictions in OpenGLES about what formats can be a color attachment.
To solve this initially, I just uploaded a big empy buffer. However, I think we can work around this by placing the padding pixels around the glyph and uploading those too. The padding pixels would then be initialized to transparent.
Before: Glyph is in the top left of its rect, with padding pixels below and to the right.
After: Glyph is centered in its rect, with padding pixels around.
So all pixels that are linearly sampled should at most include the padding pixels which are guaranteed to be initialized.