-
Notifications
You must be signed in to change notification settings - Fork 26.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Poor performance with lots of emojis. #138798
Comments
Code sample is in #134292 (comment) |
I recorded two videos, one with Skia and one with Impeller. I think the jank in the Impeller video is pretty noticeable in comparison. It might be easier to see the difference if you download the videos instead of viewing them in Google Drive's player. |
@FizbyteSoftware your videos aren;t working and you don't have a timeline. |
Screen.Recording.2023-12-05.at.12.32.01.movto me the videos are working fine |
If we need to remake the glyph atlas texture but the size is the same, then reuse the old texture. For more context, see flutter/flutter#138798 which is much slower in Impeller. This change does not fix the problem by itself.
This is causing severe performance issues and prevents us from using impeller More severe reproduction sample below:
skia.MP4impeller.MP4impeller_FLTUseFlutterMetalLayer.MP4I wonder why the UI thread is getting hammered as well on the impeller run? Any ideas @jonahwilliams ? or maybe if there is some way to be able to render the emojis the way that skia does while using impeller? Reproducible on latest master 9f8fe3f and latest stable 3.16.9 iPhone 13 Pro Max on iOS 17.0 Reproducible on MacOS Impeller as well as far as I can tell but maybe needs to be triaged |
This just needs some work to fix in Impeller, there are no work arounds I am aware of. |
Basic idea:
|
@jonahwilliams |
Have you observed poor performance with CJK text? |
We've discussed this a bit in our last weekly: We're going to adjust the caching herustics to better handle overflows, by using the previous glyph atlas to seed the next growth (up to the memory limit). This requires a few changes and refactors:
There may be more follow ups that will further improve performance: |
…52563) Work towards flutter/flutter#138798 Allow cloning a rectangle packer with all existing skylines preserved, but the size increased. This will allow us to preserve the positioning and state of all glyphs when the atlas size is increased, which is necessary for the "blit contents to top right of new texture" approach for improving append performance. EDIT: I mean top left!
… blit pass to set contents in glyph atlas. (#52510) part of flutter/flutter#138798 Works around flutter/flutter#144498 for the glyph atlas. Adds BlitPass::AddCopy implementation for GLES, which is mostly a copy of Texture::SetContents. Updates the glyph atlas to use a blit pass and device private textures instead of set contents (which is unsafely on Metal). This also removes DeviceBuffer::AsTexture, which isn't actually used anywhere - and creates a linear texture on iOS (and fails with an unsupported API on simulators). Note that in general, we don't actually have support for hostVisible textures on GLES _or_ Vulkan. Instead, VMA is falling back to device private textures. We may want to, separately, remove the concept of host visible from textures - and similarly remove the concept of transient from buffers.
… of origin for buffer to texture copies. (#52555) Based on #52510 Work towards flutter/flutter#138798 Change IPoint destination_origin to IRect destination_region, which allows us to specify an area smaller than the entire texture to replace. This will eventually allow us to only upload individual glyphs. This fixes the cubemap issue I previously hit: each face needs to track initialization separately.
…of SkBitmap. (#52567) Work towards part of flutter/flutter#138798 Allow updating single glyphs in the glyph atlas, without replacing the entire bitmap. Required to efficiently append/update to large atlases.
Fixes flutter/flutter#148251 Work towards flutter/flutter#138798 The rectangle packer was only filling up the top and right edge of each rectangle, making the atlas super inefficient.
…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
Okay, current state of things. Not quite done yet but its looking much better: demo.mp4 |
Still jitters when we add lots of new emojis though. demo.mp4 |
…ull. (#52849) Fixes flutter/flutter#138798 As far as I can tell, this gets us about as good as Skia, modulo the actual cost of rasterizing one of these glyphs is much more expensive for us. The basic strategy is: 1. If the existing atlas texture has room (defined by the rect packer addRect call succeeding) append as many glyphs as we can to the old texture. 2. Check if there were any remaining glyphs. If not return. 3. Otherwise, double the size of the texture and create a new rect packer that represents only the increased area. So for example if you doubled a 100x100 texture to 100x200, then you'd create another 100x100 rect packer, assuming the old one is full. 4. Grow this size until all remaining glyphs fit. Store the new rect packer and the "height adjustment", which basically tells you where along the y-axis the rect packer offset starts. 5. Allocate this new texture, clear it, and then blit the contents of the old texture into the top left corner. Append all additional glyphs. The allows us to recycle all previously rendered glyphs and keep them in the texture, reducing the amount of software rasterization per frame. 6. Note; if the max atlas size would be exceeded, throw it away and start again.
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Steps to reproduce
see #134292
The only difference is that the emojis get displayed, which fixed that task, but they are super janky.
(check Video demonstration)
Code sample
Code sample
[Paste your code here]
Performance profiling on master channel
Timeline Traces
Timeline Traces JSON
[Paste the Timeline Traces here]
Video demonstration
Video demonstration
[Upload media here]
IMG_4917.MOV
IMG_4914.MOV
[
What target platforms are you seeing this bug on?
iOS
OS/Browser name and version | Device information
IOS 16.7.2, Iphone X
Does the problem occur on emulator/simulator as well as on physical devices?
Yes
Is the problem only reproducible with Impeller?
Yes
Logs
Logs
[Paste your logs here]
Flutter Doctor output
Doctor output
[Paste your output here]
The text was updated successfully, but these errors were encountered: