Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Feb 16, 2023

Previously, the subpass for draw atlas with a blend mode would be as large as the final coverage needed to perform the draw. This was wasteful for a number of reasons, especially if many of the sample rect + color combinatons were repeated. I also suspect that drawing everything in its final position would lead to different rendering when colors would overlap. Finally, we need to use FramebufferBlendContents on iOS to avoid two subpasses.

This PR attempts to solve all of these problems by compressing (via color + sample rect) the drawAtlas call into a second atlas which has no overlapping and positions the sample rects in a very simple lightly packed atlas, decreasing the size of the subpass. In the wonderous app on iPads, this makes the artifact discovery confetti stuff substantially less jank.

Fixes flutter/flutter#120925
flutter/flutter#118914

After:

Anywhere from 2x to 3x as fast as before.

image

@jonahwilliams jonahwilliams force-pushed the improve_draw_atlas_performance branch 2 times, most recently from 7d423a9 to 81a6de7 Compare February 16, 2023 02:42
@jonahwilliams jonahwilliams force-pushed the improve_draw_atlas_performance branch from 81a6de7 to 9756fc9 Compare February 16, 2023 02:48
@jonahwilliams jonahwilliams self-assigned this Feb 16, 2023
@jonahwilliams jonahwilliams changed the title [Impeller] improve atlas performance by reducing size of subpass [Impeller] improve atlas performance by reducing size of subpass. Feb 16, 2023

struct Hash {
std::size_t operator()(const AtlasBlenderKey& key) const {
return fml::HashCombine(key.color.red, key.color.green, key.color.blue,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about this more, there could be some inconsistencies with the hash function and equality implementation given that the latter uses nearly equal. Instead i'll keep the int representation of the color around and use that for both the hash and equality.

@jonahwilliams jonahwilliams changed the title [Impeller] improve atlas performance by reducing size of subpass. [Impeller] Improve atlas blending performance by reducing size of subpass. Feb 16, 2023
@jonahwilliams
Copy link
Contributor Author

Could file a bug for this or write a short doc. Performance issue is apparent in wonderous on iPad.

@chinmaygarde chinmaygarde requested a review from bdero February 16, 2023 21:15
@jonahwilliams
Copy link
Contributor Author

Bug filled

key.rect.size.height);
auto sub_transform = Matrix::MakeTranslation(Vector2(x_offset, y_offset));

x_offset += (key.rect.size.width + 1.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually this logic is probably wrong if the sample rect doesn't have an integral width. I should probably just ceil the width here too.

@chinmaygarde chinmaygarde self-requested a review February 23, 2023 21:13
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optimization makes sense to me and it neatly isolated in the atlas contents. cc @bdero too in case he has thoughts.

struct AtlasBlenderKey {
Color color;
Rect rect;
int32_t color_key;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

int32_t color_key;

static int32_t ToColorKey(Color color) {
return (((std::lround(color.alpha * 255) & 0xff) << 24) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the kEhCloseEnough of the color world. Perhaps we should have an IColor in color.h that does this for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a static helper for this, I don't have a use for the type itself yet.

Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 1, 2023
@auto-submit auto-submit bot merged commit 176e277 into flutter:main Mar 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2023
@jonahwilliams jonahwilliams deleted the improve_draw_atlas_performance branch March 1, 2023 23:05
chinmaygarde pushed a commit to chinmaygarde/flutter_engine that referenced this pull request Mar 3, 2023
…pass. (flutter#39669)

[Impeller] Improve atlas blending performance by reducing size of subpass.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Improve performance of blended drawAtlas calls.

3 participants