Skip to content
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

Closed
1 task done
FizbyteSoftware opened this issue Nov 21, 2023 · 13 comments · Fixed by flutter/engine#52849
Closed
1 task done

[Impeller] Poor performance with lots of emojis. #138798

FizbyteSoftware opened this issue Nov 21, 2023 · 13 comments · Fixed by flutter/engine#52849
Assignees
Labels
e: impeller Impeller rendering backend issues and features requests P1 High-priority issues at the top of the work list slimpeller Engine binary size reduction. go/slimpeller team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@FizbyteSoftware
Copy link

FizbyteSoftware commented Nov 21, 2023

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

  • The issue still persists on the 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]
@FizbyteSoftware FizbyteSoftware added the from: performance template Issues created via a performance issue template label Nov 21, 2023
@danagbemava-nc danagbemava-nc removed the from: performance template Issues created via a performance issue template label Nov 21, 2023
@jonahwilliams
Copy link
Member

Code sample is in #134292 (comment)

@jonahwilliams jonahwilliams changed the title [Impeller] Emoji rendering is jankier in the 3.16.0 vs the lower 3.10.6 [Impeller] Poor performance with lots of emojis. Nov 21, 2023
@jonahwilliams jonahwilliams added e: impeller Impeller rendering backend issues and features requests team-engine Owned by Engine team labels Nov 21, 2023
@emesterhazy
Copy link

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.

@jonahwilliams
Copy link
Member

@FizbyteSoftware your videos aren;t working and you don't have a timeline.

@jonahwilliams jonahwilliams added P2 Important issues not at the top of the work list triaged-engine Triaged by Engine team labels Nov 27, 2023
@FizbyteSoftware
Copy link
Author

Screen.Recording.2023-12-05.at.12.32.01.mov

to me the videos are working fine

auto-submit bot pushed a commit to flutter/engine that referenced this issue Dec 12, 2023
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.
@borjandev
Copy link

This is causing severe performance issues and prevents us from using impeller

More severe reproduction sample below:

import 'dart:math';

void main() {
  runApp(const MaterialApp(
    home: ImpellerSlowFrame(),
  ));
}

class ImpellerSlowFrame extends StatefulWidget {
  const ImpellerSlowFrame({super.key});

  @override
  ImpellerSlowFrameState createState() => ImpellerSlowFrameState();
}

class ImpellerSlowFrameState extends State<ImpellerSlowFrame> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: ListView.builder(
          scrollDirection: Axis.horizontal,
          itemBuilder: (context, index) {
            return SizedBox(
              width: 25,
              child: ListView.builder(
                  scrollDirection: Axis.vertical,
                  itemBuilder: (context, index) {
                    return const RandomEmoji();
                  }),
            );
          }),
    );
  }
}

class RandomEmoji extends StatefulWidget {
  const RandomEmoji({super.key});

  @override
  RandomEmojiState createState() => RandomEmojiState();
}

class RandomEmojiState extends State<RandomEmoji> {
  late String _randomEmoji;

  @override
  void initState() {
    super.initState();
    _randomEmoji = getRandomEmoji();
  }

  String getRandomEmoji() {
    List<List<int>> emojiRanges = [
      [0x1F600, 0x1F64F],
      [0x1F300, 0x1F5FF],
      [0x1F680, 0x1F6FF],
      [0x1F700, 0x1F77F],
    ];

    Random random = Random();
    List<int> selectedRange = emojiRanges[random.nextInt(emojiRanges.length)];
    int emojiCode = random.nextInt(selectedRange[1] - selectedRange[0]) + selectedRange[0];
    return String.fromCharCode(emojiCode);
  }

  @override
  Widget build(BuildContext context) {
    return GestureDetector(
      onTap: () {
        setState(() {
          _randomEmoji = getRandomEmoji();
        });
      },
      child: RichText(
        text: TextSpan(
          text: _randomEmoji,
          style: const TextStyle(fontSize: 25, color: Colors.black),
        ),
      ),
    );
  }
}
skia.MP4
impeller.MP4
impeller_FLTUseFlutterMetalLayer.MP4

I 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

@jonahwilliams
Copy link
Member

This just needs some work to fix in Impeller, there are no work arounds I am aware of.

@jonahwilliams
Copy link
Member

jonahwilliams commented Feb 20, 2024

Basic idea:

  1. Update texture API to allow updating sub regions: ([Impeller] Allow setting the contents of a Texture subregion. engine#50768)
  2. Change incremental atlas update strategy to use this API instead of clearing the texture. ([Impeller] Allow incremental glyph atlas update. engine#50770)
  3. Update criteria for reseting atlas to erase unused entries first.

@Liloupar
Copy link

@jonahwilliams
Does this also impact the rendering performance of CJK text? Since CJK has a vast array of unique character forms, as opposed to English, which only has 26 letters...
thanks

@jonahwilliams
Copy link
Member

Have you observed poor performance with CJK text?

@jonahwilliams
Copy link
Member

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:

  1. We need to fix the thread safety of TextureMTL ([Impeller] TextureMTL::SetContents is unsynchronized and unsafe for updating existing texture. #144498 ). I plan to do that by removing these APIs on TextureMTL and TextureVK and only using blit passes.
  2. Add support to blit pass for updating a subregion of a texture [Impeller] TextureMTL::SetContents is unsynchronized and unsafe for updating existing texture. #144498
  3. Update heurstics as discussed.

There may be more follow ups that will further improve performance:

auto-submit bot pushed a commit to flutter/engine that referenced this issue May 7, 2024
…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!
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 8, 2024
… 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.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 8, 2024
… 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.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 10, 2024
…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.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 13, 2024
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.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 14, 2024
…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.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 15, 2024
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
@jonahwilliams
Copy link
Member

Okay, current state of things. Not quite done yet but its looking much better:

demo.mp4

@jonahwilliams
Copy link
Member

Still jitters when we add lots of new emojis though.

demo.mp4

@chinmaygarde chinmaygarde added the slimpeller Engine binary size reduction. go/slimpeller label May 15, 2024
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 17, 2024
…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.
Copy link

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 flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e: impeller Impeller rendering backend issues and features requests P1 High-priority issues at the top of the work list slimpeller Engine binary size reduction. go/slimpeller team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
Status: Done
Status: ⚡ Performance
7 participants